8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path #2647

amosshi · 2024-04-11T06:57:36Z

This PR contains 2 commits
- commit 1 - is the changed from git apply
- commit 2 - is the manual merge based on the ZipFile.java.rej and SetDefaultProvider.java.rej files
  - We respect all the changes of the .rej files,
  - In ZipFile.java - Added missing parameter ZipCoder zc in private static class Key
  - In SetDefaultProvider.java - Fixed Java 11 compile errors
git apply log

github.com/dev-8263940-11
branch 'backport-8263940' set up to track 'origin/backport-8263940'.
Switched to a new branch 'backport-8263940'

patching file 'src/java.base/share/classes/java/util/zip/ZipFile.java'
Reversed (or previously applied) patch detected!  Assume -R? [y] y
2 out of 3 hunks failed--saving rejects to 'src/java.base/share/classes/java/util/zip/ZipFile.java.rej'
patching file 'test/jdk/java/nio/file/spi/SetDefaultProvider.java'
1 out of 3 hunks failed--saving rejects to 'test/jdk/java/nio/file/spi/SetDefaultProvider.java.rej'

git status log

modified:   src/java.base/share/classes/java/util/zip/ZipFile.java
modified:   test/jdk/java/nio/file/spi/SetDefaultProvider.java

Untracked files:
  src/java.base/share/classes/java/util/zip/ZipFile.java.rej
  test/jdk/java/nio/file/spi/SetDefaultProvider.java.rej

src/java.base/share/classes/java/util/zip/ZipFile.java.rej content

@@ -68,7 +69,6 @@
 import jdk.internal.ref.CleanerFactory;
 import jdk.internal.vm.annotation.Stable;
 import sun.nio.cs.UTF_8;
-import sun.nio.fs.DefaultFileSystemProvider;
 import sun.security.util.SignatureFileVerifier;
 
 import static java.util.zip.ZipConstants64.*;
@@ -1255,19 +1255,14 @@
             }
         }
         private static final HashMap<Key, Source> files = new HashMap<>();
-        /**
-         * Use the platform's default file system to avoid
-         * issues when the VM is configured to use a custom file system provider.
-         */
-        private static final java.nio.file.FileSystem builtInFS =
-                DefaultFileSystemProvider.theFileSystem();
+
 
         static Source get(File file, boolean toDelete, ZipCoder zc) throws IOException {
             final Key key;
             try {
                 key = new Key(file,
-                        Files.readAttributes(builtInFS.getPath(file.getPath()),
-                                BasicFileAttributes.class), zc);
+                        Files.readAttributes(file.toPath(), BasicFileAttributes.class),
+                        zc);
             } catch (InvalidPathException ipe) {
                 throw new IOException(ipe);
             }

test/jdk/java/nio/file/spi/SetDefaultProvider.java.rej content

