RFR: 8088420: JavaFX WebView memory leak via EventListener [v4]
Kevin Rushforth
kcr at openjdk.java.net
Tue May 24 15:46:10 UTC 2022
On Sun, 22 May 2022 14:06:41 GMT, Jay Bhaskar <jbhaskar at openjdk.org> wrote:
>> This PR is new implementation of JavaEvent listener memory management.
>> Issue [JDK-8088420](https://bugs.openjdk.java.net/browse/JDK-8088420?filter=-1)
>>
>> 1. Calling remove event listener does not free jni global references.
>> 2. When WebView goes out of scope (disposed from app) , its Event Listeners are not being garbage collected.
>>
>> Solution:
>> 1. Detached the jni global reference from JavaEventListener.
>> 2. Create scoped ref counted wrapper class JavaObjectWrapperHandler for jni global reference.
>> 3. Create unique JavaObjectWrapperHandler object for each JavaEventListener.
>> 4. EventListenerManager is a singleton class , which stores the JavaObjectWrapperHandler mapped with JavaEventListener.
>> 5. EventListenerManager also stores the JavaEventListener mapped with DOMWindow.
>> 6. When Event listener explicitly removed , JavaEventListener is being forwarded to EventListenerManager to clear the listener.
>> 7. When WebView goes out of scope, EventListenerManager will de-registered all the event listeners based on the ref counts attached with WebView DOMWindow.
>
> Jay Bhaskar has updated the pull request incrementally with one additional commit since the last revision:
>
> Adding JGObject in plave of raw jni object
I left a couple comments and one question on the changes to the fix.
The new tests look good. I left a few comments and suggestions on the test, but overall they are good additions to verify the fix.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 29:
> 27: #include "JavaEventListener.h"
> 28: #include "DOMWindow.h"
> 29: #include <stdio.h>
You probably don't need to include `stdio.h`
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 99:
> 97: }
> 98:
> 99: void EventListenerManager::resetDOMWindow(DOMWindow* window)
Have you validated this logic? If I understand correctly, the `isReferringToOtherListener` var will be true if any listener in the list of listeners for this DOM window has a ref count > 1. Is my understanding correct? This doesn't seem quite right to me, but I may be missing something.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h line 53:
> 51: JavaObjectWrapperHandler(const JLObject& handler) {
> 52: JNIEnv *env = nullptr;
> 53: env = JavaScriptCore_GetJavaEnv();
You can remove these two lines, since `env` is now unused here.
modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h line 60:
> 58: JNIEnv *env = nullptr;
> 59: env = JavaScriptCore_GetJavaEnv();
> 60: env->DeleteGlobalRef(handler_);
I think you can replace these three lines with: `handler_.clear();`
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 96:
> 94: }
> 95:
> 96: static class MyListener1 implements EventListener {
There is no need for another subclass of `EventListener`. It is unused and can be removed.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 630:
> 628: */
> 629: @Test
> 630: public void TestStrongRefNewContentLoad() throws Exception {
You should set `webView2 = null` here
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 657:
> 655: });
> 656:
> 657: // Verify that all listeners have not been released
This comment is not right. It should be something like:
// Verify that the click event is not delivered to the event handler.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 664:
> 662: listeners1.clear();
> 663: domNodes1.clear();
> 664: webViewRefs.clear();
This makes the subsequent call to `assertNumActive` ineffective. There should never be a need for any test method to explicitly modify the global refs lists.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 672:
> 670: // Verify that no listeners are strongly held
> 671: assertNumActive("MyListener", listenerRefs, 0);
> 672: listenerRefs.clear();
It is not necessary to clear the `listenerRefs` list. There is never a need for a test to explicitly modify this list.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 676:
> 674:
> 675: /**
> 676: * Test that the listener ref cont increase on addevent and decrease on remove event
Typo: cont -> count
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 706:
> 704: });
> 705:
> 706: // Verify that the events are delivered to the listeners (0 and 2 are same)
Actually all three refer to the same listener.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 713:
> 711: MyListener tmpListener = listeners.get(0).get();
> 712:
> 713: // remove previously registered listeners fro dom nodes
Typo: fro -> from
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 716:
> 714: submit(() -> {
> 715: domNodes1 = getDomNodes(webView1);
> 716: assertEquals(NUM_DOM_NODES, domNodes1.size());
I don't think you need to repeat these two calls.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 731:
> 729: });
> 730:
> 731: // Verify that the events are delivered to the listeners (0 and 2 are same)
This comment is wrong. What you are verifying is that the events are _not_ delivered, which is why the count remains at 3.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 765:
> 763: assertEquals("Click count", 6, listeners.get(1).get().getClickCount() + listeners.get(0).get().getClickCount());
> 764:
> 765: // add events listeners again
that should be "remove"
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 797:
> 795: // Verify that no listeners are strongly held
> 796: assertNumActive("MyListener", listenerRefs, 0);
> 797: listenerRefs.clear();
No need to clear.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 855:
> 853: // Verify that active listener
> 854: assertNumActive("MyListener", listenerRefs, 0);
> 855: listenerRefs.clear();
No need to clear this list.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 921:
> 919: // Verify that the events are delivered to both webviews
> 920: Thread.sleep(100);
> 921: assertEquals("Click count", 3, listeners.get(0).get().getClickCount());
You might want to assert the counts for the other listeners, too.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 959:
> 957: domNodes2.clear();
> 958: webView2 = null;
> 959: Thread.sleep(100);
This isn't harmful, but isn't needed. The sleep(100) is only used in the tests prior to checking click count. The `assertNumActive` method already does sleep between checks.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 962:
> 960: // Verify that active listener
> 961: assertNumActive("MyListener", listenerRefs, 0);
> 962: listenerRefs.clear();
Same comment as previously about not clearing this list.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 991:
> 989: click(webView1, 0);
> 990: });
> 991:
It would be useful to call `assertNumActive' and verify that there are two active refs.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 1003:
> 1001: click(webView1, 0);
> 1002: });
> 1003:
It would be useful to call `assertNumActive' and verify that there is one active ref.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 1005:
> 1003:
> 1004: // Verify that listener has been released
> 1005: assertEquals("Click count", 1, listeners.get(0).getClickCount());
sleep(100) before this check.
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 1008:
> 1006: assertEquals("Click count", 2, listeners.get(1).getClickCount());
> 1007:
> 1008: // make web view , goes out of scope
Suggestion: "// make WebView go out of scope"
modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 1016:
> 1014: // Verify that active listener
> 1015: assertNumActive("MyListener", listenerRefs, 0);
> 1016: listenerRefs.clear();
Same comment about not needing to clear this.
-------------
PR: https://git.openjdk.java.net/jfx/pull/799
More information about the openjfx-dev
mailing list