590813 - TM: Crash on nested Iterator iteration

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

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.

Comment 1

[Brian Hackett [Laid off!]]

There's a patch for JM in bug 589112 which handles this by cloning the iterator on nested iteration.

Comment 2

[Andreas Gal :gal]

Was this broken in my code or did your changes break this? Can you fix for TM too?

Comment 3

[Brian Hackett [Laid off!]]

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

[Andreas Gal :gal]

Can you check whether this affects any products? If not please reopen the bug.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: fix

This patch fixes things for nested iterators by always cloning Iterator objects when they are used in 'for in' loops.

Comment 6

[Daniel Veditz [:dveditz]]

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

[Andreas Gal :gal]

Brian, whats going on here? Why are we getting an active iterator object from the cache? Thats the bug. Cloning seems the wrong fix.

Comment 8

[Brian Hackett [Laid off!]]

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

[Andreas Gal :gal]

Yeah this fix doesn't feel right. Native iterators should never escape though. So is this a problem for scripted/custom iterators only?

Comment 10

[Brian Hackett [Laid off!]]

This is only a problem when a script uses the Iterator function.

Comment 11

[Brian Hackett [Laid off!]]

Attachment: better fix

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.

Comment 12

[Andreas Gal :gal]

Comment on attachment 469923 [details] [diff] [review]
better fix

This makes the memory footprint a little worse. Probably doesn't matter.

Comment 13

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/tracemonkey/rev/a902803033d7

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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).

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a902803033d7