Closed Bug 589112 Opened 14 years ago Closed 14 years ago

JM: "Assertion failure: ni->flags & JSITER_ACTIVE,"

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: gkw, Assigned: bhackett1024)

References

Details

(Keywords: assertion, regression, testcase)

Attachments

(1 file, 3 obsolete files)

f = eval("(function(){return x=Iterator(/x/)})")
for (a in f()) {}
for (d in x) {}

asserts js debug shell on JM changeset 8a0513a5c024 without -j nor -m at Assertion failure: ni->flags & JSITER_ACTIVE, at ../jsiter.cpp:852

I checked and this doesn't seem to occur on TM changeset b22e82ce2364


Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x00168fc4 in JS_Assert (s=0x28134e "ni->flags & JSITER_ACTIVE", file=0x280f5b "../jsiter.cpp", ln=852) at ../jsutil.cpp:80
80          *((int *) NULL) = 0;  /* To continue from here in GDB: "return" then "continue". */
(gdb) bt
#0  0x00168fc4 in JS_Assert (s=0x28134e "ni->flags & JSITER_ACTIVE", file=0x280f5b "../jsiter.cpp", ln=852) at ../jsutil.cpp:80
#1  0x000c4aa0 in js_CloseIterator (cx=0x60a550, obj=0x14022d0) at ../jsiter.cpp:852
#2  0x0009a395 in js::Interpret (cx=0x60a550, entryFrame=0x10000a8, inlineCallCount=0) at ../jsinterp.cpp:3037
#3  0x000bca5e in js::RunScript (cx=0x60a550, script=0x60c2e0, fun=0x0, scopeChain=0x1402000) at jsinterp.cpp:468
#4  0x000be3c3 in js::Execute (cx=0x60a550, chain=0x1402000, script=0x60c2e0, down=0x0, flags=0, result=0xbffff6e0) at jsinterp.cpp:944
#5  0x0001719a in JS_ExecuteScript (cx=0x60a550, obj=0x1402000, script=0x60c2e0, rval=0xbffff6e0) at ../jsapi.cpp:4744
#6  0x0000ca35 in Process (cx=0x60a550, obj=0x1402000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:535
#7  0x0000d43b in ProcessArgs (cx=0x60a550, obj=0x1402000, argv=0xbffff8b0, argc=0) at ../../shell/js.cpp:862
#8  0x0000d554 in shell (cx=0x60a550, argc=0, argv=0xbffff8b0, envp=0xbffff8b4) at ../../shell/js.cpp:5151
#9  0x0000d678 in main (argc=0, argv=0xbffff8b0, envp=0xbffff8b4) at ../../shell/js.cpp:5247
Assignee: general → dmandelin
Handing this off. This is a bug in the single-item iterator cache. The bug behavior goes like this:

1. Create an iterator. It is "registered" and marked active.
2. Iterate using that iterator. When iteration starts, the existing iterator is found in the last loop in js_ValueToIterator.
3. Close the iterator. It is marked inactive.
4. Iterate again with the same iterator. Again, when iteration starts, the existing iterator is found in the last loop in js_ValueToIterator. But it is not registered again.
5. Close the iterator. We assert, because it is already marked inactive.

I tried making js_ValueToIterator register the iterator if it is not marked active, but it just crashes elsewhere.
Assignee: dmandelin → bhackett1024
Attached patch fix (obsolete) — Splinter Review
This patch fixes this testcase, passes tests and trace-tests, and asserts when a nested iterator is used (bug 590813) instead of SEGV'ing as stock TM does.
Attachment #469333 - Flags: review?
Attached patch fix for nested iterators (obsolete) — Splinter Review
This handles nested iteration on the same Iterator by cloning the iterator.
Attachment #469333 - Attachment is obsolete: true
Attachment #469510 - Flags: review?(dmandelin)
Attachment #469333 - Flags: review?
Comment on attachment 469510 [details] [diff] [review]
fix for nested iterators

I assume this passes jstests and trace-tests?

r=me with a couple of changes:

- Add a comment, probably near JSITER_ACTIVE in jsiter.h, explaining the purpose of the active flag and what it means.

>+            if (ni->flags & JSITER_ACTIVE) {
>+                /*
>+                 * Nested iteration on the same Iterator object.  We will get deeply
>+                 * confused if we try to reuse the iterator, so clone it instead.
>+                 */
>+                uintN nflags = ni->flags & ~JSITER_ACTIVE;
>+                iterobj = NewIteratorObject(cx, nflags);
>+                if (!iterobj)
>+                    return false;
>+
>+                NativeIterator *nni =
>+                    NativeIterator::allocateValueIterator(cx, ni->beginValue(), ni->numValues());
>+                if (!nni)
>+                    return false;
>+
>+                nni->init(ni->obj, nflags, 0, 0);
>+                iterobj->setNativeIterator(nni);
>+                ni = nni;

    Maybe put
                  JS_ASSERT(!(ni->flags & JSITER_ACTIVE))

    here. The following line surprised me until I realized that |ni| is changed in the if.
>+            }
>+
>+            ni->flags |= JSITER_ACTIVE;

