java.lang.VerifyError in language/statements/try of test262

[tuchida]

When I enable language/statements/try in test262, I get the following error.

org.mozilla.javascript.tests.Test262SuiteTest > test262Case[js=language/statements/try/S12.14_A7_T1.js, opt=0, strict=false] FAILED
    java.lang.VerifyError at Test262SuiteTest.java:217

...

org.mozilla.javascript.tests.Test262SuiteTest > test262Case[js=language/statements/try/S12.14_A7_T3.js, opt=0, strict=false] FAILED
    java.lang.VerifyError at Test262SuiteTest.java:217

With JavaScript code like this

try{
  try{
    throw "ex1";
  }finally{
    try{
      throw "ex2";
    }catch(er1){
    }
  }
}catch(er2){
}

I got an error like the following

Exception in thread "main" java.lang.VerifyError: Stack map does not match the one at exception handler 91
Exception Details:
  Location:
    org/mozilla/javascript/gen/_stdin__1._c_script_0(Lorg/mozilla/javascript/gen/_stdin__1;Lorg/mozilla/javascript/Context;Lorg/mozilla/javascript/Scriptable;Lorg/mozilla/javascript/Scriptable;[Ljava/lang/Object;)Ljava/lang/Object; @91: nop
  Reason:
    Type top (current frame, locals[5]) is not assignable to 'java/lang/Object' (stack map, locals[5])
  Current Frame:
    bci: @52
    flags: { }
    locals: { 'org/mozilla/javascript/gen/_stdin__1', 'org/mozilla/javascript/Context', 'org/mozilla/javascript/Scriptable', 'org/mozilla/javascript/Scriptable', '[Ljava/lang/Object;' }
    stack: { 'org/mozilla/javascript/JavaScriptException' }
  Stackmap Frame:
    bci: @91
    flags: { }
    locals: { 'org/mozilla/javascript/gen/_stdin__1', 'org/mozilla/javascript/Context', 'org/mozilla/javascript/Scriptable', 'org/mozilla/javascript/Scriptable', '[Ljava/lang/Object;', 'java/lang/Object', top, 'org/mozilla/javascript/Scriptable' }
    stack: { 'java/lang/Throwable' }

https://app.circleci.com/pipelines/github/mozilla/rhino/394/workflows/06f40ab3-873d-4ffd-b26f-753722081c8a/jobs/400/tests

I investigated the cause of this, but could not find it. If there is any way to find out, please let me know. For example, is there any way to isolate whether the StackMapTable is wrong or the bytecode itself is wrong?

[tuchida]

This is an excerpt of a dump of a class file output by ClassFileWriter and analyzed by javap.
It seems that inefficient bytecodes are being output due to the large number of nop.

        33: athrow
        34: nop
        35: nop
        36: nop
        37: nop
        38: nop
        39: nop
        40: nop
        41: nop
        42: nop
        43: nop
        44: nop
        45: nop
        46: nop
        47: nop
        48: nop
        49: nop
        50: nop
        51: athrow
        52: nop
        53: nop
        54: athrow
        55: nop
        56: nop
        57: nop
        58: nop
        59: nop
        60: nop
        61: nop
        62: nop
        63: nop
        64: nop
        65: nop
        66: nop
        67: nop
        68: nop
        69: nop
        70: nop
        71: nop
        72: nop
        73: nop
        74: nop
        75: nop
        76: nop
        77: nop
        78: nop
        79: nop
        80: nop
        81: nop
        82: athrow
        83: nop
        84: nop
        85: nop
        86: nop
        87: athrow
        88: nop
        89: nop
        90: athrow
        91: nop
        92: nop
        93: nop
        94: nop
        95: nop
        96: nop
        97: nop
        98: athrow
        99: nop
       100: nop

[p-bakker]

No expert here, but:

• java.lang.VerifyError in bytecode produced by CodeGen #407 might be a potential duplicate of this case
• maybe an idea to take the compiled bytecode and (try to) decompile it back to Java? A similar approach was taken in Slow performance with function callbacks? #376 (comment)

[tuchida]

Thanks!
This is the output of javap. There are a lot of nop in _c_script_0, which looks inefficient.
https://gist.github.com/tuchida/7dda6b1ea8476e24c4fddc7b8e3229b1

[p-bakker]

What if you take that try.class file and try to decompile it, for example using http://www.javadecompilers.com/?

Maybe if it decompiles, some sense can be made of the resulting Java code

[tuchida]

http://www.javadecompilers.com/

I tried all the decompilers, but none of them output the correct _c_script_0.
Perhaps the class file itself is corrupted.

[p-bakker]

As I said, no expert here on bytecode stuff, but that would be my guess as well, that the class is corrupt due to bad bytecode generation.

One thing to try is to create a try2.class from a similar snippet of JavaScript, but one that doesn't display this VerifyError and see if that one does decompile. If that is the case, I think its fairly safe to say that the bytecode generation for JS snippet above is wrong.

[gbrail]

These kinds of bugs are painful to find. My guess would be that the noops are a symptom and not the cause of the problem, but it's going to take some work to unravel this. I'll see if I have any ideas, but I think it's important that we fix this and I hope that some others have ideas too.

…

On Tue, Jun 29, 2021 at 5:45 AM Paul Bakker ***@***.***> wrote: As I said, no expert here on bytecode stuff, but that would be my guess as well, that the class is corrupt due to bad bytecode generation. One thing to try is to create a try2.class from a similar snippet of JavaScript, but one that doesn't display this VerifyError and see if that one does decompile. If that is the case, I think its fairly safe to say that the bytecode generation for JS snippet above is wrong. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#915 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I274XFIL4XPEABMTVELTVG55XANCNFSM45YEDHEA> .

[gbrail]

"git bisect" tells me unhelpfully that this bug started happening when we switched the target of the bytecode generation from 1.6 to 1.8, which happened in 2017 with commit f3fff47. (Literally all it does is change the version of the generated bytecode.) That would result in a change in the header that's in the generated bytecode that specifies the version and that would be it. I imagine maybe the JVM tightened up the verification rules at that point. Unfortunately that doesn't really help.

…

On Tue, Jun 29, 2021 at 8:45 AM Gregory Brail ***@***.***> wrote: These kinds of bugs are painful to find. My guess would be that the noops are a symptom and not the cause of the problem, but it's going to take some work to unravel this. I'll see if I have any ideas, but I think it's important that we fix this and I hope that some others have ideas too. On Tue, Jun 29, 2021 at 5:45 AM Paul Bakker ***@***.***> wrote: > As I said, no expert here on bytecode stuff, but that would be my guess > as well, that the class is corrupt due to bad bytecode generation. > > One thing to try is to create a try2.class from a similar snippet of > JavaScript, but one that doesn't display this VerifyError and see if that > one does decompile. If that is the case, I think its fairly safe to say > that the bytecode generation for JS snippet above is wrong. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#915 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAD7I274XFIL4XPEABMTVELTVG55XANCNFSM45YEDHEA> > . >

[p-bakker]

No idea if at all related, but found the following in https://www.oracle.com/java/technologies/javase/8-compatibility-guide.html:

Verification of the invokespecial instruction has been tightened when the instruction refers to an instance initialization method ("").

And in https://www.oracle.com/java/technologies/compatibility.html there's talk about stackmaps and verification

Hope this helps

[gbrail]

I think it's just that before Java 7, stack maps weren't required, and after that they were. I think that Rhino may have always been generating an invalid stack map for this case. It's going to take some thinking to figure it out though.

…

On Tue, Jun 29, 2021 at 10:12 AM Paul Bakker ***@***.***> wrote: No idea if at all related, but found the following in https://www.oracle.com/java/technologies/javase/8-compatibility-guide.html : Verification of the invokespecial instruction has been tightened when the instruction refers to an instance initialization method (""). And in https://www.oracle.com/java/technologies/compatibility.html there's talk about stackmaps and verification Hope this helps — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#915 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7I2YW65GKUOG67ZF5ADTTVH5JPANCNFSM45YEDHEA> .

[carlosame]

Maybe it's that ClassFileWriter.finalizeSuperBlockStarts() is not being called when it needs to be, or is not doing something that it should. Finding a short reproducer (short enough to hardcode it in ClassFileWriterTest) may not be easy.

As if this stuff wasn't already a mess, turns out that the ClassFileWriterTest class belongs to the org.mozilla.javascript.classfile.tests package but is in the testsrc/org/mozilla/classfile/tests directory. Cannot run ClassFileWriterTest from Eclipse due to that, I wonder if there is a reason why the file is at the wrong directory (or has the wrong package name). If there is a real reason, a comment there would help.

[gbrail]

I'd like to move towards an RC for 1.7.14. This bug has been in there since 2017, and so far no one has picked it up or had an idea how to fix it. I think that there are enough benefits to doing a release to move this bug out of the 1.7.14 release.

[tuchida]

@carlosame ClassFileWriterTest was added by me in #831.
I'm using ./gradlew test, but if you have problems with Eclipse, you are welcome to fix it.

[p-bakker]

I've moved this case to the v1.7.15 milestone, so it no longer blocks the v1.7.14 rc/release

(can always move it back if a PR solving the issue comes around very quickly that doesn't look too risky to include for v1.7.14 :-) )

[carlosame]

Thank you @tuchida for your comment. I found that the issue was fixed by fef5eaa hours before me posting the comment, so no problem now.

[carlosame]

I generated code to reproduce @tuchida's try.class from ASM methods with:

$ java -cp "asm-9.2.jar;asm-util-9.2.jar;buildGradle/libs/rhino-1.7.14-SNAPSHOT.jar" org.objectweb.asm.util.ASMifier try.class>try_js_1Dump.java

then executed this:

byte[] dump = try_js_1Dump.dump();
FileOutputStream oStream = new FileOutputStream("try.class.asm");
oStream.write(dump);
oStream.close();

I "textified" that dump (with ASM's Textifier) and it results in the same output as textifying the original try.class:

$ java -cp "asm-9.2.jar;asm-util-9.2.jar;buildGradle/libs/rhino-1.7.14-SNAPSHOT.jar" org.objectweb.asm.util.Textifier try.class>try.txt

Then I edited the try_js_1Dump source to produce stackmap frames with:

ClassWriter classWriter = new ClassWriter(ClassWriter.COMPUTE_FRAMES);

and repeated the dump. The Textifier produced then a different output (the console diff is of 210 lines): the stackmap frames are surprisingly different (it is easier to glimpse when compared using KDiff3 or similar). I can upload the text files (and/or the diff) if anyone is interested.

No fix for the issue so far, but I thought that sharing these results could be interesting.