351606 - nested for..in and throw causes "Assertion failure: OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE" [@ JS_SetPrivate]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> for(let d in [1,2,3,4]) try { for(let a in [5,6,7,8]) ((function(){throw 9;})()); } catch(c) {  }
Assertion failure: OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE, at jsapi.c:2368

#0  0x0000960c in JS_Assert
#1  0x0001a094 in JS_SetPrivate (cx=0x500180, obj=0x1804ec0, data=0x0) at jsapi.c:2368
#2  0x000405a8 in js_PutBlockObject (cx=0x500180, obj=0x1804ec0) at jsobj.c:1941
#3  0x000acacc in js_Interpret (cx=0x500180, pc=0x50367a "?", result=0xbfffe6f0) at jsinterp.c:5988
#4  0x0008e940 in js_Execute (cx=0x500180, chain=0x1804ec0, script=0x5035d0, down=0x0, flags=0, result=0xbfffe820) at jsinterp.c:1622
#5  0x00020838 in JS_ExecuteScript (cx=0x500180, obj=0x1804ec0, script=0x5035d0, rval=0xbfffe820) at jsapi.c:4256
#6  0x00002664 in Process (cx=0x500180, obj=0x1804ec0, filename=0x0, forceTTY=0) at js.c:265
#7  0x0000322c in ProcessArgs (cx=0x500180, obj=0x1804ec0, argv=0xbffff9fc, argc=0) at js.c:486
#8  0x00009578 in main (argc=0, argv=0xbffff9fc, envp=0xbffffa00) at js.c:3081

Comment 1

[Brendan Eich [:brendan]]

This WFM in trunk and 1.8 branch js shells.  No local patches.  How did you build, and with what source?  Try clobbering and rebuilding, if in doubt (no make deps in standalone build).

/be

Comment 2

[Jesse Ruderman]

I updated js/src, clobbered js/src/Darwin_DBG.OBJ, then rebuilt with "make -f Makefile.ref" in js/src.  I still see the bug.

I'm testing on a PowerBook G4 (Mac PPC) with a Firefox trunk tree.  I don't have any local patches to js/src except for one to make an assertion message shorter, and another to make assertions trigger the Mac OS X crash reporter dialog when the JavaScript engine is used in Firefox.

Comment 3

[Brendan Eich [:brendan]]

Argh, I keep wrapping these top-level scripts in function f(){ and } to make decompilation and disassembly easier.  That avoids the bug, so this must be a problem with translating let to var at top level.  mrbkap, can you help?

/be

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #3)
> Argh, I keep wrapping these top-level scripts in function f(){ and } to make
> decompilation and disassembly easier.  That avoids the bug, so this must be a
> problem with translating let to var at top level.  mrbkap, can you help?

I can reproduce this, both in function and at top-level. This can't be the let-to-var conversion since that takes place in the statement TOK_LET case, whereas for loops parse their own let declarations. I'm looking into this.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

We were never setting the parent of the cloned child because its grandparent was null. I believe this patch was the original intent.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 237058 [details] [diff] [review]
Fix

>         cursor = js_CloneBlockObject(cx, cursor, fp->scopeChain, fp);
>         if (!cursor) {
>             if (clonedChild)
>                 JS_POP_TEMP_ROOT(cx, &tvr);
>             return NULL;
>         }
>-        if (!parent) {
>-            if (!clonedChild)
>-                obj = cursor;
>-            else
>-                JS_POP_TEMP_ROOT(cx, &tvr);
>-            break;
>-        }
>         if (!clonedChild) {
>             obj = cursor;
>             JS_PUSH_SINGLE_TEMP_ROOT(cx, cursor, &tvr);
>         } else {
>             /*
>              * Avoid OBJ_SET_PARENT overhead as clonedChild cannot escape to
>              * other threads.
>              */
>             clonedChild->slots[JSSLOT_PARENT] = OBJECT_TO_JSVAL(cursor);
>             JS_ASSERT(tvr.u.value == OBJECT_TO_JSVAL(clonedChild));
>             tvr.u.value = OBJECT_TO_JSVAL(cursor);
>         }
>+        if (!parent) {
>+            if (!clonedChild)
>+                obj = cursor;

This if-then is unnecessary, given the dominating basic blocks.  Invert if condition and remove then.

>+            else
>+                JS_POP_TEMP_ROOT(cx, &tvr);
>+            break;
>+        }
>         clonedChild = cursor;
>         cursor = parent;
>     }
>     fp->flags |= JSFRAME_POP_BLOCKS;
>     fp->scopeChain = obj;
>     fp->blockChain = NULL;
>     return obj;
> }

Igor should review too -- sorry I did a poor job reviewing the last patch here.

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 237063 [details] [diff] [review]
Updated

Need to get this into the trunk pretty quickly.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Maybe better

The last patch did the wrong thing if the only object on fp->blockChain had a NULL parent.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 237072 [details] [diff] [review]
Maybe better

Looks good to me again, but I'm not counting for much here.  Igor's r+ counts.

Thanks for patching.

/be

Comment 11

[Igor Bukanov]

Comment on attachment 237072 [details] [diff] [review]
Maybe better

Right, I forgot that it is not enough to check for cases when n = 0, 1, 3, 4, etc. n = 2 can also happen ;) Especially when n is remaining number of blocks to clone :(

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 237072 [details] [diff] [review]
Maybe better

This fixes a regression from the patch for bug 351413 which is currently baking to get on the 1.8 branch.

Comment 14

[Igor Bukanov]

Comment on attachment 237072 [details] [diff] [review]
Maybe better

I attached a patch for 1.8.1 to bug 351413 that includes the fix.

Comment 15

[Bob Clary [:bc] (inactive)]

Checking in regress-351606.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-351606.js,v  <--  regress-351606.js
initial revision: 1.1
done

Comment 16

[Igor Bukanov]

"Fixed" on 1.8.1 branch: the committed patch for 1.8.1 does not have a regression.

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20060909 windows/mac*/linux

Comment 18

[Bob Clary [:bc] (inactive)]

Checking in regress-351606.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-351606.js,v  <--  regress-351606.js
new revision: 1.2; previous revision: 1.1
done

enclosed statement in function definition.