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

[Kevin Rushforth]

On Thu, 26 May 2022 07:26:38 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 space for map include

Comments inline.

modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp line 106:

> 104:         else
> 105:             isReferringToOtherListener = true;
> 106:     }

I think I now understand what this is trying to do, but it doesn't looks like it's implemented correctly nor is it optimal.

It seems that the `isReferringToOtherListener` flag is intended to be true iff there is any `JavaEventListener` with a ref count > 1 associated with the Window being unregistered. Ignoring any possible bugs in how it is implemented, there are two problems with this approach. First, you want to remove entries associated with a particular listener if _that_ listener isn't used by another window. So a global "is any listener referring to another window" isn't what you want. Second, since this is multimap, it would seem better to remove individual `(key,value)` pairs associated with the specific window being removed rather than calling erase only for those listeners that aren't being shared at all. If this is feasible, then you wouldn't have to even care if that listener is used by a node in another window. I'm not familiar enough with C++ multimap calls to know how easy this would be, but presumably, it shouldn't be too hard.

Not directly related to this, it might be cleaner to move this logic to `unregisterDOMWindow` instead of having a separate `resetDOMWindow` method, since this is really part of the same conceptual operation.

modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 607:

> 605:      */
> 606:     @Test
> 607:     public void TestStrongRefNewContentLoad() throws Exception {

Minor: method names should start with a lower-case letter.

modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 730:

> 728:         });
> 729: 
> 730:         // Verify that the events are delivered to the listeners (0 and 2 are same)

Minor: all three listeners are the same, not just 0 and 2.

modules/javafx.web/src/test/java/test/javafx/scene/web/EventListenerLeakTest.java line 959:

> 957:         // Verify that the event is delivered to the listener
> 958:         assertEquals("Click count", 1, listeners.get(0).getClickCount());
> 959:         assertEquals("Click count", 1, listeners.get(0).getClickCount());

The second check should be `listeners.get(1)`

-------------

PR: https://git.openjdk.java.net/jfx/pull/799