@@ -23,7 +23,7 @@
 
 /**
  * @test
- * @bug 8266345
+ * @bug 4313887 7006126 8142968 8178380 8183320 8210112 8266345 8263940
  * @modules jdk.jartool
  * @library /test/lib
  * @build SetDefaultProvider TestProvider m/* jdk.test.lib.process.ProcessTools

Testing

Local: SetDefaultProvider.java - Test results: passed: 1
Pipeline: All checks have passed
Testing Machine: SAP nightlies passed on 2024-04-29
- Automated jtreg test: jtreg_jdk_tier2
- java/nio/file/spi/SetDefaultProvider.java: SUCCESSFUL GitHub 📊⏲ - [14,363 msec]

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
JDK-8263940 needs maintainer approval

Issue

JDK-8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path (Bug - P3 - Approved)

Reviewers

Lutz Schmidt (@RealLucy - Reviewer) ⚠️ Review applies to 33bd99de
Goetz Lindenmaier (@GoeLin - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/2647/head:pull/2647
$ git checkout pull/2647

Update a local copy of the PR:
$ git checkout pull/2647
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/2647/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2647

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/2647.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-11T06:58:48Z

👋 Welcome back ashi! 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-04-11T06:59:40Z

@amosshi This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path

Reviewed-by: lucy, goetz

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

9363095: 8328194: Add a test to check default rendering engine
81f2413: 8318809: java/util/concurrent/ConcurrentLinkedQueue/WhiteBox.java shows intermittent failures on linux ppc64le and aarch64
f7d0522: 8315889: Open source several Swing HTMLDocument related tests
1e1688a: 8294137: Review running times of java.math tests
d8b297f: 8306067: Open source AWT Graphics,GridBagLayout related tests
9df5688: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
f29aa4d: 8324733: [macos14] Problem list tests which fail due to macOS bug described in JDK-8322653
ac43e68: 8255031: Update java/util/prefs/AddNodeChangeListener.java to report more failure info
7c04756: 8159927: Add a test to verify JMOD files created in the images do not have debug symbols
f3d0f2b: 8306031: Update IANA Language Subtag Registry to Version 2023-04-13
... and 37 more: https://git.openjdk.org/jdk11u-dev/compare/4daa077a4f17729d46497bcd1dcc52bcea7ac6db...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.

openjdk · 2024-04-11T06:59:50Z

This backport pull request has now been updated with issue from the original commit.

mlbridge · 2024-04-11T07:03:07Z

Webrevs

01: Full - Incremental (c772987c)
00: Full (33bd99de)

amosshi · 2024-04-22T18:53:12Z

test/jdk/java/nio/file/spi/SetDefaultProvider.java

@@ -104,7 +104,7 @@ private void createFileSystemProviderJar(Path jar, Path dir) throws IOException
            List<String> paths = stream
                    .map(path -> path.getFileName().toString())
                    .filter(f -> f.startsWith("TestProvider"))
-                    .toList();
+                    .collect(Collectors.toList());
            for(var p : paths) {


Fix compile error on Java 11:

the toList() does not work on 11

change it as collect(Collectors.toList()

amosshi · 2024-04-22T18:59:02Z

src/java.base/share/classes/java/util/zip/ZipFile.java

+                    Key key = (Key) obj;
+                    if (key.utf8 != utf8) {
+                        return false;
+                    }
                    if (!attrs.lastModifiedTime().equals(key.attrs.lastModifiedTime())) {


The private static class Key class does not have the ZipCoder zc constructor parameter, here we add it based on current latest code in https://github.com/openjdk/jdk/

The backported change does not add the zc argumenht to the call in lin 1436.
Can you please explain why you add it? Originally it was added for larger change JDK-8243254

Hi @GoeLin ,

Because in the Line 1436 of the same file, it needs the 3rd parameter zc .

Without this parameter, there will be a compile error.

1436 BasicFileAttributes.class), zc);

In the latest jdk code, the 3d parameter zc has been there already

Well in jdk11u-dev, the parameter was not there yet

Considering this is an internal private class (line 1385)

So we can add this parameter more safely with current Jira ticket.

1385 private static class Key {

Yes, but it is you who added the paramteter zc in line 1436, not the patch you backported.
The backported patch only adds builtInFS.getPath() and changes the line breaks.
So why did you add zc in line 1436? I think you should not add it there, and then you don't need the other additional changes either.

@GoeLin ,

Yes, in the diff the zc was not added.

Well because in the original commit openjdk/jdk@717792c , the zc parameter exits in the base line already. so in jdk11u-dev we were trying to keep in line with the latest code base.

Ok, if the reason is only to get 11 closer to 17, I think it is better to remove zc at the call site and in the callee from this change here. The code belongs to JDK-8243254, but that change is an enhancement and quite large, so it's not a good candidate for backporting.
Here we just want to fix the bug addressed.

RealLucy

Looks good.

openjdk · 2024-04-23T10:05:24Z

⚠️ @amosshi This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

amosshi · 2024-04-23T20:50:18Z

/approval request "Backporting for parity with 11.0.24-oracle. Un-Clean backport. SAP nightlies passed on 2024-04-16,17,19,20”

openjdk · 2024-04-23T20:50:36Z

@amosshi
8263940: The approval request has been created successfully.

GoeLin

LGTM

amosshi · 2024-04-29T06:19:39Z

/integrate

openjdk · 2024-04-29T06:20:20Z

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

9363095: 8328194: Add a test to check default rendering engine
81f2413: 8318809: java/util/concurrent/ConcurrentLinkedQueue/WhiteBox.java shows intermittent failures on linux ppc64le and aarch64
f7d0522: 8315889: Open source several Swing HTMLDocument related tests
1e1688a: 8294137: Review running times of java.math tests
d8b297f: 8306067: Open source AWT Graphics,GridBagLayout related tests
9df5688: 8327136: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java fails on libgraal
f29aa4d: 8324733: [macos14] Problem list tests which fail due to macOS bug described in JDK-8322653
ac43e68: 8255031: Update java/util/prefs/AddNodeChangeListener.java to report more failure info
7c04756: 8159927: Add a test to verify JMOD files created in the images do not have debug symbols
f3d0f2b: 8306031: Update IANA Language Subtag Registry to Version 2023-04-13
... and 37 more: https://git.openjdk.org/jdk11u-dev/compare/4daa077a4f17729d46497bcd1dcc52bcea7ac6db...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-29T06:20:28Z

@amosshi Pushed as commit d42634b.

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

amosshi added 2 commits April 10, 2024 23:55

Backport 717792c3b728584413572e7aede83290779be2a2

3213e39

8263940: Fix compile errors on Java 11

33bd99d

openjdk bot changed the title ~~Backport 717792c3b728584413572e7aede83290779be2a2~~ 8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path Apr 11, 2024

openjdk bot added backport rfr Pull request is ready for review labels Apr 11, 2024

amosshi commented Apr 22, 2024

View reviewed changes

RealLucy approved these changes Apr 23, 2024

View reviewed changes

openjdk bot added the approval label Apr 23, 2024

amosshi closed this Apr 25, 2024

amosshi deleted the backport-8263940 branch April 25, 2024 16:05

amosshi restored the backport-8263940 branch April 26, 2024 08:32

amosshi reopened this Apr 26, 2024

8263940: Remove the un-necessary parameter zc

c772987

openjdk bot added ready Pull request is ready to be integrated and removed approval labels Apr 28, 2024

GoeLin approved these changes Apr 28, 2024

View reviewed changes

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

openjdk bot closed this Apr 29, 2024

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

amosshi deleted the backport-8263940 branch April 29, 2024 06:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path #2647

8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path #2647

amosshi commented Apr 11, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 11, 2024

openjdk bot commented Apr 11, 2024 •

edited

Loading

openjdk bot commented Apr 11, 2024

mlbridge bot commented Apr 11, 2024 •

edited

Loading

amosshi Apr 22, 2024 •

edited

Loading

amosshi Apr 22, 2024

GoeLin Apr 25, 2024

amosshi Apr 25, 2024 •

edited

Loading

GoeLin Apr 26, 2024

amosshi Apr 26, 2024 •

edited

Loading

GoeLin Apr 26, 2024

RealLucy left a comment

openjdk bot commented Apr 23, 2024

amosshi commented Apr 23, 2024

openjdk bot commented Apr 23, 2024

GoeLin left a comment

amosshi commented Apr 29, 2024

openjdk bot commented Apr 29, 2024

openjdk bot commented Apr 29, 2024

8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path #2647

8263940: NPE when creating default file system when default file system provider is packaged as JAR file on class path #2647

Conversation

amosshi commented Apr 11, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 11, 2024

openjdk bot commented Apr 11, 2024 • edited Loading

openjdk bot commented Apr 11, 2024

mlbridge bot commented Apr 11, 2024 • edited Loading

Webrevs

amosshi Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

amosshi Apr 22, 2024

Choose a reason for hiding this comment

GoeLin Apr 25, 2024

Choose a reason for hiding this comment

amosshi Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

GoeLin Apr 26, 2024

Choose a reason for hiding this comment

amosshi Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

GoeLin Apr 26, 2024

Choose a reason for hiding this comment

RealLucy left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 23, 2024

amosshi commented Apr 23, 2024

openjdk bot commented Apr 23, 2024

GoeLin left a comment

Choose a reason for hiding this comment

amosshi commented Apr 29, 2024

openjdk bot commented Apr 29, 2024

openjdk bot commented Apr 29, 2024

amosshi commented Apr 11, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 11, 2024 •

edited

Loading

mlbridge bot commented Apr 11, 2024 •

edited

Loading

amosshi Apr 22, 2024 •

edited

Loading

amosshi Apr 25, 2024 •

edited

Loading

amosshi Apr 26, 2024 •

edited

Loading