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

[Attila Szegedi]

On Mar 9, 2015, at 2:22 PM, Tal Liron <tal.liron at threecrickets.com> wrote:

> Could you possibly articulate 1) *exactly* what the issue is, 

Well, your case is peculiar. My suspicion was originally that the issue only occurred in situations where transfer of control happens to outside the try/finally (e.g. continue/break jumps to outside of try/finally) *and* you had a catch block *and* the finally block also threw an exception. In that case, the catch block that was a sibling to the finally block could have caught the exception erroneously. I'll set up a dev environment with the code before my try/finally compilation rewrite and get back to you when I understand why was this one compiled erroneously.

Mind you, I didn't just do a point fix of that one problem but re-thought how Nashorn compiles try/finally, so I can confirm that my rewrite fixed your issue too.

> 2) how to search for potential issues in my vast amount of JavaScript code, 3) and how to work around it?

I just had an in-the-shower idea. I can take 8u40 code, add some light logging to it (basically add a log for "inlined finally block from line x to line y"; in your example program it'd say "inlined finally block from line 11 to line 5" (that's what the bug is about – having to inline finally blocks at nonlocal transfer of control points to outside the try block; the bug is that in your case the target of the continue is within the try block, so no inlining should happen). I can add the same logging code to 8u60 codebase. Then I could give you an utility that compiles your source code with both versions, and prints the log diff. As I trust that 8u60 finally-inlining logic is sound, any difference between the two will point out a location in your source code that pre-8u60 was incorrectly inlining.

As for workaround; I don't know; I need to first understand why it happens (see my answer to your point 1).

> The problem is that I'm not self-deploying: I am building products that run on top of the JVM with JavaScript, and yet I cannot recommend Nashorn to anyone in its current state. At the very least, I can make sure that my own JavaScript code works with 8u40.

> Actually, the "meta" bug here is that Nashorn's release system is broken: these kinds of breaking bugs need an immediate notice and a fix released to the public. And yet Nashorn is tied to JDK releases, which are once every six months. Something is very wrong about this system, and I'm sure it must be frustrating to the Nashorn team, too.

Yes, being tied to the release cycle of the JDK can obviously be frustrating in situations like these. I'm very concerned about the pain that this is causing users. FWIW, we also have CPU (Critical Patch Update) releases. Next one (8u45) is coming up in mid-April. I can't promise a fix will get into a CPU release as the decision to include something into a CPU is not mine; I can only nominate a fix it for it.

> Barring the possibility of a bugfix release, there *must* to be a "known issues" right there on the JDK download page. You simply cannot release a JavaScript engine to the public that does not honor try/finally.

I entirely agree with you. I was quite horrified when I realized Nashorn has this problem. (As I said, it's an ancient (in Nashorn time) compiler logic bug, I can track it to before the initial release.) I spent good two weeks understanding the problem and rewriting the relevant compiler pipeline parts to make sure try/finally is correctly compiled under all circumstances. I wish I could've completed it in time for 8u40.

The reason I didn't think it warrants mention (unlike that vararg array issue I wrote an e-mail about upon 8u40 release) was that "it has always been like this", and we sincerely believed it only affects the very specific case I explained above (transfer of control to outside of try block + catch block + finally block itself throws) that felt extremely rare, and hey, nobody complained about it for more than a year. Your case is something else though.

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.

> 
> On 03/09/2015 02:48 AM, Attila Szegedi wrote:
>> This isn't new to 8u40; I just tested, and it's broken in 8u31 and 8u25 too. If I had to guess, it has been like this since the initial release. I base my guess on the fact that the way Nashorn compiled finally blocks in presence of control flow transfers (break/continue/return) was unfortunately broken, I myself discovered this last December and fixed it early February – that's indeed the JIRA issue that Anthony pointed out. Unfortunately, it didn't make it in time for 8u40 :-(
>