319490 - js1_5/Regress/regress-104077 - return in finally should clear pending exception

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

The test fails in the browser but not shell.

Output:

In finally case of tryThis() function
In finally case of tryThis() function
In finally case of tryThis() function
In finally case of tryThis() function
In finally case of tryThis() function
In finally case of tryThis() function
Is the provided argument to myTest() null? : YES
Caught thrown exception = ZZZ
In finally block of addValues_3() function: sum = 7
In finally finally block of addValues_3() function: sum = 8
In finally block of addValues_4() function: sum = 7
In finally finally block of addValues_4() function: sum = 8
In finally block of addValues_5() function: sum = 7
In finally finally block of addValues_5() function: sum = 8
in finally block of testObj() function
function a120571() { while (0) { try { } catch (e) { continue; } } }
function b() { for (;;) { try { } catch (e) { continue; } } }
BUGNUMBER: 104077
STATUS: Just testing that we don't crash on with/finally/return -
FAILED!: uncaught exception: 43

The failing part of the test is:

function testObj(obj)
{
  var x = 42;

  try
  {
    with (obj)
    {
      if (obj.p)
        throw obj.p;
      x = obj.q;
    }
  }
  finally
  {
    writeLineToLog("in finally block of testObj() function");
    return 999;
  }
}

status = inSection(12);
obj = {p:43};
actual = testObj(obj);

The return in the finally clause should have cleared the thrown exception.

Comment 1

[Mike Shaver (:shaver emeritus)]

Attachment: patch

This seems to fix the issue, though I haven't run the browser-requiring test.  cx->throwing is cleared after returning from my simple test function, though, and it wasn't before.

function f() { try { throw 5; } finally { return 17; } }

Wafer-thin refactoring of js_InWithStatement with some macro sugar to minimize change size.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 206305 [details] [diff] [review]
patch

>+        /* If we're in a finally clause, then returning must clear any pending
>+         * exception.

Nit: major comment starts with /* on its own line, indented appropriately.

Might say more about this case.

>+         */
>+        if (js_InStatement(&cg->treeContext, STMT_SUBROUTINE) &&
>+            (js_NewSrcNote(cx, cg, SRC_HIDDEN) < 0 ||
>+             js_Emit1(cx, cg, JSOP_EXCEPTION) < 0 ||
>+             js_NewSrcNote(cx, cg, SRC_HIDDEN) < 0 ||
>+             js_Emit1(cx, cg, JSOP_POP) < 0)) {
>+            return JS_FALSE;
>+        }
>+

This will push the exception, then pop it, after the return value (an expression result or undefined) is pushed, so the stack has budget N+1 compared to before the patch, in the case where it had N depth before, and this return value reached that depth.  Better slightly to do this before the previous paragraph, instead of after.

Also slightly better in view of the possibility of an exception being thrown by the return value.  If due to a bug we had a false/null return but no pending exception set, a stale ("caught" by finally) exception might be replayed.

