8305072: Win32ShellFolder2.compareTo is inconsistent #18126

aivanov-jdk · 2024-03-05T17:01:45Z

The implementation of Win32ShellFolder2.compareTo is inconsistent: there are cases where
a < b & b < c but a == c
which violates its general contract.

In particular, it happens for the personal folder (Documents) if it is listed twice: as a special and as a regular folder.

The evaluation performed by the submitter of the bug provided enough details to create a test, reproduce the problem and finally resolve it.

Without the fix, the regression test always fails:

a < b & b < c but a >= c
where
  a = C:\Users\<user>\Documents(true)
  b = C:\Users\<user>(false)
  c = C:\Users\<user>\Documents(false)

as well as for the reverse case: a > b & b > c.

How it is possible to have the same folder in a list of files twice remains unknown. I believe it is another bug in JDK, however, no one has been able to reproduce it so far.

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-8305072: Win32ShellFolder2.compareTo is inconsistent (Bug - P4)

Reviewers

Phil Race (@prrace - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18126

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-05T17:02:25Z

👋 Welcome back aivanov! 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 · 2024-03-05T17:03:53Z

@aivanov-jdk 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 · 2024-03-05T17:07:01Z

Webrevs

01: Full - Incremental (7bccc847)
00: Full (7c442382)

The desktop folder on Windows 10 does not include the Personal (Documents) folder.

prrace · 2024-03-12T19:35:30Z

src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java

@@ -522,7 +522,7 @@ static int compareShellFolders(Win32ShellFolder2 sf1, Win32ShellFolder2 sf2) {
        boolean special1 = sf1.isSpecial();
        boolean special2 = sf2.isSpecial();

-        if (special1 || special2) {
+        if (special1 && special2) {


Could you please say something on how/why this change solves it ?

It is well described in a JBS comment by the submitter.

Let's consider the failing case where:

a = C:\Users\<user>\Documents(true) b = C:\Users\<user>(false) c = C:\Users\<user>\Documents(false)

The value in parentheses is isSpecial() flag.

When a and c are compared, special1 = true and special2 = false, therefore the code flow enters this if-block. Because the equals method doesn't take into account the isSpecial flag, both i1 and i2 are 0.

jdk/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java

Lines 534 to 535 in 7bccc84

int i1 = topFolderList.indexOf(sf1);

int i2 = topFolderList.indexOf(sf2);

Thus, a and c are considered equal, which breaks the general contract of Comparable.compareTo.

If you look below this if-statement:

jdk/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java

Lines 545 to 550 in 7bccc84

// Non-file shellfolders sort before files

if (special1 && !special2) {

return -1;

} else if (special2 && !special1) {

return 1;

}

The comment and the code say that any special folder sorts above any regular folder. Therefore the code shouldn't enter that if-block above if only one of the folders is special — the order is already well-defined for such case.

The purpose of the if-block is to make some of the special folders even more special. In other words, it ensures the following special folders—getPersonal, getDesktop, getDrives, getNetwork—are at the top of the list, above any other special folders.

So, changing the condition in the if-block from special1 || special2 to special1 && special2 makes the sorting order consistent because the relation is now transitive:

a < b & b < c therefore a < c

Hi, I'm the submitter of the original bug report. I can confirm that you got the reason for the change exactly right.

@uckelman, Your message is hidden until you accept OpenJDK Terms of Use.

Thank you, @uckelman, for the detailed analysis. Greatly appreciated.

@uckelman Is there a way to reproduce the original bug in the user's environment?

I think there's a bug in JDK which leads to two Documents folders in the list. If the bug is still reproducible, collecting more data will help us identify the problem and fix it.

If I can get in contact with the user, I expect he'd still be able to trigger the bug with my test case, yeah. I'll try to get his attention.

I haven't heard back yet. I hope investigating the second bug will not hold up the fix for the first bug getting merged.

I haven't heard back yet. I hope investigating the second bug will not hold up the fix for the first bug getting merged.

It's not a show-stopper. These are different bugs. If possible, I'd like to follow up and find the root cause of why there are two identical folders in the file list.

openjdk · 2024-03-13T20:11:45Z

@aivanov-jdk 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:

8305072: Win32ShellFolder2.compareTo is inconsistent

Reviewed-by: prr, 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 532 new commits pushed to the master branch:

dd930c5: 8329787: Fix typo in CLDRConverter
115f419: 8329659: Serial: Extract allowed_dead_ratio from ContiguousSpace
9ac3b77: 8329775: Serial: Remove unused declarations in serialFullGC.hpp
7475824: 8329510: Update ProblemList for JFileChooser/8194044/FileSystemRootTest.java
6439375: 8329533: TestCDSVMCrash fails on libgraal
3ebf8c9: 8329663: hs_err file event log entry for thread adding/removing should print current thread
be45de1: 8328627: JShell documentation should be clearer about "remote runtime system"
8648890: 8329749: Obsolete the unused UseNeon flag
fc18201: 8327111: Replace remaining usage of create_bool_from_template_assertion_predicate() which requires additional OpaqueLoop*Nodes transformation strategies
7c66465: 8325088: Overloads that differ in type parameters may be lost
... and 522 more: https://git.openjdk.org/jdk/compare/8f6edd8dc866bf970b7e7b8358f62832887e6e8b...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.

aivanov-jdk · 2024-04-08T17:05:54Z

Any other reviewer?

aivanov-jdk · 2024-04-09T13:18:38Z

/integrate

openjdk · 2024-04-09T13:19:44Z

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

635cb3c: 8329745: Update the documentation of ServerSocket and Socket to refer to StandardSocketOptions instead of legacy SocketOptions
5c9f036: 8329858: G1: Make G1VerifyLiveAndRemSetClosure stateless
492b954: 8329750: Change Universe functions to return more specific Klass* types
87131fb: 8329629: GC interfaces should work directly against nmethod instead of CodeBlob
5ea21c3: 8329878: Reduce public interface of CardTableBarrierSet
a48289a: 8329761: Remove unused KeyBuilder and unusedSets from StyleContext
8907eda: 8325485: IncrementInstructions.of(int, int) is not storing the args
b9331cd: 8329823: RISC-V: Need to sync CPU features with related JVM flags
71c5bbc: 8329527: Opcode.IFNONNULL.primaryTypeKind() is not ReferenceType
58e39c1: 8329884: Serial: Fix build failure due to ‘Copy’ has not been declared
... and 542 more: https://git.openjdk.org/jdk/compare/8f6edd8dc866bf970b7e7b8358f62832887e6e8b...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-09T13:19:50Z

@aivanov-jdk Pushed as commit 2fcb816.

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

uckelman · 2024-04-29T12:44:14Z

Will this be backported to Java 21 and 22? It would be very helpful if it could be.

aivanov-jdk · 2024-04-29T13:11:02Z

Will this be backported to Java 21 and 22? It would be very helpful if it could be.

It is already backported to 22.
I'm working on backporting it to all supported Oracle releases of Java. It is up to the OpenJDK community to backport the changeset into OpenJDK-based releases.

aivanov-jdk added 2 commits March 5, 2024 16:06

8305072: Add test case for Win32ShellFolder2.compareTo

77659e7

8305072: Fix Win32ShellFolder2.compareTo

7c44238

openjdk bot added the rfr Pull request is ready for review label Mar 5, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Mar 5, 2024

Handle fakePersonal on Windows 10

7bccc84

The desktop folder on Windows 10 does not include the Personal (Documents) folder.

prrace reviewed Mar 12, 2024

View reviewed changes

prrace approved these changes Mar 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 26, 2024

mrserb approved these changes Apr 8, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 9, 2024

openjdk bot closed this Apr 9, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 9, 2024

aivanov-jdk deleted the 8305072-Win32ShellFolder2.compareTo branch April 9, 2024 13:19

uckelman mentioned this pull request Apr 29, 2024

java.lang.IllegalArgumentException: Comparison method violates its general contract! skylot/jadx#1628

Open

This was referenced Apr 29, 2024

Critical error when install netbeans (Comparison method violates its general contract!) apache/netbeans#6339

Closed

can not open a project or any file chooser (Comparison method violates its general contract!) apache/netbeans#6295

Closed

mmdanggg2 mentioned this pull request Jun 18, 2024

getting errors jmc2obj/j-mc-2-obj#254

Open

avently mentioned this pull request Sep 18, 2024

[Bug]: Again. The application crashes after trying to save a file. simplex-chat/simplex-chat#4831

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8305072: Win32ShellFolder2.compareTo is inconsistent #18126

8305072: Win32ShellFolder2.compareTo is inconsistent #18126

aivanov-jdk commented Mar 5, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 5, 2024

openjdk bot commented Mar 5, 2024

mlbridge bot commented Mar 5, 2024 •

edited

Loading

prrace Mar 12, 2024

aivanov-jdk Mar 14, 2024 •

edited

Loading

aivanov-jdk Mar 14, 2024

uckelman Mar 15, 2024 •

edited by bridgekeeper bot

Loading

aivanov-jdk Mar 15, 2024

aivanov-jdk Mar 21, 2024

aivanov-jdk Mar 21, 2024

uckelman Mar 21, 2024

uckelman Mar 25, 2024

aivanov-jdk Mar 26, 2024

openjdk bot commented Mar 13, 2024 •

edited

Loading

aivanov-jdk commented Apr 8, 2024

aivanov-jdk commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

uckelman commented Apr 29, 2024

aivanov-jdk commented Apr 29, 2024

	int i1 = topFolderList.indexOf(sf1);
	int i2 = topFolderList.indexOf(sf2);

	// Non-file shellfolders sort before files
	if (special1 && !special2) {
	return -1;
	} else if (special2 && !special1) {
	return 1;
	}

8305072: Win32ShellFolder2.compareTo is inconsistent #18126

8305072: Win32ShellFolder2.compareTo is inconsistent #18126

Conversation

aivanov-jdk commented Mar 5, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 5, 2024

openjdk bot commented Mar 5, 2024

mlbridge bot commented Mar 5, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

aivanov-jdk Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uckelman Mar 15, 2024 • edited by bridgekeeper bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Mar 13, 2024 • edited Loading

aivanov-jdk commented Apr 8, 2024

aivanov-jdk commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024

uckelman commented Apr 29, 2024

aivanov-jdk commented Apr 29, 2024

aivanov-jdk commented Mar 5, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 5, 2024 •

edited

Loading

aivanov-jdk Mar 14, 2024 •

edited

Loading

uckelman Mar 15, 2024 •

edited by bridgekeeper bot

Loading

openjdk bot commented Mar 13, 2024 •

edited

Loading