8088420: JavaFX WebView memory leak via EventListener

[jaybhaskar]

This PR is new implementation of JavaEvent listener memory management.
Issue JDK-8088420

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.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8088420: JavaFX WebView memory leak via EventListener

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/799/head:pull/799
$ git checkout pull/799

Update a local copy of the PR:
$ git checkout pull/799
$ git pull https://git.openjdk.java.net/jfx pull/799/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 799

View PR using the GUI difftool:
$ git pr show -t 799

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/799.diff

[bridgekeeper]

👋 Welcome back jbhaskar! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 08: Full - Incremental (c609ad9c)
• 07: Full - Incremental (79f30067)
• 06: Full - Incremental (d6fd438b)
• 05: Full - Incremental (4c8e4a5b)
• 04: Full - Incremental (2a09a847)
• 03: Full - Incremental (43f8e549)
• 02: Full - Incremental (5dc52378)
• 01: Full - Incremental (2d5604bf)
• 00: Full (a62558ea)

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with 1 of role reviewers, 1 of role authors).

[kevinrushforth]

While testing, I noticed one problem in that the new unit tests calls Platform.exit() (this was my fault for adding it when sending you the test case), causing any subsequent web test to fail.

[kevinrushforth]

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.

[kevinrushforth]

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.

[arapte]

My review is not complete. I shall continue later. Providing few comments for now.
Please take a look.

[kevinrushforth]

I left a couple comments on one of your test changes.

I think the only remaining question (aside from the few minor comments that Ambarish left) is around the logic in EventListenerManager::resetDOMWindow.

[kevinrushforth]

Comments inline.

[kevinrushforth]

The changes look good. While doing my final review I noticed one more thing that I think should be changed.

[kevinrushforth]

Looks good.

[arapte]

LGTM

[openjdk]

@jaybhaskar This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8088420: JavaFX WebView memory leak via EventListener

Reviewed-by: kcr, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

• d677003: 8286256: Update libxml2 to 2.9.14
• 19a855e: 8286261: Selection of non-expanded non-leaf treeItem grows unexpectedly when adding two-level descendants
• 18b2366: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @arapte) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[jaybhaskar]

/integrate

[openjdk]

@jaybhaskar
Your change (at version c609ad9) is now ready to be sponsored by a Committer.

[kevinrushforth]

/sponsor

[openjdk]

Going to push as commit f534850.
Since your change was applied there have been 3 commits pushed to the master branch:

• d677003: 8286256: Update libxml2 to 2.9.14
• 19a855e: 8286261: Selection of non-expanded non-leaf treeItem grows unexpectedly when adding two-level descendants
• 18b2366: 8285197: TableColumnHeader: calc of cell width must respect row styling (TreeTableView)

Your commit was automatically rebased without conflicts.

[openjdk]

@kevinrushforth @jaybhaskar Pushed as commit f534850.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.