JDK-8089681 : WebView leaks memory when containing object acts as javascript callback handler
  • Type: Bug
  • Component: javafx
  • Sub-Component: web
  • Affected Version: 8,9
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2015-01-29
  • Updated: 2020-02-24
  • Resolved: 2016-03-11
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8 JDK 9
8u102Fixed 9Fixed
Related Reports
Duplicate :  
Duplicate :  
Duplicate :  
Duplicate :  
Duplicate :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Sub Tasks
JDK-8171401 :  
Description
I use a WebView that is contained in an extension of a Pane class (lets call it the view) and then use the  view as the javascript callback handler. If I now close the application window without quiting (no implicit exit) the view and all of its memory is not reclaimed.

Here is some example code with a large long[] array that takes about 80MB of RAM so the leak is easy to spot in jvisualvm:



package bugreports;

import javafx.application.Application;
import javafx.application.Platform;
import javafx.concurrent.Worker;
import javafx.scene.Scene;
import javafx.scene.layout.BorderPane;
import javafx.scene.web.WebView;
import javafx.stage.Stage;
import netscape.javascript.JSObject;

public class WebEngineNotReleasingMemory extends Application
{
	static final String html = "<!DOCTYPE html>" +
			"<meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-16\">" +
			"<html>" +
			"<button type=\"button\" onClick=\"javaCallback.sayWorld()\">Hello</button>" +
			"</html>";

	public static final class CallbackView extends BorderPane
	{
		private final long[] memoryWaster = new long[10000000];


		private final WebView view = new WebView();

		public CallbackView()
		{
			view.getEngine().getLoadWorker().stateProperty().addListener((ov, o, n) -> {
				if (n == Worker.State.SUCCEEDED)
				{
					final JSObject window = (JSObject) view.getEngine().executeScript("window");
					assert window != null;
					window.setMember("javaCallback", this);
				}
			});

			view.getEngine().loadContent(html);
			setCenter(view);
		}

		// called from JavaScript
		public void sayWorld()
		{
			System.out.println("World!");
		}
	}

	@Override
	public void start(final Stage primaryStage) throws Exception
	{
		Platform.setImplicitExit(false);

		final CallbackView pane = new CallbackView();
		primaryStage.setScene(new Scene(pane));
		primaryStage.show();
	}

}



How to reproduce:
1) Run the application
2) Close the window
3) Connect with jvisualvm to the application and force garbage collection, memory is not being reclaimed


Comments
Changeset: 42b461505f27 Author: mbilla Date: 2016-03-11 12:34 -0800 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/42b461505f27
11-03-2016

Looks fine, +1
11-03-2016

Looks great. All tests run, too. +1 Unless Alexander has further comments, I will push version .11
11-03-2016

webrev: http://cr.openjdk.java.net/~mbilla/8089681/webrev.11/ Incorporated above review comments.
11-03-2016

.10 looks good with just two minor problems that I found by code inspection and that I missed earlier: 1) JNIUtility.cpp: result is uninitialized in the following: jvalue result; + + // Since object is WeakGlobalRef, creating a localref to safeguard instance() from GC + JLObject jlinstance(object, true); + + if (!jlinstance) { + LOG_ERROR("Could not get javaInstance for %p in JNIUtility::callJNIMethod", jlinstance); + return result; + } + This is an existing problem (see default case), but should be fixed. 2) JavaFieldJSC.cpp replace abort() calls with "return;" if the JLOjbect is null, since it isn't a catastrophic failre.
11-03-2016

webrev: http://cr.openjdk.java.net/~mbilla/8089681/webrev.10/ Tested Unit test cases ( :web:test) and no crashes observed.
11-03-2016

At least one of the web unit tests crashes with the .09 patch. I will send the crash logs.
11-03-2016

webrev: http://cr.openjdk.java.net/~mbilla/8089681/webrev.09/
10-03-2016

[~kcr] The file - modules/web/src/main/native/Source/WebCore/bridge/jni/jni_jsobject.mm is not used currently. Regarding modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JNIUtilityPrivate.cpp - I added the check at one more place in this file in 08 version. webrev: http://cr.openjdk.java.net/~mbilla/8089681/webrev.08/
10-03-2016

@Alexander: I spoke with Murali and he prefers to keep it as it. For one thing, the return value isn't the same in all cases. I guess he could do with with two different macros -- one for returning nothing from a void function and one that takes the error return value (which the caller would have to provide), but the change would be quite intrusive and would take more testing. I am OK with either approach. @Murali: During my final review, I was looking for other possible uses of instance() that might also need this treatment. I see that you made changes to the following file, but I see other uses of jobject as well. I presume you checked all of them to make sure they are OK? modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JNIUtilityPrivate.cpp Also, did you check the following file? modules/web/src/main/native/Source/WebCore/bridge/jni/jni_jsobject.mm I don't know if this is even used, but it looks like it has similar code in it to those you changed. If you are satisfied that the above are not problems, then I am fine with the .07 version and will approve it.
10-03-2016

