Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Mon, 29 Apr 2024 12:44:14 GMT, Joel Uckelman wrote: > 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. - PR Comment: https://git.openjdk.org/jdk/pull/18126#issuecomment-2082703443
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Tue, 5 Mar 2024 17:40:01 GMT, Alexey Ivanov wrote: >> 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\Documents(true) >> b = C:\Users(false) >> c = C:\Users\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. > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Handle fakePersonal on Windows 10 > > The desktop folder on Windows 10 does not include > the Personal (Documents) folder. Will this be backported to Java 21 and 22? It would be very helpful if it could be. - PR Comment: https://git.openjdk.org/jdk/pull/18126#issuecomment-2082632924
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Tue, 5 Mar 2024 17:40:01 GMT, Alexey Ivanov wrote: >> 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\Documents(true) >> b = C:\Users(false) >> c = C:\Users\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. > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Handle fakePersonal on Windows 10 > > The desktop folder on Windows 10 does not include > the Personal (Documents) folder. Marked as reviewed by serb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18126#pullrequestreview-1987018502
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Tue, 5 Mar 2024 17:40:01 GMT, Alexey Ivanov wrote: >> 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\Documents(true) >> b = C:\Users(false) >> c = C:\Users\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. > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Handle fakePersonal on Windows 10 > > The desktop folder on Windows 10 does not include > the Personal (Documents) folder. Any other reviewer? - PR Comment: https://git.openjdk.org/jdk/pull/18126#issuecomment-2043254746
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Tue, 5 Mar 2024 17:40:01 GMT, Alexey Ivanov wrote: >> 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\Documents(true) >> b = C:\Users(false) >> c = C:\Users\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. > > Alexey Ivanov has updated the pull request incrementally with one additional > commit since the last revision: > > Handle fakePersonal on Windows 10 > > The desktop folder on Windows 10 does not include > the Personal (Documents) folder. Marked as reviewed by prr (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18126#pullrequestreview-1961661797
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Mon, 25 Mar 2024 23:48:23 GMT, Joel Uckelman wrote: > 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1539108157
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Thu, 21 Mar 2024 17:01:49 GMT, Joel Uckelman wrote: >> @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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1538374102
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Thu, 21 Mar 2024 12:02:52 GMT, Alexey Ivanov wrote: >> Hi, I'm the submitter of the original bug report. I can confirm that you got >> the reason for the change exactly right. > > @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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1534298705
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Fri, 15 Mar 2024 15:00:40 GMT, Joel Uckelman wrote: >> 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 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1533767996
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Fri, 15 Mar 2024 15:00:40 GMT, Joel Uckelman wrote: >> 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. Thank you, @uckelman, for the detailed analysis. Greatly appreciated. - PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1533761341
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Thu, 14 Mar 2024 16:56:03 GMT, Alexey Ivanov wrote: >> It is well described in a [JBS >> comment](https://bugs.openjdk.org/browse/JDK-8305072?focusedId=14610400=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14610400) >> by the submitter. >> >> Let's consider the failing case where: >> >> a = C:\Users\Documents(true) >> b = C:\Users(false) >> c = C:\Users\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. >> >> https://github.com/openjdk/jdk/blob/7bccc847fe9e8c5e640d8fa9c93ce32563a0ccfa/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java#L534-L535 >> >> Thus, `a` and `c` are considered equal, which breaks the general contract of >> [`Comparable.compareTo`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Comparable.html#compareTo(T)). >> >> If you look below this `if`-statement: >> >> https://github.com/openjdk/jdk/blob/7bccc847fe9e8c5e640d8fa9c93ce32563a0ccfa/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java#L545-L550 >> >> 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1526422961
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Thu, 14 Mar 2024 16:56:03 GMT, Alexey Ivanov wrote: >> It is well described in a [JBS >> comment](https://bugs.openjdk.org/browse/JDK-8305072?focusedId=14610400=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14610400) >> by the submitter. >> >> Let's consider the failing case where: >> >> a = C:\Users\Documents(true) >> b = C:\Users(false) >> c = C:\Users\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. >> >> https://github.com/openjdk/jdk/blob/7bccc847fe9e8c5e640d8fa9c93ce32563a0ccfa/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java#L534-L535 >> >> Thus, `a` and `c` are considered equal, which breaks the general contract of >> [`Comparable.compareTo`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Comparable.html#compareTo(T)). >> >> If you look below this `if`-statement: >> >> https://github.com/openjdk/jdk/blob/7bccc847fe9e8c5e640d8fa9c93ce32563a0ccfa/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java#L545-L550 >> >> 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 @uckelman, Your message is hidden until you accept OpenJDK Terms of Use. - PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1526738043
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Thu, 14 Mar 2024 16:49:39 GMT, Alexey Ivanov wrote:
>> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java
>> line 525:
>>
>>> 523: boolean special2 = sf2.isSpecial();
>>> 524:
>>> 525: if (special1 && special2) {
>>
>> Could you please say something on how/why this change solves it ?
>
> It is well described in a [JBS
> comment](https://bugs.openjdk.org/browse/JDK-8305072?focusedId=14610400=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14610400)
> by the submitter.
>
> Let's consider the failing case where:
>
> a = C:\Users\Documents(true)
> b = C:\Users(false)
> c = C:\Users\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.
>
> https://github.com/openjdk/jdk/blob/7bccc847fe9e8c5e640d8fa9c93ce32563a0ccfa/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java#L534-L535
>
> Thus, `a` and `c` are considered equal, which breaks the general contract of
> [`Comparable.compareTo`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Comparable.html#compareTo(T)).
>
> If you look below this `if`-statement:
>
> https://github.com/openjdk/jdk/blob/7bccc847fe9e8c5e640d8fa9c93ce32563a0ccfa/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java#L545-L550
>
> 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
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1525214351
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Tue, 12 Mar 2024 19:35:30 GMT, Phil Race wrote:
>> Alexey Ivanov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Handle fakePersonal on Windows 10
>>
>> The desktop folder on Windows 10 does not include
>> the Personal (Documents) folder.
>
> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java
> line 525:
>
>> 523: boolean special2 = sf2.isSpecial();
>> 524:
>> 525: if (special1 && special2) {
>
> Could you please say something on how/why this change solves it ?
It is well described in a [JBS
comment](https://bugs.openjdk.org/browse/JDK-8305072?focusedId=14610400=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14610400)
by the submitter.
Let's consider the failing case where:
a = C:\Users\Documents(true)
b = C:\Users(false)
c = C:\Users\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.
https://github.com/openjdk/jdk/blob/7bccc847fe9e8c5e640d8fa9c93ce32563a0ccfa/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java#L534-L535
Thus, `a` and `c` are considered equal, which breaks the general contract of
[`Comparable.compareTo`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Comparable.html#compareTo(T)).
If you look below this `if`-statement:
https://github.com/openjdk/jdk/blob/7bccc847fe9e8c5e640d8fa9c93ce32563a0ccfa/src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java#L545-L550
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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1525204254
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
On Tue, 5 Mar 2024 17:40:01 GMT, Alexey Ivanov wrote:
>> 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\Documents(true)
>> b = C:\Users(false)
>> c = C:\Users\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.
>
> Alexey Ivanov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Handle fakePersonal on Windows 10
>
> The desktop folder on Windows 10 does not include
> the Personal (Documents) folder.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java
line 525:
> 523: boolean special2 = sf2.isSpecial();
> 524:
> 525: if (special1 && special2) {
Could you please say something on how/why this change solves it ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18126#discussion_r1522020052
Re: RFR: 8305072: Win32ShellFolder2.compareTo is inconsistent [v2]
> 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\Documents(true) > b = C:\Users(false) > c = C:\Users\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. Alexey Ivanov has updated the pull request incrementally with one additional commit since the last revision: Handle fakePersonal on Windows 10 The desktop folder on Windows 10 does not include the Personal (Documents) folder. - Changes: - all: https://git.openjdk.org/jdk/pull/18126/files - new: https://git.openjdk.org/jdk/pull/18126/files/7c442382..7bccc847 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18126=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18126=00-01 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18126.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18126/head:pull/18126 PR: https://git.openjdk.org/jdk/pull/18126
