RFR: 8088420: JavaFX WebView memory leak via EventListener [v5]

[Ambarish Rapte]

On Wed, 25 May 2022 10:10:08 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:
> 
>   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