8305352: updateIconImages may lead to deadlock after JDK-8276849

[dimitryc]

Please review this PR which suggests to skip updateIconImages() for not yet visible windows. The displayChanged notification can be sent to a window that is in the process of initiailization and calling updateIconImages() may lead to the deadlock.

Testing: the deadlock no longer happens with this patch. The reg test for JDK-8276849 works (window icon is updated after changing DPI settings).

---

Progress

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

Issue

• JDK-8305352: updateIconImages may lead to deadlock after JDK-8276849

Reviewers

• Alexey Ivanov (@aivanov-jdk - Reviewer)
• Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13263

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/13263.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@dimitryc The following label will be automatically applied to this pull request:

• client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (117578db)

[prrace]

No objections to a safe fix for this, but the app is creating and - more to the point - showing (setVisible) the AWT window on the main thread.
Even for AWT (non-swing) apps, it is recommended to do it on the EDT to avoid threading issues - such as this.
Lots of apps do use the main thread, and have no issues but that's not guaranteed.

I can't find where this is documented .. I do remember it was quite a number of years ago that this advice was first offered.

I suppose you could not create a reliable test for this ? Please add noreg-hard to the bug.

[openjdk]

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

8305352: updateIconImages may lead to deadlock after JDK-8276849

Reviewed-by: aivanov, serb

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 276 new commits pushed to the master branch:

• b2240bf: 8304696: Duplicate class names in dynamicArchive tests can lead to test failure
• cb158ff: 8296153: Bump minimum boot jdk to JDK 20
• 117c5b1: 8279216: Investigate implementation of premultiplied alpha in the Little-CMS 2.13
• 723037a: 8298048: Combine CDS archive heap into a single block
• d518dbf: 8306440: Rename PSS:_num_optional_regions to _max_num_optional_regions
• 9cd5741: 8306436: Rename PSS*:_n_workers to PSS*:_num_workers
• 6e77e14: 8306456: Don't leak _worklist's memory in PhaseLive::compute
• be6031b: 8303703: Add support of execution tests using virtual thread factory jtreg plugin
• 5a00617: 8306543: GHA: MSVC installation is failing
• 3da987a: 8306075: Micro-optimize Enum.hashCode
• ... and 266 more: https://git.openjdk.org/jdk/compare/9df20600592427550998c6685f103737e3115a51...master

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.

[prrace]

that's a good point. If the window is created and later iconifiied (for example) and then this notification comes in, will it be missed ?

[prrace]

It seems that the root cause is a cyclic dependency between GraphicsPrimitiveMgr and Blit during static initialization

Yes, and there was a bug on this a long time ago, but for bureaucratic reasons it got closed.
It ought to be re-opened : https://bugs.openjdk.org/browse/JDK-6995195

[dimitryc]

that's a good point. If the window is created and later iconifiied (for example) and then this notification comes in, will it be missed ?

Such notifications won't be ignored as isVisible() returns true for iconified windows
Verified the scenario:

• window displayed (WWindowPeer.initialize() calls updateIconImages())
• window iconified
• changed DPI setting (WWindowPeer.updateGC() calls updateIconImages())
• window de-iconified (with properly updated icon)

[mrserb]

Such notifications won't be ignored as isVisible() returns true for iconified windows

But it will be ignored if the window is invisible. It is better to fix the https://bugs.openjdk.org/browse/JDK-6995195

[dimitryc]

But it will be ignored if the window is invisible.

If we show/hide a window and then change DPI settings, then notifications will be ignored. But as soon as we show the window next time, we'll get new updateGC notification artifically initiated from WWindowPeer.show (https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/sun/awt/windows/WWindowPeer.java#L287). Later, when the window will be displayed on the screen, it will have the updated icon without delay.

It's a low-risk change that fixes the regression and keeps the original scenario working. It looks safe to backport to JDK 20 update.

It is better to fix the https://bugs.openjdk.org/browse/JDK-6995195

Created new PR for this issue: #13459

[aivanov-jdk]

Even for AWT (non-swing) apps, it is recommended to do it on the EDT to avoid threading issues - such as this. Lots of apps do use the main thread, and have no issues but that's not guaranteed.

I can't find where this is documented .. I do remember it was quite a number of years ago that this advice was first offered.

I tried to find any documentation stating the threading model for AWT components but I didn't find any. I presume AWT should be thread-safe. A couple of times on code review I saw it had been proposed to drop EventQueue.invokeLater because creating and changing AWT components from main thread is safe.

[aivanov-jdk]

But it will be ignored if the window is invisible.

If we show/hide a window and then change DPI settings, then notifications will be ignored. But as soon as we show the window next time, we'll get new updateGC notification artifically initiated from WWindowPeer.show (https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/sun/awt/windows/WWindowPeer.java#L287). Later, when the window will be displayed on the screen, it will have the updated icon without delay.

It's a low-risk change that fixes the regression and keeps the original scenario working. It looks safe to backport to JDK 20 update.

If the window is not visible, updateGC gets called when the window is shown; if the window is already visible, nothing changes. Thus, the icon will be updated in all the cases.

[mrserb]

If the window is not visible, updateGC gets called when the window is shown; if the window is already visible, nothing changes. Thus, the icon will be updated in all the cases.

Then it looks fine.

[dimitryc]

Thanks for the reviews.

/integrate

[openjdk]

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

• 2985738: 8306773: Problemlist jdk/incubator/vector/ShortMaxVectorTests.java on x86_32
• 31a73b0: 8306714: Open source few Swing event and AbstractAction tests
• 8063aa2: 8306695: Divide by zero in G1Policy::logged_cards_processing_time
• 2471919: 8304036: Use CommandLine class from shared module
• f39641c: 8306682: Open source a few more AWT Choice tests
• b5362da: 8306484: Open source several AWT Choice jtreg tests
• 2ea62c1: 8303276: Secondary assertion failure in AdapterHandlerLibrary::contains during crash reporting
• 0dc03c9: 8305922: [aix,linux] Avoid comparing 'this' to nullptr
• 328e512: 8306780: ProblemList java/lang/Thread/virtual/HoldsLock.java#id0 in Xcomp
• 937dd34: 8306775: Problemlist runtime/Monitor/GuaranteedAsyncDeflationIntervalTest.java
• ... and 296 more: https://git.openjdk.org/jdk/compare/9df20600592427550998c6685f103737e3115a51...master

Your commit was automatically rebased without conflicts.

[openjdk]

@dimitryc Pushed as commit f968da9.

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