Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-67011] bridge-method-injector creates questionable bytecode #34

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

basil
Copy link
Member

@basil basil commented Nov 17, 2022

Problem

JENKINS-67011 reported two problems with bridge-method-injector:

  • bridge-method-injector unnecessarily modified the class header.
  • As part of these unnecessary modifications, bridge-method-injector inserted a duplicate class entry for hudson/model/Node.

Evaluation

I could reproduce the former but not the latter running on Java 11 with latest bits. Looking into why the class header is being unnecessarily modified, it is because of commit 9cbb78f, itself a fix for JENKINS-22525. Ironically, that commit was a workaround for a problem to eliminate duplicate class entries in the constant pool. So the workaround is now creating the very problem it was meant to eliminate. Moreover, this workaround is unnecessary for two reasons:

  • It was working around JDK-5053846 which was fixed in Java 8 a long time ago.
  • JDK-5053846 (fixed in Java 8) only affected Eclipse OpenJ9, which is not a supported runtime.

Solution

Revert commit 9cbb78f. Not only is that commit not needed, but it exposes us to JENKINS-67011.

Testing done

I ran mvn clean verify -Dtest=jenkins.bugs.BridgeMethodsTest on Jenkins core with these changes, successfully.

I compared the generated bytecode before and after this change. Before this change the entire class file was rewritten by ASM, including the class header. After this change the class remains identical to what javac produced with only the additions made by bridge-method-injector, the original class header untouched. Thus if the original output of javac does not contain duplicate constant pool entries (and it indeed does not as of Java 8 and the fix for JDK-5053846) then neither does the output of bridge-method-injector. Therefore JENKINS-67011 bug is fixed.

@basil basil added the bug label Nov 17, 2022
@basil basil requested a review from jglick November 17, 2022 21:57
@basil basil merged commit 95cee5b into jenkinsci:master Nov 17, 2022
@basil basil deleted the JENKINS-67011 branch November 18, 2022 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants