389605 - Assertion with empty-destructuring-let and try..catch

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> let ([] = function() { }) { try { throw 1; } catch(x) { } }
Assertion failure: OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE, at jsapi.c:2777


Variations trigger other assertions:

Assertion failure: OBJ_GET_CLASS(cx, obj) == &js_BlockClass, at jsinterp.c:5948

Assertion failure: (3 + 1) < (obj)->map->freeslot, at jsobj.c:1986


Doesn't crash opt.

Comment 1

[Jesse Ruderman]

Triggers a different assertion now:

js> let ([] = function() { }) { try { throw 1; } catch(x) { } }
Assertion failure: (size_t) (blocksp - fp->spbase) <= script->depth, at jsinterp.c:6515

Still doesn't crash opt.

Comment 2

[Igor Bukanov]

The reason for the bug is that let ([] = xxx) introduces a let block with 0 depth yet the stack unwind code relies on the stack depth to unwind the scope chain.

As such the interpreter, when transferring the control to the catch block would skip the let block scope and sets the scope chain to the call object for the function. Yet JSOP_LEAVESCOPE would still be executed and tries to unwind the already put object.

Comment 3

[Igor Bukanov]

One way to fix this is to disable empty destructuring patterns so the bug never happens. But this non-compatible and is no-go, right?

Another way is to add an artificial property like "" to such let blocks so OBJ_BLOCK_COUNT is always non-zero.

Comment 4

[Igor Bukanov]

Yet another possibility for a fix is to record the depth of scope/block chains into the try note and to unwind the scope precisely to that depth. It should make the unwinding much more robust. 

Comment 5

[Brendan Eich [:brendan]]

Yes, we need to allow empty destructuring patterns per ES4 as proposed. Recording the depth sounds best, robustness trumps minimizing space overhead.

/be

Comment 6

[Damon Sicore (:damons)]

Brendan, Igor, should this block 1.9?

Comment 7

[Brian Crowder]

