Closed
Bug 590813
Opened 14 years ago
Closed 14 years ago
TM: Crash on nested Iterator iteration
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: bhackett1024, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
When nesting iteration on the same Iterator object, TM crashes.
var x = Iterator([1,2,3]);
for (a in x) { for (b in x) { } }
I don't know what the right semantics are here.
Reporter | ||
Comment 1•14 years ago
|
||
There's a patch for JM in bug 589112 which handles this by cloning the iterator on nested iteration.
Comment 2•14 years ago
|
||
Was this broken in my code or did your changes break this? Can you fix for TM too?
Updated•14 years ago
|
blocking2.0: --- → ?
Reporter | ||
Comment 3•14 years ago
|
||
This is broken in your code (though it might have been broken before your iterator work; bug 589112 is definitely mine), I'll make a fix for TM and attach it here.
Comment 4•14 years ago
|
||
Can you check whether this affects any products? If not please reopen the bug.
Group: core-security
Reporter | ||
Comment 5•14 years ago
|
||
This patch fixes things for nested iterators by always cloning Iterator objects when they are used in 'for in' loops.
Attachment #469569 -
Flags: review?(gal)
Comment 6•14 years ago
|
||
This hangs up xpcshell in a recent trunk nightly, too. The process becomes unkillable on Mac until it eventually crashes. It appears to have sucked up a lot of my machine memory (remaining went to 0) although the memory wasn't credited to the xpcshell process. When it finally died on its own ("bus error", null dereference) the terminal window it was in was unusable and I had to kill it.
Comment 7•14 years ago
|
||
Brian, whats going on here? Why are we getting an active iterator object from the cache? Thats the bug. Cloning seems the wrong fix.
Reporter | ||
Comment 8•14 years ago
|
||
The iterator object isn't in the cache. The problem is that a script can call the Iterator function, which constructs and returns to the script a new iterator object (with getNativeIterator and everything). When someone uses that object in a 'for in' loop, it is used directly (calling .next() on it will advance the for loop, for example). See the 'Enumerate Iterator.prototype directly' test in js_ValueToIterator, and the iterator_iterator method which that test invokes.
The iteration code can't cope with the same native iterator being used in two nested 'for in' loops, so this patch always clones the native iterator when an existing Iterator object was passed in. The patch for JM has the JSITER_ACTIVE flag so can do this only when nested iteration is occurring.
These patches might just be band-aids on bigger issues related to using Iterator objects directly in 'for-in' loops which were constructed by and can be manipulated by scripts.
Comment 9•14 years ago
|
||
Yeah this fix doesn't feel right. Native iterators should never escape though. So is this a problem for scripted/custom iterators only?
Reporter | ||
Comment 10•14 years ago
|
||
This is only a problem when a script uses the Iterator function.
Reporter | ||
Comment 11•14 years ago
|
||
This avoids the SEGV by not eagerly destroying uncacheable native iterators at JSOP_ENDITER, and does the right thing with nested iteration per bug 589112 comment 8.
Attachment #469569 -
Attachment is obsolete: true
Attachment #469923 -
Flags: review?(gal)
Attachment #469569 -
Flags: review?(gal)
Comment 12•14 years ago
|
||
Comment on attachment 469923 [details] [diff] [review]
better fix
This makes the memory footprint a little worse. Probably doesn't matter.
Attachment #469923 -
Flags: review?(gal) → review+
Reporter | ||
Comment 13•14 years ago
|
||
Comment 14•14 years ago
|
||
autoBisect shows this is probably related to the following changeset:
The first bad revision is:
changeset: 41832:b15fd8b568e4
user: Andreas Gal
date: Fri May 07 17:52:52 2010 -0700
summary: fast object iteration (558754, r=brendan, CLOSED TREE).
Blocks: fastiterators
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 15•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•