8297301: Cleanup unused methods in JavaUtilJarAccess

[pandaapo]

The cache named signerToCodeSource in JarVerifier is never used now.

---

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-8297301: Cleanup unused methods in JavaUtilJarAccess

Reviewers

• Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11072

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

Using diff file

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

[bridgekeeper]

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

@pandaapo The following labels will be automatically applied to this pull request:

• core-libs
• security

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

[mlbridge]

Webrevs

• 06: Full - Incremental (1aa65b2b)
• 05: Full - Incremental (fb933804)
• 04: Full - Incremental (a8fb419c)
• 03: Full - Incremental (c4cfb499)
• 02: Full - Incremental (ef4ba3df)
• 01: Full - Incremental (b3dbd279)
• 00: Full (59d5c534)

[wangweij]

/label remove core-libs

[openjdk]

@wangweij
The core-libs label was successfully removed.

[wangweij]

The method is not called since JDK 9, and that's why I set Affected Version/s to 8-pool. We don't intend to fix this in the current release.

[pandaapo]

The method is not called since JDK 9, and that's why I set Affected Version/s to 8-pool. We don't intend to fix this in the current release.

Oh. Could you tell me where I should submit this PR? jdk8u?

[wangweij]

I'm not sure. You can probably ask on jdk8u-dev@openjdk.java.net.

[seanjmullan]

If you want to instead do some general cleanup in this class for JDK 20, you could instead remove this method and several other unused methods from JarVerifier.

[pandaapo]

If you want to instead do some general cleanup in this class for JDK 20, you could instead remove this method and several other unused methods from JarVerifier.

Before I began to make general cleanup, I looked up from this unused method JarVerifier#mapSignersToCodeSource()and found all related call stacks:

Can I also remove these methods from JarFile, JavaUtilJarAccess and its only implementation JavaUtilJarAccessImpl?

[seanjmullan]

If you want to instead do some general cleanup in this class for JDK 20, you could instead remove this method and several other unused methods from JarVerifier.

Before I began to make general cleanup, I looked up from this unused method JarVerifier#mapSignersToCodeSource()and found all related call stacks: Can I also remove these methods from JarFile, JavaUtilJarAccess and its only implementation JavaUtilJarAccessImpl?

Yes. I believe the setEagerValidation method can also be removed and all code that calls it.

[pandaapo]

Yes. I believe the setEagerValidation method can also be removed and all code that calls it.

OK, I will remove it and all methods that calls it:
.

Because JarVerifier#setEagerValidation will be removed, the field eagerValidation will always be false.

There are some codes using this field:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/jar/JarVerifier.java#L361-L381
Can I remove the eagerValidation condition from if(eagerValidation && sigFileSigners.get(name) != null)?

If I can, can I remove this field from JarVerifier then?

[seanjmullan]

Because JarVerifier#setEagerValidation will be removed, the field eagerValidation will always be false.

There are some codes using this field: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/jar/JarVerifier.java#L361-L381 Can I remove the eagerValidation condition from if(eagerValidation && sigFileSigners.get(name) != null)?

You can remove the whole if block since eagerValidation will never be true, and this block was only performed if it was true.

If I can, can I remove this field from JarVerifier then?

Yes.

[pandaapo]

Because JarVerifier#setEagerValidation will be removed, the field eagerValidation will always be false.
There are some codes using this field: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/jar/JarVerifier.java#L361-L381 Can I remove the eagerValidation condition from if(eagerValidation && sigFileSigners.get(name) != null)?

You can remove the whole if block since eagerValidation will never be true, and this block was only performed if it was true.

OK, then the first param JarFile jar of this method will be redundant, I will refactor it and related codes.

[wangweij]

Great to see more lines removed. Since entries2 in JavaUtilJarAccess is removed, I assume the entries2 in JarFile is also useless, and there are 2 newEntry methods who claims to be invoked by entries2. Also, in JarVerifier, it seems all lines after // Extended JavaUtilJarAccess CodeSource API Support before isTrustedManifestEntry are related to those removed methods.

