JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin #1331

Maran23 · 2024-01-11T20:13:09Z

For some reason the skinProperty did not allow to set a new skin when it is the same class as the previous one.
This leads to multiple issues:

When creating a new skin (same class as previous), the skin will likely install listener but is then rejected when setting it due to the skinProperty class constraint
-> PopupControl is in a weird state as the current skin was not disposed since it is still set, although we already created and 'set' a new skin
A skin of the same class can behave differently, so it is not really valid to reject a skin just because it is the same class as the previous
-> Just imagine we have the following skin class

class MyCustomSkin<C extends PopupControl> extends Skin<C> {
    public MyCustomSkin(C skinnable, boolean someFlag) { ... }
}

Now if we would change the skin of the PopupControl two times like this:

popup.setSkin(new MyCustomSkin(popup, true));
popup.setSkin(new MyCustomSkin(popup, false));

The second time the skin will be rejected as it is the same class, although I may changed the skin behaviour, in this case demonstrated by a boolean flag for showing purposes.

This is the same issue and fix as done for Control in JDK-8276056 (PR: #806)

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-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Michael Strauß (@mstr2 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1331

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

Using diff file

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

Webrev

Link to Webrev Comment

…on discarded Skin

bridgekeeper · 2024-01-11T20:14:07Z

👋 Welcome back mhanl! 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 · 2024-01-11T20:27:33Z

Webrevs

00: Full (2452f36b)

andy-goryachev-oracle · 2024-01-11T20:35:08Z

modules/javafx.controls/src/main/java/javafx/scene/control/PopupControl.java

@@ -213,18 +213,6 @@ public PopupControl() {
        // a reference to the old value.
        private Skin<?> oldValue;

-        @Override


This logic was introduce to deal with skins set from the css JDK-8096194

Are you sure it will still work? Do we need to test this scenario explicitly?

No, the skin/skinProperty logic was there before, just moved.
It was added in https://bugs.openjdk.org/browse/JDK-8127070,
with the comment from David Grieve: The code for handling the skin property in PopupControl needs to look like the code in Control.

So they basically synchronized the whole skin/skin name API to PopupControl.

andy-goryachev-oracle · 2024-01-12T19:53:10Z

/reviewers 2

openjdk · 2024-01-12T19:54:10Z

@andy-goryachev-oracle
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).

andy-goryachev-oracle · 2024-01-12T19:56:32Z

I think the proposed fix is correct. I've tried to re-load skin via CSS (see https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/bugs/PopupControl_SetSkin_8323615.java ) and it does seem to reload correctly, as far as I can tell.

Loading fails without the fix, as expected.

Still, i would like a second pair of eyes to look at this, if possible.

andy-goryachev-oracle · 2024-01-12T19:53:31Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/PopupControlTest.java

+
+        assertNotEquals("New skin was not set", oldSkin, newSkin);
+    }
+


minor: please remove extra newline, I'll reapprove if you choose to make the change

kevinrushforth · 2024-01-20T00:05:22Z

@arapte Can you be the second reviewer?

mstr2

Looks good, and works as I expect it.

openjdk · 2024-01-30T05:37:00Z

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

8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin

Reviewed-by: angorya, mstrauss

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

876d535: 8324239: JFXPanelHiDPITest fails on Windows 11
767b027: 8324797: Code example in JavaDoc of ObservableValue#when doesn't compile
359ffb9: 8308955: MediaPlayer/AudioClip skip data on seek/loop
9e7e8e1: 8324270: Update boot JDK to 21.0.2
1170658: 8322784: JFXPanel calls InputMethodRequests on wrong thread
ff4f8e3: 8324237: Typo in comment in GlassApplication.m
c1c52e5: 8322215: [win] OS events that close the stage can cause Glass to reference freed memory
3cf9390: 8323879: constructor Path(Path) which takes another Path object fail to draw on canvas html
872dbc8: 8323543: NPE when table items are set to null
e2bc52d: Merge
... and 13 more: https://git.openjdk.org/jfx/compare/caba6788e2b62b3c1af102d1ff311ac2eaeab8c5...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.

arapte · 2024-02-01T12:09:36Z

@Maran23
This is ready for integration. If you were waiting for me. Thanks, It looks good, I don't have any comments.

Maran23 · 2024-02-01T14:10:08Z

@Maran23 This is ready for integration. If you were waiting for me. Thanks, It looks good, I don't have any comments.

Thank you for checking as well!

/integrate

openjdk · 2024-02-01T14:10:51Z

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

af7e057: 8324879: Platform-specific preferences keys are incorrect for Windows toolkit
40809a3: 8322703: Intermittent crash in WebView in a JFXPanel from IME calls on macOS
c5ab220: 8324658: Allow animation play/start/stop/pause methods to be called on any thread
52840a1: 8324337: Cherry-pick WebKit 617.1 stabilization fixes
d653a31: 8260013: Snapshot does not work for nodes in a subscene
876d535: 8324239: JFXPanelHiDPITest fails on Windows 11
767b027: 8324797: Code example in JavaDoc of ObservableValue#when doesn't compile
359ffb9: 8308955: MediaPlayer/AudioClip skip data on seek/loop
9e7e8e1: 8324270: Update boot JDK to 21.0.2
1170658: 8322784: JFXPanel calls InputMethodRequests on wrong thread
... and 18 more: https://git.openjdk.org/jfx/compare/caba6788e2b62b3c1af102d1ff311ac2eaeab8c5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-01T14:10:59Z

@Maran23 Pushed as commit aac2df1.

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

JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() …

2452f36

…on discarded Skin

openjdk bot added the rfr Ready for review label Jan 11, 2024

andy-goryachev-oracle reviewed Jan 11, 2024

View reviewed changes

andy-goryachev-oracle approved these changes Jan 12, 2024

View reviewed changes

kevinrushforth requested a review from arapte January 20, 2024 00:04

mstr2 approved these changes Jan 30, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jan 30, 2024

openjdk bot added the integrated Pull request has been integrated label Feb 1, 2024

openjdk bot closed this Feb 1, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Feb 1, 2024

Maran23 deleted the 8323615-popup-control-skin branch July 29, 2024 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin #1331

JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin #1331

Maran23 commented Jan 11, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 11, 2024

mlbridge bot commented Jan 11, 2024

andy-goryachev-oracle Jan 11, 2024 •

edited

Loading

Maran23 Jan 11, 2024

andy-goryachev-oracle commented Jan 12, 2024

openjdk bot commented Jan 12, 2024

andy-goryachev-oracle commented Jan 12, 2024

andy-goryachev-oracle Jan 12, 2024 •

edited

Loading

kevinrushforth commented Jan 20, 2024

mstr2 left a comment

openjdk bot commented Jan 30, 2024

arapte commented Feb 1, 2024

Maran23 commented Feb 1, 2024

openjdk bot commented Feb 1, 2024

openjdk bot commented Feb 1, 2024

JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin #1331

JDK-8323615: PopupControl.skin.setSkin(Skin) fails to call dispose() on discarded Skin #1331

Conversation

Maran23 commented Jan 11, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jan 11, 2024

mlbridge bot commented Jan 11, 2024

Webrevs

andy-goryachev-oracle Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Maran23 Jan 11, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle commented Jan 12, 2024

openjdk bot commented Jan 12, 2024

andy-goryachev-oracle commented Jan 12, 2024

andy-goryachev-oracle Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

kevinrushforth commented Jan 20, 2024

mstr2 left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 30, 2024

arapte commented Feb 1, 2024

Maran23 commented Feb 1, 2024

openjdk bot commented Feb 1, 2024

openjdk bot commented Feb 1, 2024

Maran23 commented Jan 11, 2024 •

edited by openjdk bot

Loading

andy-goryachev-oracle Jan 11, 2024 •

edited

Loading

andy-goryachev-oracle Jan 12, 2024 •

edited

Loading