We should also get some more guidance on semantics. Brendan, is cloning the right thing to do?
Attachment #469510 - Flags: review?(dmandelin)
Attachment #469510 - Flags: review?(brendan)
Attachment #469510 - Flags: review+
Attached patch fixed patch (obsolete) — Splinter Review
Actually, the last patch did fail a jstest.  This one passes jstests and trace-tests, and fixes that ugly nni stuff to be clearer.
Attachment #469510 - Attachment is obsolete: true
Attachment #469568 - Flags: review?(dmandelin)
Attachment #469510 - Flags: review?(brendan)
Attachment #469568 - Flags: review?(dmandelin) → review+
Language semantics are clear enough without me (I hope -- if not from the spec then from our past behavior and past and current behavior in other browsers, which while it varies for prototype and dense-array reasons does not vary AFAIK for nesting reasons).

Andreas really should be in the loop since this is all really new code for the fastiterators bug.

/be
My understanding (from IRC) is that the Iterator function (not anything else to do with iteration) is our own thing; are there specs for our own language features besides what's on MDN?  JSC and V8 do not recognize Iterator.  My 3.6.8 browser doesn't crash on nesting an Iterator, but its behavior doesn't make much sense either --- it runs the outer loop once, the inner loop once (fetching the first value for both), then stops.
Sorry, Iterator is indeed ours, but you can resolve what to do by asking "What would Python do?"

=== i.py
x = None

def f():
    global x
    x = iter([0,1,2])
    return x

for a in f():
    print 'a', a
for d in x:
    print 'd', d

=== i.py output
a 0
a 1
a 2

=== i2.py
x = None

def f():
    global x
    x = iter([0,1,2])
    return x

for a in f():
    print 'a', a
    for d in x:
        print 'd', d

=== i2.py output
a 0
d 1
d 2

=== i.js
function f(){return x=Iterator([0,1,2])};
for (a in f())
    print('a', a[1]);
for (d in x)
    print('d', d[1]);

=== i.js output
a 0
a 1
a 2

=== i2.js
function f(){return x=Iterator([0,1,2])};
for (a in f()) {
    print('a', a[1]);
    for (d in x)
        print('d', d[1]);
}

=== i2.js output
a 0
d 1
d 2

Modulo our backward compatibility constrains and the changes they induced in Iterator (re: iter, specifically for-in in JS goes over keys for arrays, not values of sequence types as in Python; wherefore Iterator returns key-value pairs always), this all works.

Er, it works in a Fx3-era JS shell. In m-c and tm debug shells, i2.js crashes!

$ ./Darwin_DBG.OBJ/js /tmp/i2.js
a 0
d 1
d 2
Segmentation fault

(gdb) r /tmp/i2.js
Starting program: /Users/brendaneich/Hacking/hg.mozilla.org/mozilla-central/js/src/Darwin_DBG.OBJ/js /tmp/i2.js
Reading symbols for shared libraries .++++..................................................................................... done
a 0
d 1
d 2

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000010
0x0000000100090bd9 in IteratorMore (cx=0x100612980, iterobj=0x101603438, cond=0x7fff5fbfe51b, rval=0x1010001b8) at ../jsinterp.cpp:2017
warning: Source file is more recent than executable.
2017	        *cond = (ni->props_cursor < ni->props_end);
(gdb) bt 
#0  0x0000000100090bd9 in IteratorMore (cx=0x100612980, iterobj=0x101603438, cond=0x7fff5fbfe51b, rval=0x1010001b8) at ../jsinterp.cpp:2017
#1  0x000000010009758c in js::Interpret (cx=0x100612980) at ../jsinterp.cpp:2890
#2  0x00000001000bb4f9 in js::Execute (cx=0x100612980, chain=0x101603000, script=0x100614960, down=0x0, flags=0, result=0x0) at jsinterp.cpp:886
#3  0x0000000100015709 in JS_ExecuteScript (cx=0x100612980, obj=0x101603000, script=0x100614960, rval=0x0) at ../jsapi.cpp:4754
#4  0x000000010000a6b7 in Process (cx=0x100612980, obj=0x101603000, filename=0x7fff5fbff8eb "/tmp/i2.js", forceTTY=0) at ../../shell/js.cpp:440
#5  0x000000010000b2eb in ProcessArgs (cx=0x100612980, obj=0x101603000, argv=0x7fff5fbff770, argc=1) at ../../shell/js.cpp:854
#6  0x000000010000b3d3 in shell (cx=0x100612980, argc=1, argv=0x7fff5fbff770, envp=0x7fff5fbff780) at ../../shell/js.cpp:5047
#7  0x000000010000b4cf in main (argc=1, argv=0x7fff5fbff770, envp=0x7fff5fbff780) at ../../shell/js.cpp:5134

Maybe that's due only to this bug? I didn't see an assertion botching. Andreas?

/be
Anyway, an escaping, explicitly constructed iterator is not mysterious. Whoever calls .next implicitly or explicitly gets the next value, until it is exhausted. That explains the two-loops-in-a-row and nested-for-in-loops behaviors.

This bug needs to block, and the null pointer dereference (if different) needs to be filed, and it should block too.

/be
blocking2.0: --- → ?
The null dereference is indeed a different bug, see bug 590813.  This one is only in JM.  I'll fix both today.
Attached patch right fixSplinter Review
This fixes the assertion, handles nested iteration per comment 8, and adds test cases for this and bug 590813.
Attachment #469568 - Attachment is obsolete: true
Attachment #469944 - Flags: review?(dmandelin)
Attachment #469944 - Flags: review?(dmandelin) → review+
blocking2.0: ? → beta6+
Now that JM branch has landed on TM branch, is this fixed-in-tracemonkey?
We've been marking JM-only bugs fixed when they get fixed there, so this probably should have been marked fixed when it landed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: