Closed
Bug 589112
Opened 14 years ago
Closed 14 years ago
JM: "Assertion failure: ni->flags & JSITER_ACTIVE,"
Categories
(Core :: JavaScript Engine, defect)
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)
5.20 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
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
Updated•14 years ago
|
Assignee: general → dmandelin
Comment 1•14 years ago
|
||
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
Assignee | ||
Comment 2•14 years ago
|
||
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?
Assignee | ||
Comment 3•14 years ago
|
||
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 4•14 years ago
|
||
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+
Assignee | ||
Comment 5•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #469568 -
Flags: review?(dmandelin) → review+
Comment 6•14 years ago
|
||
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
Assignee | ||
Comment 7•14 years ago
|
||
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.
Comment 8•14 years ago
|
||
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
Comment 9•14 years ago
|
||
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: --- → ?
Assignee | ||
Comment 10•14 years ago
|
||
The null dereference is indeed a different bug, see bug 590813. This one is only in JM. I'll fix both today.
Assignee | ||
Comment 11•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #469944 -
Flags: review?(dmandelin) → review+
Assignee | ||
Comment 12•14 years ago
|
||
http://hg.mozilla.org/projects/jaegermonkey/rev/4af07fce189f
Updated•14 years ago
|
blocking2.0: ? → beta6+
Comment 13•14 years ago
|
||
Now that JM branch has landed on TM branch, is this fixed-in-tracemonkey?
Comment 14•14 years ago
|
||
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
Updated•11 years ago
|
Flags: in-testsuite+
Reporter | ||
Comment 15•11 years ago
|
||
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.
Description
•