8301219: JavaFX crash when closing with the escape key

[beldenfox]

While processing a key down event the Glass GTK code sends out PRESSED and TYPED KeyEvents back to back. If the stage is closed during the PRESSED event the code will end up referencing freed memory while sending out the TYPED event. This can lead to intermittent crashes.

In GlassApplication.cpp the EventCounterHelper object ensures the WindowContext isn't deleted while processing an event. Currently the helper object is being created after IME handling instead of before. If the IME is enabled it's possible for the WindowContext to be deleted in the middle of executing a number of keyboard-related events.

The fix is simple; instantiate the EventCounterHelper object earlier. There isn't always a WindowContext so I tweaked the EventCounterHelper to do nothing if the context is null.

To make the crash more reproducible I altered the WindowContext such that when it's deleted the freed memory is filled with 0xCC. This made the crash more reproducible and allowed me to test the fix. I did the same with GlassView since that's the only other Glass GTK class that's instantiated with new and discarded with delete.

---

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)

Issues

• JDK-8301219: JavaFX crash when closing with the escape key (Bug - P4)
• JDK-8087368: java runtime environment error when trying to execute showAndWait() function (Bug - P3)

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)
• Thiago Milczarek Sayao (@tsayao - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1307/head:pull/1307
$ git checkout pull/1307

Update a local copy of the PR:
$ git checkout pull/1307
$ git pull https://git.openjdk.org/jfx.git pull/1307/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1307

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1307.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mfox! 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

• 02: Full - Incremental (09f8ede5)
• 01: Full - Incremental (6a4a4e63)
• 00: Full (ccb60e84)

[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 at least 1 Reviewer, 1 Author).

[beldenfox]

/issue JDK-8087368

[openjdk]

@beldenfox
Adding additional issue to issue list: 8087368: java runtime environment error when trying to execute showAndWait() function.

[kevinrushforth]

The fix looks good. I tested it by reverting your fix and leaving in the debug memory fill on delete. The system test added by this PR crashes without the fix and passes with the fix. So do the tests associated with the fixed bugs.

I also ran a headful test run on our lab systems. All good.

As for the debug fill-on-delete code, it seems useful to have it available, but not enabled in production. I recommend to ifdef it out on a flag, either DEBUG or some unique flag defined in DeletedMemDebug.h, but off by default.

[kevinrushforth]

LGTM now. I did ask one question and will reapprove if you make the change.

[kevinrushforth]

Should this use FILL instead of hard-coded 0xcc?

[beldenfox]

Yes, it should use FILL. Will fix.

[kevinrushforth]

@tsayao Would you like to review this? If not, can someone else be the second reviewer?

[tsayao]

The change looks correct to me - the change makes sense, but I can't reproduce the crash on my systems.
I can confirm that it's reaching the delete ctx correctly.

I didn't know about the 0xCC debug trick.

[tsayao]

Looks good.

[openjdk]

@beldenfox 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:

8301219: JavaFX crash when closing with the escape key
8087368: java runtime environment error when trying to execute showAndWait() function

Reviewed-by: kcr, tsayao

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 1 new commit pushed to the master branch:

• ab68b71: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[beldenfox]

/integrate

[openjdk]

Going to push as commit 2493a23.
Since your change was applied there has been 1 commit pushed to the master branch:

• ab68b71: 8321970: New table columns don't appear when using fixed cell size unless refreshing tableView

Your commit was automatically rebased without conflicts.

[openjdk]

@beldenfox Pushed as commit 2493a23.

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