This looks like a null-ptr dereference, so I would normally suggest not blocking.  I think this bug (and Igor's proposed fix, specifically) might represent a -class- of similar bugs, though, so it may be worth blocking on...

Comment 8

[Damon Sicore (:damons)]

-'ing based on Comment #7.

Comment 9

[Igor Bukanov]

Attachment: v1

This is an untested patch that just makes sure that the let block always adds at least one slot to the stack.

The alternative approach of recording scope chain depth in try notes requires much more code and would be more difficult to backport to 1.8.

Comment 10

[Igor Bukanov]

Attachment: v2

Testing v1 has shown that adding a synthetic property to the let block at the emit time is too late. Any property that contributes to the OBJ_BLOCK_COUNT must be added right after the code parsed the let variables to prevent pre-synthetic OBJ_BLOCK_COUNT entering calculations.

This is what that v2 does. I also removed changes to jsopcode.c as they are not as the decompiler ignores any synthetic let property not referenced by the bytecode.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 314151 [details] [diff] [review]
v2

>+            JS_ASSERT(vp > regs.sp);
>             JS_ASSERT(vp <= fp->spbase + script->depth);

The added assertion reads slightly better to me if reordered to regs.sp < vp -- number line order, and it matches:

>             while (regs.sp < vp) {
>                 STORE_OPND(0, JSVAL_VOID);
>                 regs.sp++;
>             }
[snip]
>+#if JS_HAS_DESTRUCTURING
>+/*
>+ * The catch/finally handler implementation in the interpreter assumes that
>+ * any operation that introduces a new scope (like a "let" or "with" block)
>+ * increases the stack depth. This way it is possible to restore the scope

Comma after "This way".

>+ * chain based on stack depth of the handler alone. A let block with an empty
>+ * destructuring pattern like in

s/like in/such as/

>+ *
>+ *   let [] = 1;
>+ *
>+ * would violate it as the there would be no let locals to store on the stack.

s/it /this assumption, /

>+ * To workaround it we add an empty property to such blocks so

s/workaround it/satisfy it/

>+ * OBJ_BLOCK_COUNT(cx, blockObj), that gives the number of slots, would be
>+ * always positive.
>+ */
>+static JSBool
>+MakeNonEmptyLet(JSContext *cx, JSTreeContext *tc)

s/Make/Ensure/g

because of this test at the top of the function:

>+{
>+    if (0 != OBJ_BLOCK_COUNT(cx, tc->blockChain))

Nit: generally we avoid constant on the left for != and ==.

>+        return JS_TRUE;
>+
>+    return js_DefineNativeProperty(
>+               cx, tc->blockChain,
>+               ATOM_TO_JSID(cx->runtime->atomState.emptyAtom),
>+               JSVAL_VOID, NULL, NULL, JSPROP_PERMANENT | JSPROP_READONLY,
>+               SPROP_HAS_SHORTID, 0, NULL);

Neat trick. Could it be indented more conventionally?

    return js_DefineNativeProperty(cx, tc->blockChain,
                                   ATOM_TO_JSID(cx->runtime->atomState
                                                .emptyAtom),
                                   JSVAL_VOID, NULL, NULL,
                                   JSPROP_PERMANENT | JSPROP_READONLY,
                                   SPROP_HAS_SHORTID, 0, NULL);

Thanks for fixing this. The first time 'round, I had to find a different way of handling the empty destructuring special case, after missing it when implementing for js1.7 from a draft es4 proposal.

/be

Comment 12

[Igor Bukanov]

Attachment: v3

The previous version of the patch adds an empty synthetic property when putting the let blocks. v3 fixes this and addresses the nits. Here is a delta from v2:

--- /home/igor/m/ff/mozilla/quilt.LBI6Dv/js/src/jsinterp.c	2008-04-08 16:54:37.000000000 +0200
+++ js/src/jsinterp.c	2008-04-08 09:33:54.000000000 +0200
@@ -6485,3 +6485,3 @@ interrupt:
             vp = regs.sp + OBJ_BLOCK_COUNT(cx, obj);
-            JS_ASSERT(vp > regs.sp);
+            JS_ASSERT(regs.sp < vp);
             JS_ASSERT(vp <= fp->spbase + script->depth);
--- /home/igor/m/ff/mozilla/quilt.LBI6Dv/js/src/jsparse.c	2008-04-08 16:54:37.000000000 +0200
+++ js/src/jsparse.c	2008-04-08 09:42:42.000000000 +0200
@@ -1664,3 +1664,3 @@ BindLet(JSContext *cx, BindData *data, J
  * any operation that introduces a new scope (like a "let" or "with" block)
- * increases the stack depth. This way it is possible to restore the scope
+ * increases the stack depth. This way, it is possible to restore the scope
  * chain based on stack depth of the handler alone. A let block with an empty
@@ -1670,4 +1670,4 @@ BindLet(JSContext *cx, BindData *data, J
  *
- * would violate it as the there would be no let locals to store on the stack.
- * To workaround it we add an empty property to such blocks so
+ * would violate this assumption as the there would be no let locals to store
+ * on the stack. To satisfy it we add an empty property to such blocks so
  * OBJ_BLOCK_COUNT(cx, blockObj), that gives the number of slots, would be
@@ -1676,4 +1676,6 @@ BindLet(JSContext *cx, BindData *data, J
 static JSBool
-MakeNonEmptyLet(JSContext *cx, JSTreeContext *tc)
+EnsureNonEmptyLet(JSContext *cx, JSTreeContext *tc)
 {
+    jsid id;
+    
     if (0 != OBJ_BLOCK_COUNT(cx, tc->blockChain))
@@ -1681,7 +1683,7 @@ MakeNonEmptyLet(JSContext *cx, JSTreeCon
 
-    return js_DefineNativeProperty(
-               cx, tc->blockChain,
-               ATOM_TO_JSID(cx->runtime->atomState.emptyAtom),
-               JSVAL_VOID, NULL, NULL, JSPROP_PERMANENT | JSPROP_READONLY,
-               SPROP_HAS_SHORTID, 0, NULL);
+    id = ATOM_TO_JSID(cx->runtime->atomState.emptyAtom);
+    return js_DefineNativeProperty(cx, tc->blockChain, id,
+                                   JSVAL_VOID, NULL, NULL,
+                                   JSPROP_PERMANENT | JSPROP_READONLY,
+                                   SPROP_HAS_SHORTID, 0, NULL);
 }
@@ -3002,3 +3004,3 @@ Statement(JSContext *cx, JSTokenStream *
                     pn3 = DestructuringExpr(cx, &data, tc, tt);
-                    if (!pn3 || !MakeNonEmptyLet(cx, tc))
+                    if (!pn3 || !EnsureNonEmptyLet(cx, tc))
                         return NULL;
@@ -3620,3 +3622,3 @@ Variables(JSContext *cx, JSTokenStream *
 #if JS_HAS_DESTRUCTURING
-    if (let && !MakeNonEmptyLet(cx, tc))
+    if (let && !EnsureNonEmptyLet(cx, tc))
         return NULL;
--- /home/igor/m/ff/mozilla/quilt.LBI6Dv/js/src/jsobj.c	2008-04-03 00:26:14.000000000 +0200
+++ js/src/jsobj.c	2008-04-08 09:42:32.000000000 +0200
@@ -1976,2 +1976,8 @@ js_PutBlockObject(JSContext *cx, JSBool 
                 continue;
+            if (sprop->id == ATOM_TO_JSID(cx->runtime->atomState.emptyAtom)) {
+                /* See comments before EnsureNonEmptyLet from jsparse.c. */
+                JS_ASSERT(OBJ_BLOCK_COUNT(cx, obj) == 1);
+                JS_ASSERT(sprop->shortid == 0);
+                continue;
+            }
             slot = depth + (uintN) sprop->shortid;

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 314340 [details] [diff] [review]
v3

>+    if (0 != OBJ_BLOCK_COUNT(cx, tc->blockChain))

The 0 != expr form still goes against prevailing style and makes me wince (memories of Windows(tm) programmers phearing assignment operator by typo, but in this case the right operand is not an lvalue).

r=me again, esp. if you can stand to pick this nit.

We should take this for 1.9 to avoid regressing from JS1.7 / Firefox 2.

/be

Comment 14

[Igor Bukanov]

Attachment: v3b

The new version of the patch addresses the last nit:

@@ -1680,3 +1680,3 @@ EnsureNonEmptyLet(JSContext *cx, JSTreeC
 
-    if (0 != OBJ_BLOCK_COUNT(cx, tc->blockChain))
+    if (OBJ_BLOCK_COUNT(cx, tc->blockChain) != 0)
         return JS_TRUE;

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314702 [details] [diff] [review]
v3b

a1.9=beltzner

Comment 16

[Igor Bukanov]

I checked in the patch from the comment 14 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1207842567&maxdate=1207842660&who=igor%25mir2.org

Comment 17

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/regress/regress-389605.js

Comment 18

[Bob Clary [:bc] (inactive)]

v 1.9.0

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/regress/regress-389605.js

ignore syntax error on trunk due to bug 418051 fix.

Comment 20

[Bob Clary [:bc] (inactive)]

when this bug is opened, the test should be checked in.