On Wed, 25 May 2022 10:10:08 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: > > Review comments applied My review is not complete. I shall continue later. Providing few comments for now. Please take a look. modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 34: > 32: EventListenerManager& EventListenerManager::get_instance() > 33: { > 34: static NeverDestroyed<EventListenerManager> sharedManager; This does behave well as a singleton class. But I think the instance should be class member variable or a pointer. modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 38: > 36: } > 37: > 38: EventListenerManager::EventListenerManager() { } Empty constructor. Can be moved to .h file. modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 52: > 50: it = listener_lists.find(ptr); > 51: JNIEnv *env = nullptr; > 52: env = JavaScriptCore_GetJavaEnv(); env seems unused. should be removed. modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 109: > 107: isReferringToOtherListener = false; > 108: else > 109: isReferringToOtherListener = true; I have not looked very clearly, but here is a question: Should the loop break when `if` condition passes ? modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 115: > 113: if (window == win_it->second) > 114: windowHasEvent.erase(win_it->first); > 115: } I would change the code to be something like: if (!isReferringToOtherListener) { for (win_it = windowHasEvent.begin(); win_it != windowHasEvent.end(); win_it++) { if (window == win_it->second) windowHasEvent.erase(win_it->first); } } modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h line 36: > 34: #include<map> > 35: #include <wtf/NeverDestroyed.h> > 36: #include<iterator> minor: Add space after `#include`, on line 34, 36. modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.h line 79: > 77: > 78: private: > 79: EventListenerManager(); I would recommend to move it in previous private block. (line 65, 66) 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); `this` will never be null so the statement can be changed to: `return this == jother;` modules/javafx.web/src/main/native/Source/WebCore/dom/Node.cpp line 2151: > 2149: #if PLATFORM(JAVA) > 2150: > EventListenerManager::get_instance().registerDOMWindow(targetNode->document().domWindow(), > 2151: static_cast<JavaEventListener *> > (&listener.copyRef().get())); minor: Alignment correction, should move few spaces to left. ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/799