[wangweij]

BTW, since this bug description is about a coding error but your fix is all cleanup, how about I file another bug and you use it in the title? I am trying to add a noreg-cleanup label to the bug and it does not apply to the jdk8u fix (suppose there will be one).

[pandaapo]

since this bug description is about a coding error but you fix is about cleanup, how about I file another bug and you use it in the title?

OK. Thanks.

I am trying to add a noreg-cleanup label to the bug and it does not apply to the jdk8u fix (suppose there will be one).

I had sent an email to jdk8u-dev@openjdk.java.net for asking which repository I can fork if I want to fix 8296734. And still waiting for reply.

[wangweij]

I just filed https://bugs.openjdk.org/browse/JDK-8297301.

[AlanBateman]

/label add core-libs

[openjdk]

@AlanBateman
The core-libs label was successfully added.

[wangweij]

codomain, VerifierCodeSource, isSigningRelated, and getUnsignedCS also used nowhere.

[pandaapo]

codomain, VerifierCodeSource, isSigningRelated, and getUnsignedCS also used nowhere.

Thanks. I think your codomain must mean csdomain.

[wangweij]

Looks almost complete. Just 2 small comments.

[wangweij]

Everything looks fine now. Thanks so much for the cleanup. It's always nice to see unused lines removed.

[openjdk]

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

8297301: Cleanup unused methods in JavaUtilJarAccess

Reviewed-by: weijun

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

• 392ac70: 8297211: Expensive fillInStackTrace operation in HttpURLConnection.getOutputStream0 when no content-length in response
• 5a45c25: 8297164: Update troff man pages and CheckManPageOptions.java
• f12710e: 8288047: Accelerate Poly1305 on x86_64 using AVX512 instructions
• cd6a203: 8297348: make CONF=xxx should match if xxx is an exact match
• 817e039: 8297352: configure should check pandoc version
• 15e2e28: 8297353: Regenerated checked-in html files with new pandoc
• b366d17: 8294073: Performance improvement for message digest implementations
• 57f5cfd: 8296399: crlNumExtVal might be null inside X509CRLSelector::match
• 0b04a99: 8297347: Problem list compiler/debug/TestStress*.java
• 0ac0148: 8297342: make LOG=debug is too verbose
• ... and 44 more: https://git.openjdk.org/jdk/compare/ab6b7ef98800bee5e87e39a616af642874eb154a...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@wangweij) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[pandaapo]

Everything looks fine now. Thanks so much for the cleanup. It's always nice to see unused lines removed.

Thank you for your review.

[pandaapo]

/integrate

[openjdk]

@pandaapo
Your change (at version 1aa65b2) is now ready to be sponsored by a Committer.

[wangweij]

/sponsor

[openjdk]

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

• 392ac70: 8297211: Expensive fillInStackTrace operation in HttpURLConnection.getOutputStream0 when no content-length in response
• 5a45c25: 8297164: Update troff man pages and CheckManPageOptions.java
• f12710e: 8288047: Accelerate Poly1305 on x86_64 using AVX512 instructions
• cd6a203: 8297348: make CONF=xxx should match if xxx is an exact match
• 817e039: 8297352: configure should check pandoc version
• 15e2e28: 8297353: Regenerated checked-in html files with new pandoc
• b366d17: 8294073: Performance improvement for message digest implementations
• 57f5cfd: 8296399: crlNumExtVal might be null inside X509CRLSelector::match
• 0b04a99: 8297347: Problem list compiler/debug/TestStress*.java
• 0ac0148: 8297342: make LOG=debug is too verbose
• ... and 44 more: https://git.openjdk.org/jdk/compare/ab6b7ef98800bee5e87e39a616af642874eb154a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@wangweij @pandaapo Pushed as commit f0e99c6.

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