341510 - Iterators: crash in close handler with assignment to non-existing name

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Consider the following test case for JS shell:

function gen(i) { yield i; }
var iter = gen(1);
iter.close = function() { name_that_does_not_exist_in_the_scope_chain = 1; }
iter = null;
gc();


Currently it segfaults during execution of the close handler on assignment.

Comment 1

[Igor Bukanov]

I was wrong that the bug comes from the assignment. According to the stack trace it comes from marking phase:

#0  0x080e159f in js_SearchScope (scope=0x9b31fd0, id=0, adding=0)
    at jsscope.c:265
        hash0 = 3219092864
        hash1 = 9701048
        hash2 = 10957144
        hashShift = 134794443
        sizeLog2 = 4104
        stored = (JSScopeProperty *) 0x18
        sprop = (JSScopeProperty *) 0xa5dc0c
        spp = (JSScopeProperty **) 0x80b8196
        firstRemoved = (JSScopeProperty **) 0x9b2d8e8
        sizeMask = 63194805
#1  0x080b7fc5 in js_Mark (cx=0x9b2d8e8, obj=0x9b2ee90, arg=0x0)
    at jsobj.c:4448
        scope = (JSScope *) 0x9b31fd0
        sprop = (JSScopeProperty *) 0x150
        clasp = (JSClass *) 0x9b2ec10
#2  0x08087fdd in MarkGCThingChildren (cx=0x9b2d8e8, thing=0x9b2ee90, 
    flagp=0x9b2eb02 "\020\001", shouldCheckRecursion=1) at jsgc.c:1491
        rt = (JSRuntime *) 0x9b2a828
        obj = (JSObject *) 0x9b2ee90
        v = 162703620
        vp = (jsval *) 0x9b420ec
        end = (jsval *) 0xbfdf7598
        next_thing = (void *) 0x9b2ec00
        next_flagp = (uint8 *) 0x150 <Address 0x150 out of bounds>
        str = (JSString *) 0x52
        stackDummy = 162720784
#3  0x08088c1f in js_MarkGCThing (cx=0x9b2d8e8, thing=0x9b2ee90) at jsgc.c:1874
        flagp = (uint8 *) 0x9b2eb02 "\020\001"
#4  0x08088e9b in js_MarkStackFrame (cx=0x9b2d8e8, fp=0xbfdf7754)
    at jsgc.c:1988
        depth = 134604006
        nslots = 162716248
#5  0x08089827 in js_GC (cx=0x9b2d8e8, gcflags=0) at jsgc.c:2404
        rt = (JSRuntime *) 0x9b2a828
        iter = (JSContext *) 0x9b2d8e8
        acx = (JSContext *) 0x9b2d8e8
        fp = (JSStackFrame *) 0xbfdf7754
        chain = (JSStackFrame *) 0xbfdf7754
        i = 10
        type = 4
        sh = (JSStackHeader *) 0x0
        tvr = (JSTempValueRooter *) 0x0
        nbytes = 80
        limit = 8192
        offset = 224
        a = (JSGCArena *) 0x0
        ap = (JSGCArena **) 0x9b2a898
        flags = 4 '\004'
        flagp = (uint8 *) 0x9b3d1c2 '&#65533; <repeats 200 times>...
        firstPage = (uint8 *) 0x9b3d400 '&#65533; <repeats 200 times>...
        thing = (JSGCThing *) 0x9b3d490
        freeList = (JSGCThing *) 0x0
        arenaList = (JSGCArenaList *) 0x9b2a898
        finalizer = 0
        allClear = 1
#6  0x08088e33 in js_ForceGC (cx=0x9b2d8e8, gcflags=0) at jsgc.c:1963
        i = 16
#7  0x08052345 in JS_GC (cx=0x9b2d8e8) at jsapi.c:1906
No locals.
#8  0x0804a55e in GC (cx=0x9b2d8e8, obj=0x9b2ee90, argc=0, argv=0x9b42540, 
    rval=0xbfdf7774) at js.c:728
        rt = (JSRuntime *) 0x9b2a828
        preBytes = 9232
#9  0x0808c831 in js_Invoke (cx=0x9b2d8e8, argc=0, flags=0) at jsinterp.c:1328
        mark = (void *) 0x9b42548
        fp = (JSStackFrame *) 0xbfdf81c4
        frame = {callobj = 0x0, argsobj = 0x0, varobj = 0x9b2ee90, 
  script = 0x0, fun = 0x9b329d8, thisp = 0x9b2ee90, argc = 0, 
  argv = 0x9b42540, rval = -2147483647, nvars = 0, vars = 0x9b42540, 
  down = 0xbfdf81c4, annotation = 0x0, scopeChain = 0x9b2ee90, pc = 0x0, 
  sp = 0x9b42540, spbase = 0x0, sharpDepth = 0, sharpArray = 0x0, flags = 0, 
  dormantNext = 0x0, xmlNamespace = 0x0, blockChain = 0x0}
        sp = (jsval *) 0x9b42540
        newsp = (jsval *) 0x1
        limit = (jsval *) 0x9b2bfb0
        vp = (jsval *) 0x9b42538
        v = 162721832
        thisv = 162721424
        funobj = (JSObject *) 0x9b2f028
        parent = (JSObject *) 0x9b2ee90
        thisp = (JSObject *) 0x9b2ee90
        ok = 14
        clasp = (JSClass *) 0x8123700
        ops = (JSObjectOps *) 0x9b34b18
        native = 0x804a538 <GC>
        fun = (JSFunction *) 0x9b329d8
        script = (JSScript *) 0x0
        nslots = 0
        nvars = 0
        nalloc = 15
        surplus = 15
        hook = 0
        hookData = (void *) 0x0