>         /*
>          * EmitNonLocalJumpFixup mutates op to JSOP_RETRVAL after emitting a
>          * JSOP_SETRVAL if there are open try blocks having finally clauses.

Hey, there's a nice major comment style example! :-P

/be

Comment 3

[Mike Shaver (:shaver emeritus)]

Attachment: how ya like me now?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 206312 [details] [diff] [review]
how ya like me now?

I ran around and proved to myself that simply clearing cx->throwing was sufficient (as we do in the JSOP_EXCEPTION case in jsinterp.c) and wasn't going to leave cx->exception dangling for use after the POP.

There is a comment in the JSOP_EXCEPTION case in jsopcode.c that might want some updating. r=mrbkap with that.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 206312 [details] [diff] [review]
how ya like me now?

Nice minimal patch.  Only thought is this: don't we *really* want to clear the pending exception at the start of a finally block that has no catch, only try?  Then we don't have ispending hazards on the way to return?

Also, what if there's no return in the finally and we just fall through to subsequent code?

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 206312 [details] [diff] [review]
how ya like me now?

Marking what I think brendan actually wanted.

Don't we want to throw if we reach the end of a finally block and there was no return?

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 206312 [details] [diff] [review]
how ya like me now?

I think I stomped on mrbkap's r+, but he didn't mean it ;-).

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 206312 [details] [diff] [review]
how ya like me now?

No, I didn't mean to + it. I was gently poking a hole in it :-).

/be

Comment 9

[Mike Shaver (:shaver emeritus)]

(In reply to comment #5)
> (From update of attachment 206312 [details] [diff] [review] [edit])
> Nice minimal patch.  Only thought is this: don't we *really* want to clear the
> pending exception at the start of a finally block that has no catch, only try? 
> Then we don't have ispending hazards on the way to return?

No, because we don't want to clear a pending exception unless we're certain to be returning, which replaces the Result(THROW) with Result(RETURN).

> Also, what if there's no return in the finally and we just fall through to
> subsequent code?

If there is an exception pending, then we don't fall through to subsequent code, we keep throwing out of the current try block or function.

js> try { throw 5; } finally { print("oh dear!"); }
oh dear!
uncaught exception: 5

I'll fix the JSOP_EXCEPTION comment and repost for posterity.  (Still tempting to just clear cx->throwing for JSOP_RETURN, though, or on non-error return from js_Invoke, so that JS_SetPendingException and then true return from a native doesn't hurt us the same way!)

Comment 10

[Brendan Eich [:brendan]]

Ok, so ECMA-262 Edition 3 really says try/finally (no catch) does not catch, and the exception propagates *unless* there is an explicit return in the finally (or another throw, but that's not relevant here and we handle that correctly).

In that case, then you're right that we could make return clear throwing in the interpreter.  That would be an even smaller patch.  What would be the downside, apart from performance?  Performance is a possible issue, but call/return are not optimized nearly enough today.  Comments?

/be

Comment 11

[Mike Shaver (:shaver emeritus)]

Attachment: best ever

Moving review forward optimistically, but feel free to nit me on the comment change if you want.

Shall I also produce a patch, so small that it is invisible to the naked eye, for clearing cx->throwing from JSOP_RETURN?

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> Created an attachment (id=206314) [edit]
> best ever
> 
> Moving review forward optimistically, but feel free to nit me on the comment
> change if you want.

No, looks great -- thanks for edumacating me.

> Shall I also produce a patch, so small that it is invisible to the naked eye,
> for clearing cx->throwing from JSOP_RETURN?

No, we don't want that.  You could JS_ASSERT(!cx->throwing) there, however.

/be

Comment 13

[Mike Shaver (:shaver emeritus)]

Attachment: and with an assertion, too!

I can do this all day!

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 206320 [details] [diff] [review]
and with an assertion, too!

>+++ jsinterp.c	19 Dec 2005 20:53:50 -0000
>@@ -2103,6 +2103,7 @@ interrupt:
>           END_CASE(JSOP_SETRVAL)
> 
>           BEGIN_CASE(JSOP_RETURN)
>+            JS_ASSERT(!cx->throwing);
>             CHECK_BRANCH(-1);
>             fp->rval = POP_OPND();
>             /* FALL THROUGH */

Don't you want this in JSOP_RETRVAL code too?  Hmm, and in JSOP_SETRVAL for that matter.

/be

Comment 15

[Mike Shaver (:shaver emeritus)]

Attachment: if one assertion is good, two must be better

As committed to trunk.

I moved the JSOP_RETURN assertion into JSOP_RETRVAL to take advantage of the fall-through.  If a branch callback sets cx->throwing and doesn't return false, they deserve the assertion that they get, IMO.

Comment 16

[Mike Shaver (:shaver emeritus)]

FIXED la la la la, la la, la la.

Comment 17

[Bob Clary [:bc] (inactive)]

verified windows/linux/mac