Serious bug: "finally" block is run whenever "continue" is reached

[Attila Szegedi]

I have understood the extent of the issue, and it is not pretty. Basically, it affects every code shape with a continue or break in a try/finally. I put up a small test here: https://gist.github.com/szegedi/22d634d70d422445be9a It passes with 9 and 8u60 builds, but fails with all released JDK 8 versions up to and including 8u40. I was curious how could this go on for this long undetected. I tallied all of the tests that we run on every build that have the words "finally" and either "break" or "continue" in them, including jquery, sunspider, octane, and test262.ecmascript.org tests. We have 9 own tests, test262 suite has 17, and the rest of the tests (octane etc. combined) have 20 files where these occur. Turns out, even the ~11.500 test262 test suite doesn't test this exact scenario :-( There's some tests with a loop in a try/finally, but they only test exiting those loops with a throw, and don't consider break/continue where the control flow stays within the try finally. Come to think of it, that's actually not even a special case, so I can sort-of understand why it wouldn't be tested separately. I'll definitely be adding that gist as a test to Nashorn.

In light of this, I'm not sure if I need to write a tool for finding occurrences, as the rule is quite simple (unfortunately): every break/continue inside a try/finally is affected. More precisely: every break/continue lexically found in a try/finally within the same function body is affected.

This also gives us a workaround: encapsulating the code surrounding an affected break/continue into a function expression, e.g. in your original example:

var finalies = 1
var line = 'TEST THIS LINE'
try {
    (function(){ // <---
    while ('' !== (line = line.substring(0, line.length - 1))) {
        if (line.length % 2 == 0) {
            continue
        }
        java.lang.System.out.println(line)
    }
    })(); // <---
} finally {
    java.lang.System.out.println('Finalies ' + (finalies++))
}

Of course, it doesn't have to be an immediately-invoked-anonymous-function-expression; you can structure it as a named function you invoke too. It ain't too pretty, but workarounds seldom are. You can also play with passing parameters for better local access performance in the new function body, e.g.

    (function(line){
    ...
    })(line);

Hope this helps until we ship the real fix.
  Attila.

On Mar 10, 2015, at 1:34 AM, Tal Liron <tal.liron at threecrickets.com> wrote:

> Thank you, Atilla, your response does inspire confidence.
> 
> I will just add that it's not just me that would need this tool: none of us can afford to have a broken try/finally.
> 
> On 03/09/2015 05:58 PM, Attila Szegedi wrote:
>> So my action plan for this is: 1. nominate the fix for inclusion into a CPU release 2. whip up a small one-off tool that'd show you the differences in finally inlining between 8u40 and 8u60 code. That should help you find the trouble spots in your code. 3. understand what caused the faulty inlining in your case that will hopefully let me suggest a workaround for the time being. Attila.
>