I have a minor suggestion, there are many repeating blocks like 77 // Since jfield is WeakGlobalRef, creating a localref to safeguard instance() from GC 78 JLObject jlfield(jfield, true); 79 80 if (!jlfield) { 81 LOG_ERROR("Could not get javaInstance for %p in JavaField::valueFromInstance", jlfield); 82 return jsresult; 83 } and they can be replaced with a macros(for function names we can use __FUNCTION__). The fix itself looks good to me.
09-03-2016

I will finish my review later today.
09-03-2016

webrev: http://cr.openjdk.java.net/~mbilla/8089681/webrev.07/ I removed the redundant JLObject creation from JNIUtility,h Creation of JLObject ref in callJNIMethodIDA and callJNIMethod is not needed as callers of these methods already have JLObject ref.
08-03-2016

It looks like .06 fixes most of the issues and it now compiles on Mac, but still has issue #1 regarding the changes in JNIUtility.h. The changes in the callA and callV methods are either unnecessary or incomplete. Either the caller always checks, in which case the JLObject reference is redundant and should be removed, or there are some cases where the caller might not check, in which case you must check for a null pointer to avoid a crash. The creation of JLObject ref in callJNIMethodIDA and callJNIMethod might be needed (please verify). If so, then you need a null pointer check. Other than that it looks good to me. One question: have you double-checked that all accesses to wrapped objects -- meaning jobject references that are stored in and retrieved from JobjectWrapper -- first create a JLObject and check for null? It might be good for someone else to go through it too in case they might catch a case that you or I missed. And of course this still needs more testing.
08-03-2016

webrev: http://cr.openjdk.java.net/~mbilla/8089681/webrev.06/
07-03-2016

On the code changes in version 05: 1. The changes in JNIUtility.h don't look right. If those methods really can be called with a weak ref, then you need to check the newly-created local ref for null after you create it or it will crash. The problem, of course, is what to return in the case of methods that expect to return a value of a given type? I think it would be better to ensure that the callers always ensure that the reference is valid (and there is a local ref holding onto it so it won't be garbage collected). That is the approach taken in the other changes you made. 2. In JNIUtilityPrivate.cpp I have a similar question about whether obj (input param) can really be a weak global ref. In this case, though, there is no harm in your change, since it correctly checks for null. 3. Minor: You missed a couple places in JavaFieldJSC.cpp to add a comment (the second JLObject in each method) JLObject jlinstance(jinstance, true); 4. Minor: In at least a couple places (e.g., JavaInstanceJSC.cpp) the comment about creating a local ref is in the wrong place (that is, it isn't next to the creation of the local ref))
05-03-2016

I get compilation errors on the Mac (it compiles OK on Windows, but this suggests a missing #include or similar issue). make[2]: *** [obj/JNIUtility.o] Error 1 In file included from ../../../../src/main/native/Source/WebCore/bridge/jni/JobjectWrapper.cpp:28: In file included from ../../../../src/main/native/Source/WebCore/bridge/jni/JobjectWrapper.h:32: ../../../../src/main/native/Source/WebCore/bridge/jni/JNIUtility.h:70:9: error: unknown type name 'JLObject' JLObject jlinstance(obj, true); ^ ... make[2]: *** [obj/JobjectWrapper.o] Error 1 make[1]: *** [sub-TargetJava-pri-make_first-ordered] Error 2 make: *** [sub-WebCore-WebCoreJava-pro-make_first-ordered] Error 2 :web:compileNativeMac FAILED
04-03-2016

webrev: http://cr.openjdk.java.net/~mbilla//8089681/webrev.05/ [~kcr] : Incorporated above suggested changes. I checked that clear() method is called for every JLObject reference we create. Also added extra checks in below new files compared to 04 version : modules/web/src/main/native/Source/WebCore/bridge/jni/JNIUtility.cpp modules/web/src/main/native/Source/WebCore/bridge/jni/JNIUtility.h modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JavaClassJSC.cpp modules/web/src/main/native/Source/WebCore/bridge/jni/jsc/JNIUtilityPrivate.cpp
04-03-2016

Review comments: 1) I found a missed case in the following method: JSValue JavaInstance::defaultValue 2) Do you think it would be cleaner to use the jlobject in the actual calls rather than just relying on the fact that it is created and will remain valid until it goes out of scope? If you do prefer the current approach (which is fewer lines of code change, but maybe less clear), then please add a comment above the JLObject assignment as to why you are doing it, so someone doesn't come along later and "optimize it" out. I could see a compiler generating a lint warning about an unused variable. 3) I recommend to name the second parameter in the JobjectWrapper constructor and create method as "useGlobalRef" since that is its purpose (currently accesscontrolcontext is the only example, but in the future I could imagine others, and it's clearer).
04-03-2016

Tested top 10 websites with webrev.04 and did not observed any issues.
03-03-2016

webrev: http://cr.openjdk.java.net/~mbilla/8089681/webrev.04/ The crash in the above mentioned scenario is fixed.
02-03-2016

I haven't reviewed the code itself carefully, but I did some testing. It fixes the leak, but it also crashes in at least one case if a reference is released (gets garbage collected) and is no longer reachable before the first time it is used from JavaScript. If it used at least once before being garbage collected, then it does not crash. So there may be some other place where a local reference and a check for null is needed. I will send a crash dump and modified test program.
02-03-2016

webrev: http://cr.openjdk.java.net/~mbilla/8089681/webrev.03/
01-03-2016

I discussed this offline with Murali and he will generate a new webrev: 1) The accessControlContext still needs a hard global ref (rather than a weak ref) or it will be subject to premature garbage collection. 2) there are whitespace issues (tabs and trailing whitespace) that need to be fixed.
01-03-2016

webrev: http://cr.openjdk.java.net/~mbilla/8089681/webrev.02/ Note: in JavaField::setValueToInstance method, abort() is called in case of return error in order to keep inline with default switch case in the same method.
01-03-2016

Regarding the checking of the WeakReference, ArunPrasad is correct that you cannot just check for null. Based on my understanding of the JNI docs for weak references, you will need to have a pattern of doing something like the following every time you go to use the weak global ref: 1) Create a new LocalRef from the WeakGlobalRef (so it doesn't get GC'ed out from under you) 2) Check whether the local ref is null (which it will be if the underlying object has been freed). You might need to free the local ref explicitly if the native call did not originate from Java via a JNI call (otherwise it will still leak). You can read the section on local refs, global ref, and weak global refs here: http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#weak_global_references
01-03-2016

Currently incorporated the review comments and testing the patch.. Today I will send the new patch for review.
01-03-2016

[~arajkumar] 1. Regarding "PLATFORM(JAVA)", i will remove the check guards. 2. Regarding checking of jobject (m_instance), here goal is to check whether m_instance is already deleted by VM or not. If m_instance is deleted , hence we wanted to check for m_instance existence. In this case, we need not check exactly whether m_instance is WeakPtr type or not, since we already know that m_instance is of WeakRef Ptr type. I dont see any issues by using weakptr in this case. I run the GC and GC is not de-allocating the weak ref when the windows is alive.
29-02-2016

1. Remove all "PLATFORM(JAVA)" guards. All these files are applicable only in JavaFX WebKit port. if (!jinstance) { + LOG_ERROR("Could not get javaInstance for %p", jinstance); + return jsresult; + } 2. Simple null validation is not proper way to validate jobject from JNI side. You have to use GetObjectRefType[1] JNI function to validate it. If it is a WeakPtr type, the mentioned function should return JNIWeakGlobalRefType enum value. Also using WeakPtr may not work in all the cases. We have to find a better way to fix it. [1] http://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#objreftype
29-02-2016

Webrev link: http://cr.openjdk.java.net/~mbilla/8089681/webrev.00/ Basic testing is done and observed from the logs that javascript callback destructors are called and js instance objects are being deleted.
29-02-2016

Apart from the suggested work around in application side, there is a fix available in source code. I tested the fix and observed that once application hides, javascript callback instance count is zero. Once testing completes, will send the patch for review.
28-02-2016

Below code is the Work around from application (by using weak reference via callbackview delegate ) /* * To change this license header, choose License Headers in Project Properties. * To change this template file, choose Tools | Templates * and open the template in the editor. */ package notrleasingmemory; import java.lang.ref.WeakReference; import javafx.application.Application; import javafx.application.Platform; import javafx.concurrent.Worker; import javafx.scene.Scene; import javafx.scene.layout.BorderPane; import javafx.scene.web.WebView; import javafx.stage.Stage; import netscape.javascript.JSObject; public class WebEngineNotReleasingMemory extends Application { static final String html = "<!DOCTYPE html>" + "<meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-16\">" + "<html>" + "<button type=\"button\" onClick=\"javaCallback.sayWorld()\">Hello</button>" + "</html>"; public static final class CallbackView extends BorderPane { private final long[] memoryWaster = new long[10000000]; private final WebView view = new WebView(); public CallbackView() { view.getEngine().getLoadWorker().stateProperty().addListener((ov, o, n) -> { if (n == Worker.State.SUCCEEDED) { final JSObject window = (JSObject) view.getEngine().executeScript("window"); assert window != null; window.setMember("javaCallback", new CallbackViewDelegate(this)); } }); view.getEngine().loadContent(html); setCenter(view); } // called from JavaScript public void sayWorld() { System.out.println("World!"); } } public static class CallbackViewDelegate { private final WeakReference<CallbackView> callbackviewref; CallbackViewDelegate(CallbackView callbackview) { callbackviewref = new WeakReference<>(callbackview); } public void sayWorld() { CallbackView callbackview = callbackviewref.get(); if (callbackview != null) { callbackview.sayWorld(); } } } @Override public void start(final Stage primaryStage) throws Exception { Platform.setImplicitExit(false); final CallbackView pane = new CallbackView(); primaryStage.setScene(new Scene(pane)); primaryStage.show(); } public static void main(String[] args) { Application.launch(args); } }
26-02-2016

See - updates to JDK-8149707.
20-02-2016

One of my customers opened BugDB Bug 22720281 - WebView leaks memory when containing object acts as javascript callback handler - saying this is a critical issue for them. They are porting their application over to JavaFX 8. This bug is show stopper for them. Can this be fixed in JDK 9, so the fix can be backported over to JDK 8?
20-02-2016