JDK-8088420 : JavaFX WebView memory leak via EventListener
  • Type: Bug
  • Component: javafx
  • Sub-Component: web
  • Affected Version: jfx11,8,jfx17
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2013-05-14
  • Updated: 2023-09-08
  • Resolved: 2022-05-30
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 Other
8u333Fixed jfx11.0.17Fixed
Related Reports
Relates :  
Description
Hi,

I found out that calling eventtarget.removeEventListener doesn't remove the Eventlistener so it's being garbage collected. 

If you run this code:
import javafx.application.Application;
import javafx.beans.value.ChangeListener;
import javafx.beans.value.ObservableValue;
import javafx.concurrent.Worker.State;
import javafx.event.ActionEvent;
import javafx.event.EventHandler;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.HBox;
import javafx.scene.layout.Pane;
import javafx.scene.web.WebView;
import javafx.stage.Stage;

import org.w3c.dom.events.Event;
import org.w3c.dom.events.EventListener;
import org.w3c.dom.events.EventTarget;

public class WebViewTest extends Application{

	private Scene scene;	
	private Pane top;
	private Pane bottom;
	private EventListener listener;

	public static void main(String[] args) throws Exception {
		launch(args);
	}

	@Override
	public void start(Stage primaryStage) throws Exception {

		primaryStage.setScene(new Scene(new BorderPane()));
		scene = primaryStage.getScene();
		top = new HBox();
		((BorderPane)scene.getRoot()).setTop(top);
		bottom = new HBox();
		((BorderPane)scene.getRoot()).setBottom(bottom);
		addWebView();
		Button but = new Button();
		but.setText("add and remove eventlistener");
		but.setOnAction(new EventHandler<ActionEvent>() {

			@Override
			public void handle(ActionEvent event) {
				addWebView();				
			}
		});
		bottom.getChildren().add(but);
		primaryStage.show();

	}

	public void addWebView() {
		top.getChildren().clear();
		final WebView webView = new WebView();
		top.getChildren().add(webView);
		webView.getEngine().loadContent("HELLO");
		listener = new EventListener() {

			@Override
			public void handleEvent(Event evt) {
			}
		};
		webView.getEngine().getLoadWorker().stateProperty().addListener(new ChangeListener<State>() {

			@Override
			public void changed(ObservableValue<? extends State> arg0, State arg1, State arg2) {
				if (arg2 == State.SUCCEEDED) {
					EventTarget target = (EventTarget) webView.getEngine().getDocument().getDocumentElement();
					target.addEventListener("keydown",listener , false);
					webView.getEngine().getLoadWorker().stateProperty().removeListener(this);
					target.removeEventListener("keydown",listener , false);
					listener = null;
				}

			}
		});
	}
}

And clicking on the button for example 10 times, then when you look in for example JProfiler, so can see 10 instances of EventListenerImpl, and it never decreases.. Isn't this a bug?

Best regards
Troels
Comments
A pull request was submitted for review. URL: https://git.openjdk.org/jfx11u/pull/105 Date: 2022-08-17 13:33:12 +0000
17-08-2022

A pull request was submitted for review. URL: https://git.openjdk.org/jfx17u/pull/73 Date: 2022-08-09 12:13:46 +0000
09-08-2022

Changeset: f5348503 Author: Jay Bhaskar <jbhaskar@openjdk.org> Committer: Kevin Rushforth <kcr@openjdk.org> Date: 2022-05-30 12:33:53 +0000 URL: https://git.openjdk.java.net/jfx/commit/f5348503143e8d08f09b4cd48b6a3864bd09c336
30-05-2022

A pull request was submitted for review. URL: https://git.openjdk.java.net/jfx/pull/799 Date: 2022-05-19 13:13:01 +0000
19-05-2022

NOTE: We have determined that "case 2" as described in the old comments above, regarding the "legacy" setOnxxx methods, is no longer relevant. The code in question -- the Java side setOnXxxxx and getOnXxxx methods and their implementation in EventListenerImpl -- is dead code that can no longer be reached. If it ever used to be an actual problem (rather than merely a theoretical one), it no longer is. Only "case 1", which was the subject of this bug report as filed, needs to be considered.
09-05-2022

We've had another report of this leak, so I'll take a look at fixing it.
19-02-2022

