On Sun, 22 May 2022 14:06:41 GMT, Jay Bhaskar <jbhas...@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
