8388508: [macOS] MenuItem graphics in the system menu bar is corrupted or disappears, or JVM crashes in CoreGraphics, under memory pressure

[jperedadnr]

This PR creates a native owned copy of the pixel buffer passed to GlassPixels::getImage, to let AppKit start, at any later time, the rasterization of the images for the system menu bars with a native object that outlives the Java buffer. Therefore, if under certain conditions of high memory pressure, the latter is reclaimed and collected, the former is still available.

A manual test has been included.

Since this is a memory pressure/timing issue, it is not deterministic, and depends on several conditions. Therefore, it needs to be run several times to spot the issue (before the proposed fix). After the fix, the issue should never happen again.

When running it from tests/manual/systemmenu, with:

$JAVA_HOME/bin/java @../../../build/run.args MacOSSystemMenuImageGraphicTest.java

it might not trigger the crash, but still the icons might be missing or garbled most of the runs.

However, with:

$JAVA_HOME/bin/javac @../../../build/run.args MacOSSystemMenuImageGraphicTest.java // remove --enable-native-access from run.args
$JAVA_HOME/bin/java @../../../build/run.args MacOSSystemMenuImageGraphicTest

the crash is more likely to happen in different runs.

Other options to increase the likelihood of failure is to use a low Xmx, or to increase the COUNTER value.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

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-8388508: [macOS] MenuItem graphics in the system menu bar is corrupted or disappears, or JVM crashes in CoreGraphics, under memory pressure (Bug - P2)

Reviewers

• Martin Fox (@beldenfox - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2219

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

The total number of required reviews for this PR has been set to 2 based on the presence of this label: rfr. This can be overridden with the /reviewers command.

[mlbridge]

Webrevs

• 00: Full (51bdfa11)

[beldenfox]

I could easily reproduce the bug in the master branch and this PR fixes it. Linux and Windows also copy the data (it's deferred on Windows but it does get copied). The reference counting in this PR looks correct and overall the code looks good.

I think there's an existing bug which leaks one of these NSImages. In GlassPasteboard.m look for jPixelsAttachData (currently on line 270). The NSImage created there is not released and since it's created using alloc it won't be autoreleased either. The two other calls to jPixelsAttachData are releasing their NSImages.

[jperedadnr]

Thanks @beldenfox.

As you say, GlassPasteboard creates a NSImage that is allocated in GlassPixels::getImage (which precisely gets modified in this PR), but never released (while GlassCursor and GlassMenu do release it properly). While that is a clear leak in GlassPasteboard, I fail to see this as related to the same bug/JBS ticket.

While the fix for this leak would be really small (just adding [image release]; to GlassPasteboard), I'm not sure it should be part of this PR? Shouldn't we file a separated JBS ticket for it instead?

[beldenfox]

While the fix for this leak would be really small (just adding [image release]; to GlassPasteboard), I'm not sure it should be part of this PR? Shouldn't we file a separated JBS ticket for it instead?

Yeah, we should probably treat this as a separate bug even though the fix is so small.

[kevinrushforth]

While the fix for this leak would be really small (just adding [image release]; to GlassPasteboard), I'm not sure it should be part of this PR? Shouldn't we file a separated JBS ticket for it instead?

Yeah, we should probably treat this as a separate bug even though the fix is so small.

Yes, please. Can one of you file a new bug for this?

[jperedadnr]

I can do it

[jperedadnr]

I've filed https://bugs.openjdk.org/browse/JDK-8388786 for the NSImage leak.

[beldenfox]

Looks good to me.