...

So I changing the title to better reflect the reality.

Comment 2

[Igor Bukanov]

The bug happens because SPROP_MARK and ATOM_MARK is not set on new properties and atoms created during execution of close handlers. In the above test case gc would first execute the close handler and finalize just created scope properties since they were not marked even if they belong to live objects. Since the close handler sets gcPoke, GC is executed once again and this time segfaults when it marks just finalized scope properties.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> The bug happens because SPROP_MARK and ATOM_MARK is not set on new properties
> and atoms created during execution of close handlers.

Argh.  So obvious in hindsight.  You get to fix since you reviewed ;-).  Seriously, thanks for pursuing this.

/be

Comment 4

[Igor Bukanov]

I am going to fix this by invoking close handlers for unreachable objects *after* the finalization stage. Then nothing should be marked during the execution of the close handlers.

Comment 5

[Igor Bukanov]

Attachment: Fix

The patch makes a sandwich form 2 parts of the close phase and the sweeping phase. Now the close handlers are executed after all the finalization is done.

Comment 6

[Brendan Eich [:brendan]]

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

>+typedef struct JSObjectsToClose {
>+    uint32 count;
>+    uint32 offset;      /* JSGCCloseTable.array[offset, offset + count) are
>+                           objects to close by executing their close hooks */

Just a thought: put the overlong inline comment up front, as a major comment before typedef.

Also, offset suggests byte offset, but it's really an index.  Call it index?

>+    /*
>+     * First half of the close phase: loop over the objects that we
>+     * set aside in the close table and mark them to protect them
>+     * against finalization during sweeping phase.
>+     */
>+    index = pivot;
>+    do {
>+        GC_MARK(cx, array[index], "close-phase object");
>+    } while (++index != count);
>+
>+    /*
>+     * Mark children of things that caused too deep recursion during the
>+     * just-completed marking half of the close phase.
>+     */

More major comment nits: the first one above needs vim Q love, or by-hand reformatting (but vim is a lot faster).  The second fits tw=78.

Nits aside, good fix.  All that struggling to ensure newborns from the close hooks were marked goes away with post-finalization hook execution.

/be

Comment 7

[Igor Bukanov]

Attachment: Fix v2

In the previous patch when I moved the execution of close hooks to ExecuteCloseHooks I carelessly updated lines that calculates the number of elements in the close table after the close phase. So this patch fixes that and addresses the nits.

Comment 8

[Igor Bukanov]

Attachment: Fix v2 for real

I forgot to regenerate the diff with the previous attachment.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 225763 [details] [diff] [review]
Fix v2 for real

>+        /* Skip the close phase when threre are no objects to close. */

"there".

>+     * To ensure liveness during the sweep phase we mark all objects to close.

This reads slightly funny to me, how about: "objects we are about to close"?

Comment 10

[Igor Bukanov]

Attachment: Fix v2b

This version of the patch includes the suggestion from comment 9.

Comment 11

[Igor Bukanov]

I committed to the trunk the patch from comment 10. 

Comment 12

[Bob Clary [:bc] (inactive)]

RCS file: /cvsroot/mozilla/js/tests/js1_7/iterable/regress-341500.js,v
done
Checking in regress-341500.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341500.js,v  <--  regress-341500.js
initial revision: 1.1
done

Comment 13

[Bob Clary [:bc] (inactive)]

I fubarred the test name.  renamed test for bug 341510

Checking in regress-341510.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341510.js,v  <--  regress-341510.js
initial revision: 1.1

Comment 14

[Bob Clary [:bc] (inactive)]

crash in 1.9 debug browser windows 20060811 at

 	ntdll.dll!_RtlEnterCriticalSection@4()  + 0x90 bytes	

bug 341815 Comment #15 and bug 343455 Comment #40

Comment 15

[Bob Clary [:bc] (inactive)]

filed Bug 348606 on the _RtlEnterCriticalSection debug browser crash

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060818 windows/mac(ppc|tel)/linux

Comment 17

[Bob Clary [:bc] (inactive)]

Removing regress-341500.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341500.js,v  <--  regress-341500.js
new revision: delete; previous revision: 1.2
done