Handling the "legacy" setOnxxx methods will be somewhat painful. Basically, we need to make some changes to CodeGeneratorJava.pm. Consider ElementImpl#setOnkeyup, which is (generated code): public void setOnkeyup(EventListener value) { setOnkeyupImpl(getPeer(), EventListenerImpl.getPeer(value)); } where setOnkeyupImpl is this native method. JNIEXPORT void JNICALL Java_com_sun_webkit_dom_ElementImpl_setOnkeyupImpl(JNIEnv* env, jclass clazz, jlong peer, jlong value) { static_cast<Element*>(jlong_to_ptr(peer)) ->setOnkeyup(static_cast<EventListener*>(jlong_to_ptr(value))); } Element::setOnkeyup is defined using the macro DEFINE_ATTRIBUTE_EVENT_LISTENER as: void setOnkeyup(PassRefPtr<EventListener> listener) { setAttributeEventListener(KEYUP_NAME, listener); } \ where KEYUP_NAME is an AtomicString for "keyup". To fix this, we need to pass the EventListener value to setOnkeyupImpl: public void setOnkeyup(EventListener value) { setOnkeyupImpl(getPeer(), value, EventListenerImpl.getPeer(value)); } Then setOnkeyupImpl needs to call setAttributeEventListener directly. Something vaguely like following: JNIEXPORT void JNICALL Java_com_sun_webkit_dom_ElementImpl_setOnkeyupImpl(JNIEnv* env, jclass clazz, jlong peer, jobject jvalue, jlong value) { JavaEventListener jelistener* = static_cast<JavaEventListener*>(jlong_to_ptr(value)); if (static_cast<Element*>(jlong_to_ptr(peer)) ->setAttributeEventListener(KEYUP_NAME,jelistener)) jelistener.addGlobalRef(jvalue); } The 'if' is because setAttributeEventListener returns true if the listener was new (i.e. not previously registered). The new JavaEventListener::addGlobalRef increments the listener reference count, and if necessary creates the JNI GlobalRef from the JavaEventListener to the Java EvenetListener jvalue. This leaves out the complication that we have to decrement the reference count if we replace an existing listener. Bottom line: this looks doable, but it seems a fair amount of tedious and tricky work to get right. I'll get started on it - unless you think there is something higher-priority for me to work on.
11-06-2013

Classes and dependencies, in-depth: MyListener (the user's Java class that implements org.w3c.dom.events.EventListener) com.sun.webkit.dom.EventListenerImpl implements org.w3c.dom.events.EventListener Has a field: private final EventListener eventListener; An instance of EventListenerImpl is created in case (2) [of the previous comment] - i.e. when we need a Java representention/wrapper around a JavaScript event-listener. (In this case the eventListener field is null.) An EventListenerImpl is also created in case (1) [of the previous comment], in which case the eventListener field points to the MyListener. (There doesn't seem to be any real reason for allocating an EventListenerImpl in this case - it's just an extra layer of indirection.) (The rest of this note assumes case (1) - i.e. we start with a Java EventListener.) The C++ EventListener peer is created by EventListenerImpl#getPeer. Assuming there is no existing peer (after looking up in the static WeakHashMap EL2peer), it first creates a (semi-useless) EventListenerImpl. #getPeer calls twkCreatePeer, which creates the native JavaEventListener. The JavaEventListener extends WebCore::EventListener. It has a JGObject m_joListener field, which is a JNI GlobalRef to the EventListenerImpl, which in turn has a hard link to MyListener. These links are needed so the MyListener#handleEvent can be called when an event triggers. It needs to be hard (non-weak) link, since there event might still need to be handled, even if MyListener is no longer accessible from the Java world. This causes an uncollectible cycle: - The JNI GlobelRef points to the EventListenerImpl. - The EventListenerImpl points to MyListener. - The EL2peer WeakHashMap maps the MyListener, to a Long, which is the address of the JavaEventListener peer. - The JavaEventListener points to the JNI GlobelRef. (In addition the peer2EL HashMap contains a back-reference from the peer to the MyListener. However, this is a WeakReference, so it isn't a problem - though it is also useless in case (1).) The way to fix the cycle is to remove the JNI GlobelRef when the listener is no longer registered with any EventTarget. However, actually doing this is not an easy fix, as I will explain in the next comment.
11-06-2013

The Java class EventListenerImpl and the C++ class JavaEventListener are used in two different cases: (1) When a Java EventListener object needs to be passed to the native WebKit event classes, for example when in the implementation of Node#addEventListener. (Note the difference between "JavaEventListener" (a C++ class) and a "Java EventListener" (a Java object that implements the Java EventListener interface).) (2) When a JavaScript event listener needs to be passed to Java code - for example in DocumentImpl#getOnload. (In this case we return a EventListenerImpl that proxies the JavaScript handler.) I'm concerned here with case (1). The problem shown by RT-30374 is that each addEventListener creates a JavaEventListener, but removeEventListener does not delete it, even after GC, because the reference count never does to zero. The reference count for the JavaEventListener is 1 for the reference from Java, and 1 for each reference from the C++ event system (i.e. entry in EventListenerMap). Thus after the addEventListener the RC is 2, and a later removeEventListener reduces the RC to 1. The problem is the Java EventListener can never be collected: The JavaEventListener has a strong JNI global reference to the EventListenerImpl, whose eventListener field points to the Java EventListener. Thus we never get the RC down to 0. It's not clear to me how to deal with this: We cannot collect the Java EventListener as long as it referenced from Java *or* it is needed by an active event handler. We cannot delete the JavaEventListener in the same situation. One solution in theory is to hook into the ref/deref. For example when the count goes to one then we can call DeleteGlobalRef on the back-reference to the EventListenerImpl. (Or rather replace it by WeakGlobalRef, since the listener might be re-registered.) Then we use the Disposer mechanism to decrement the JavaEventListener RC. It would be relatively easy if the only times a Java EventListener was registered was with Node#addEventListener and Node#removeEventListener. However, we also have to deal with methods like ElementImpl#getOnload and #setOnload.
11-06-2013

I reproduced this. jhat does report multiple EventListenerImpl instances. Specifically, it reports these are JNI global references. Which gives us a good clue what the problem is. I'll dig deeper tomorrow.
31-05-2013