On Thu, 19 May 2022 15:57:46 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: > > Platform.exit() , removing code block, as it is causing other test fail The fix looks good with a couple questions about removing entries from the map when they are done and some cleanup comments. In addition to the inline comments I left about the test, I think the following scenarios should be added to the automated test: 1. Multiple listeners single webiew implicit release (i.e., WebView goes out of scope) 2. Multiple listeners multiple webiew with explicit release of one webview and implicit release of the other (this one is basically a combination of the others) 3. Ref count test (single webview is fine) with adding and removing listeners. It should handle the cases of both increasing and decreasing the ref count, and adding the listener to another DOM node after its ref count has gone to zero to make sure that case works. modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 57: > 55: if (it->second && it->second->use_count() == 1) { > 56: delete it->second; > 57: it->second = nullptr; Do you need to remove the item from the list in this case? modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 91: > 89: for (win_it = windowHasEvent.begin(); win_it != windowHasEvent.end(); > win_it++) { > 90: // de register associated event listeners with window > 91: if(window == win_it->second) { Minor: add space after the `if`. modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 92: > 90: // de register associated event listeners with window > 91: if(window == win_it->second) { > 92: unregisterListener(win_it->first); Same question as earlier: do you need to also remove the entries matching the `window` from the list here? modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h line 48: > 46: > 47: class JavaObjectWrapperHandler { > 48: jobject handler_; Have you considered using `JGObject` here instead of raw `jobject`? modules/javafx.web/src/main/native/Source/WebCore/bindings/java/JavaEventListener.cpp line 50: > 48: ? static_cast<const > JavaEventListener*>(&other) > 49: : nullptr; > 50: return jother && ( this == jother); Minor: you can remove the space after `(` modules/javafx.web/src/main/native/Source/WebCore/bindings/java/JavaEventListener.h line 32: > 30: #include "Node.h" > 31: > 32: #if PLATFORM(JAVA) This is a java platform-specific class, so you don't need the `#if PLATFORM(JAVA)` in this file. modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 43: > 41: import javafx.scene.web.WebEngine; > 42: import javafx.scene.web.WebView; > 43: import org.junit.AfterClass; This import is no longer used. modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 53: > 51: import org.junit.Before; > 52: import org.w3c.dom.Document; > 53: import org.w3c.dom.NodeList; Minor: maybe move these up to the previous block (with the ordinary imports? modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 631: > 629: > 630: // Verify that all listeners have not been released > 631: Thread.sleep(100); The sleep should be right before the call to `getClickCount` (as in other cases in the test). modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 637: > 635: }); > 636: > 637: assertEquals("Click count", 1, > listeners1.get(0).getClickCount()); You should add a comment that this check is testing that the immediately previous click does _not_ get delivered since the associated DOM node is not part of the page any more. This is why the count remains at 1 (from the first click on the original page). Also, I think it would be useful here to clear the references to the listeners and WebView and make sure that the listener attached to the previously loaded page for this WebView gets released. Something like this as the final statements of the method: // Clear strong reference to listener and WebView listeners1.clear(); webView1 = null; // Verify that there is no strong reference to the WebView assertNumActive("WebView", webViewRefs, 0); // Verify that no listeners are strongly held assertNumActive("MyListener", listenerRefs, 0); tests/manual/web/EventListenerLeak.java line 1: > 1: import javafx.application.Application; You need to add a copyright header block. tests/manual/web/EventListenerLeak.java line 265: > 263: public static void main(String[] args) { > 264: > 265: // NOTE: this timer is probably not needed It turns out that this is needed to ensure GC, so you can remove the comment. ------------- PR: https://git.openjdk.java.net/jfx/pull/799
