This is a spin-off of the bug 354151. See comments there related to incorrect assumptions about Array instances while iterating. To summaries, the are 2 independent bugs caused by jsinterp.c calling js_CallIteratorNext with 2 pointers to unrooted locations where GC-things can be stored. Example 1 (bug 354151 comment 3): -------------------------------------------- ~/m/trunk/mozilla/js/src> cat ~/m/files/y.js var obj = {get a(){ return new Object(); }}; function setter(v) { // Push out obj.a from all temp roots var tmp = { get toString() { return new Object(); }}; try { String(tmp); } catch (e) { } gc(); } Array.prototype.__defineGetter__(0, function() { }); Array.prototype.__defineSetter__(0, setter); for (var i in Iterator(obj)) print(uneval(i)); ~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y.js Key value before 9232, after 9232, break 08142000 Segmentation fault -------------------------------------------- Example 2 (bug 354151 comment 13): -------------------------------------------- ~/m/trunk/mozilla/js/src> cat ~/m/files/y.js function get_value() { // Unroot the first element this[0] = 0; // Call gc to collect atom corresponding to Math.sqrt(2) gc(); } var counter = 2; Iterator.prototype.next = function() { if (counter-- <= 0) throw StopIteration; var a = [Math.sqrt(2), 1]; a.__defineGetter__(1, get_value); return a; }; for (i in [1]); ~/m/trunk/mozilla/js/src> js ~/m/files/y.js before 9232, after 9232, break 08180000 Segmentation fault

I think the best way to address this would be to move the current code that follows the call to js_CallIteratorNext in jsinterp.c until the code that assigns the result using switch before end_forinloop into js_CallIteratorNext itself. This still would require to replace &rval by a pointer to a rooted location as described in bug 354151 comment 8, but the end result would more managable code I believe. For example, in the case of JSITER_ENUMERATE, only fid would eventially becomes rval and there is no need to read rval from the [key, value] pair in CheckKeyValueReturn. But to avoid passing to js_CallIteratorNext way to much parameters I need to clarify the following: 1. jsinterp.c starting from line 2731 contains the following comments: /* * Store the current object below the iterator for generality: * with the iteration protocol, we cannot assume that a native * iterator was found or created by js_ValueToIterator, so we * can't use its parent slot to track the current object being * iterated along origobj's prototype chain. We need another * stack slot, which JSOP_STARTITER allocated for us. */ vp[-1] = OBJECT_TO_JSVAL(obj); But as far as I understand, it is necessary to track the prototype chain only for JSITER_ENUMERATE iterator. And that can only be the standard native iterator, which has obj in its parent slot. Did I miss something? 2. Line 2851 contains: if (flags & JSITER_FOREACH) { /* Clear the local foreach flag set by our prefix bytecode. */ flags = 0; But flags are not used after that AFAICS. Is this line really necessary?

(In reply to comment #1) > But as far as I understand, it is necessary to track the prototype chain only > for JSITER_ENUMERATE iterator. And that can only be the standard native > iterator, which has obj in its parent slot. Did I miss something? No, I did -- and this allows us to shrink the stack budget allocated by JSOP_STARTITER and freed (along with another slot) by JSOP_ENDITER by one slot. I should have seen this -- thanks for pointing it out. Can you patch this bug today? I'll do it otherwise, but I think I should fire myself as enumeration vs. iteration hacker. > 2. Line 2851 contains: > if (flags & JSITER_FOREACH) { > /* Clear the local foreach flag set by our prefix bytecode. */ > flags = 0; > > But flags are not used after that AFAICS. Is this line really necessary? Evidently not -- a holdover from previous revisions, or always-useless code. /be

This is 1.8.1 and later bug as it affects only the new code implementing JS 1.7 iterator protocol. In 1.8.0 and earlier versions the enumerator never creates any temporary arrays. Thus "blocking 1.8.0.9" flag can be set to "-".

About a fix. I would like to do it after fixing bug 354982 as it would be much simpler to do after the cleanup.

A minimal fix. Strictly speaking the rooting in CheckKeyValueReturn is not necessary as I already learned from bug 354982 that the rval is not used. But I want to have a bulletproof fix.

Comment on attachment 241230 [details] [diff] [review] Fix v1 Is the atom (not its key) GC-safe? Only if a last ditch GC is the only kind of GC that could nest. If a full GC could be forced, the atom (id) could be unreachable (if the property is deleted) and then collected. /be

Note that this would be conflicting with patch for 354982 but the remedy there is trivial: continue to remove the CheckKeyValueReturn and keep the fix for NewKeyValuePair.

Fix against the trunk. Unless the patch from bug 354982 is also landed, the fix for 1.8.1 would aslo require rooting of the atom in CheckKeyValueReturn.

Comment on attachment 241264 [details] [diff] [review] Fix v2 The patch applies to 1.8.1 as is and fixes explotable GC hazard.

Comment on attachment 241264 [details] [diff] [review] Fix v2 I like it, but we usually don't make such explicit comments for patches to s-s bugs. Not that it adds much info, but .... Drivers: if you don't approve for 1.8.1, please nominate for 1.8.1.1. /be

Patch to commit with comments removed.

I committed the patch from comment 11 to the trunk: Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.49; previous revision: 3.48 done

Comment on attachment 241271 [details] [diff] [review] Fix v2 without comments Asking for approval for the committed patch.

This a diff with smaller context so the patch can be applied to 1.8.1 with or without the cleanup patch from bug 355410.

(In reply to comment #14) > Created an attachment (id=241272) [edit] > Fix v2 without comments and smaller diff context > > This a diff with smaller context so the patch can be applied to 1.8.1 with or > without the cleanup patch from bug 355410. The cleanup patch from bug 354982, if I'm following. /be

(In reply to comment #15) > > This a diff with smaller context so the patch can be applied to 1.8.1 with or > > without the cleanup patch from bug 355410. > > The cleanup patch from bug 354982, if I'm following. Yes, bug 354982. Late night hacking, you know.

I could not reproduce a crash with this test.

verified fixed 1.9 20061005 windows/linux

Blocking for Fx2 RC3

Comment on attachment 241272 [details] [diff] [review] Fix v2 without comments and smaller diff context Approved for RC3.

I committed the patch from comment 11 to MOZILLA_1_8_BRANCH: Checking in jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.17.2.24; previous revision: 3.17.2.23 done

verified fixed 20061009 1.8 windows/linux/mac* 1.9 windows/linux note to self: for the -01 test the harness failed to report the test results but did not crash.

reset Array.prototype[0]

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-354499-01.js,v <-- regress-354499-01.js /cvsroot/mozilla/js/tests/js1_7/extensions/regress-354499-02.js,v <-- regress-354499-02.js