273931 - JS exception handling doesn't pop the scope chain when it should

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brendan Eich [:brendan]]

URL says it all, it should get a ReferenceError: foo is not defined in the catch
block's alert call argument evaluation.  Instead it alerts "bar".

/be

Comment 1

[Brendan Eich [:brendan]]

Consider similar cases:

javascript:try { with ({foo:"bar"}) eval("throw 42"); } catch (e) { alert(foo) }

or eval(s) where s = "throw 42", possibly in some other file at runtime, so no
compiler can see what's going on.  We can't pop the scope chain at the throw
site, and (since there could be several), it would be better to pop all With
objects from the scope chain that were pushed within the try, when we do the
JSOP_SETSP at the beginning of the corresponding catch.

IOW, we need to extend JSOP_SETSP's runtime code.

Short of doing away with With objects and coming up with a different runtime
organization for the scope chain than __parent__ linkage, the right fix seems to
be one that helps JSOP_SETSP notice when OBJ_GET_CLASS(cx, fp->scopeChain) is
&js_WithClass, so it can pop all the With objects that were stacked within the
try code.

That means we need to correlate stacked With references with linked With
objects.  Rather than searching the stack being popped by JSOP_SETSP for With
object refs (not foolproof given the With ctor, and anyway prone to O(n^2)
growth), we could give each With object private data: an INT-tagged jsval
pointer into the stack where the With object reference was pushed by JSOP_ENTERWITH.

Then JSOP_SETSP's special case would just pop as many With objects from the
scope chain as had private data that (when untagged) was at or above the new sp
being set.

/be

Comment 2

[Mike Shaver (:shaver emeritus)]

Attachment: Like this?

I love it when it's actually as simple as it sounds.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 168635 [details] [diff] [review]
Like this?

>+++ jsinterp.c	13 Dec 2004 17:12:42 -0000
>@@ -1997,6 +1997,8 @@ js_Interpret(JSContext *cx, jsval *resul
>             withobj = js_NewObject(cx, &js_WithClass, obj, fp->scopeChain);
>             if (!withobj)
>                 goto out;

Oop, can you put a little last-ditch fix here by doing SAVE_SP(fp); immediately
before the js_NewObject calling line?  Thanks.

>+            OBJ_SET_SLOT(cx, withobj, JSSLOT_PRIVATE,
>+                         INT_TO_JSVAL(sp - fp->spbase));

How about using i just for symmetry:

	    i = INT_TO_JSVAL(sp - fp->spbase);
	    OBJ_SET_SLOT(cx, withobj, JSSLOT_PRIVATE, i);

>@@ -4864,6 +4866,17 @@ js_Interpret(JSContext *cx, jsval *resul
>             i = (jsint) GET_ATOM_INDEX(pc);
>             JS_ASSERT(i >= 0);
>             sp = fp->spbase + i;

One empty line here please.

>+            /*
>+             * Pop any with objects pushed onto the scope chain after the
>+             * scope-depth checkpoint saved in this SETSP.
>+             */
>+            while (OBJ_GET_CLASS(cx, fp->scopeChain) == &js_WithClass &&
>+                   JSVAL_TO_INT(OBJ_GET_SLOT(cx, fp->scopeChain,
>+                                             JSSLOT_PRIVATE)) > i) {

Could use obj = fp->scopeChain; first for shorter lines, no wrapped second
clause of &&, less repitition (compilers can do the c.s.e., that's not an issue
these days, AFAIK), etc.

>+                rval = OBJ_GET_SLOT(cx, fp->scopeChain, JSSLOT_PARENT);
>+                JS_ASSERT(JSVAL_IS_OBJECT(rval));
>+                fp->scopeChain = JSVAL_TO_OBJECT(rval);

This should just be

		fp->scopeChain = OBJ_GET_PARENT(cx, obj);

given the obj suggestion above.  OBJ_GET_PARENT is the right tool here, not
OBJ_GET_SLOT, to preserve the layering and allow parent to be fused with other
predefined slots and their JSObject, some day soon.

r=me with the above.

/be

Comment 4

[Brendan Eich [:brendan]]

Er, you know what I mean:

		obj = OBJ_GET_PARENT(cx, obj);

IOW, set obj = fp->scopeChain; before the while loop, and set fp->scopeChain =
obj after.  Oh, and you need to null-test obj in a first clause in the while
loop's condition, or else.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 168635 [details] [diff] [review]
Like this?

shaver's gonna slap a new patch in here today.

/be

Comment 6

[Mike Shaver (:shaver emeritus)]

Attachment: With review comments addressed.

Today, schmoday.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 174598 [details] [diff] [review]
With review comments addressed.

>+            i = INT_TO_JSVAL(sp - fp->spbase);
>+            OBJ_SET_SLOT(cx, withobj, JSSLOT_PRIVATE, i);

i is not of type jsval -- use rval, ok?

>+            obj = fp->scopeChain;
>+            while (OBJ_GET_CLASS(cx, obj) == &js_WithClass &&
>+                   JSVAL_TO_INT(OBJ_GET_SLOT(cx, obj, JSSLOT_PRIVATE)) > i) {
>+                obj = fp->scopeChain = OBJ_GET_PARENT(cx, obj);
>+            }

Set fp->scopeChain = obj after the loop, not in it.

r=me with those changes.

/be

Comment 8

[Bob Clary [:bc] (inactive)]

this was checked in. marking fixed. I'll have a testcase later.

Comment 9

[Bob Clary [:bc] (inactive)]

js/tests/js1_5/Exceptions/regress-273931.js checked in.

Comment 10

[Bob Clary [:bc] (inactive)]

verified fixed.