Consider the following example: function getter() { return 1; } function setter(v) { return v; } var xml = <a xmlns="foo:bar"/>; Array.prototype.__defineGetter__(0, getter); Array.prototype.__defineSetter__(0, setter); xml.namespace(); Currently it crashes because xml_inScopeNamespaces assumes that Array instance may only contain array elements it explicitly put there. As this example demonstrates, this is not the case since script can define a getter on Array.prototype.

Here is a pure GC-hazard in xml_namespaceDeclarations where the setter for the first element unroots the second namespace stored in local variables and accessed later: var xml = <tag xmlns:n="uri:1" xmlns:n2="uri:2" n:a="1" n2:a="1"/>; function getter() { } function setter(v) { delete xml.@*::a; xml.removeNamespace("uri:2"); gc(); } Array.prototype.__defineGetter__(0, getter); Array.prototype.__defineSetter__(0, setter); xml.namespaceDeclarations(); ~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/files/y.js before 9232, after 9232, break 08141000 Segmentation fault

The patch changes XML not to use JS Array instances for temporaries. Instead it uses a rooted wrapper around XMLArray to store namespaces temporary. Note that the patch roots more then strictly necessary, but I prefer this bit of paranoia hunting new bugs in future.

I am changing the title to reflect more generic nature of the bug as it exists not only in jsxml.c. Consider the following: ~/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 The crash happens because NewKeyValuePair in jsiter.c gets just weakly rooted value as its val argument and passes it to js_NewArrayObject as the second element of the local array. So setter for the first can unroot and collect it. Note that the bug would not exist if jsinterp.c would pass a pointer to a rooted location to js_CallIteratorNext as rval and I wish a year ago all those local "jsval v" would be banned from sources.

Comment on attachment 240137 [details] [diff] [review] Fix >+ /* After thos point the control must flow through label out. */ s/thos/this/ > } Nicer with blank line here? >+ if (!ns) { >+ *rval = JSVAL_VOID; >+ } else { >+ nsobj = js_GetXMLNamespaceObject(cx, ns); >+ if (!nsobj) { >+ ok = JS_FALSE; >+ goto out; >+ } >+ *rval = OBJECT_TO_JSVAL(ns->object); Use nsobj instead of ns->object. >+ /* Finishing must be in reveres order of initialization to follow LIFO. */ s/reveres/reverse/ Nice cleanup, separates XMLArray to JS Array from E4X guts, probably for net code size savings -- true? Nominate for 1.8.1 as soon as this lands -- there's a chance to get this into rc2. /be

(In reply to comment #3) > Note that the bug would not exist if jsinterp.c would pass a pointer to a > rooted location to js_CallIteratorNext as rval and I wish a year ago all those > local "jsval v" would be banned from sources. Me too, but they're too easy to type. There is precedent for passing a vp that points to the operand stack, instead of v or &v. That's still not enforced by the type system, but better. But thanks to you we have other bugs on mandatory explicit rooting or better implicit rooting. Want me to patch jsiter.c, or are you on it? Thanks, /be

(In reply to comment #5) > Want me to patch jsiter.c, or are you on it? I would like to change jsinter.c to pass a pointer to a rooted location to js_CallIteratorNext, but I do not know where to get one. Any hints?

Patch to commit with nits addressed.

(In reply to comment #6) > (In reply to comment #5) > > Want me to patch jsiter.c, or are you on it? > > I would like to change jsinter.c to pass a pointer to a rooted location to > js_CallIteratorNext, but I do not know where to get one. Any hints? The operand stack will have room for the true or false value ultimately pushed at end_forinloop:, so you could use that as a scanned local root, provided fp->sp is set to a greater address to protect it during calls out of the interpreter code. The SAVE_SP_AND_PC(fp) done just before the "Is this the first iteration?" comment soon after do_forinloop: would want to pre-increment sp, and the one use of sp that I see after this point and before end_forinloop would want to change from sp[i-depth] to sp[i-depth-1] of course. The sp += i + 1 done first thing at end_forinloop: would need to change to += i, and PUSH_OPND(rval) on the next line would be STORE_OPND(-1, rval). JSOP_FORELEM pushes the currently enumerated string id's value before control flow reaches end_forinloop, a fly in the ointment. It could claim the protected stack slot, and adjust sp or i so that the revised STORE_OPND at end_forinloop works as before, leaving the stack two deeper for this opcode (so from initial state [current-obj-on-proto-chain, iterobj] JSOP_FORELEM leaves the stack looking like [current-obj-on-proto-chain, iterobj, id-as-string, true-or-false] thus its nuses of 2 and ndefs of 4 in jsopcode.tbl). /be

(In reply to comment #8) > JSOP_FORELEM pushes the currently enumerated string id's value before control Super-clarification: s/string id's value/id's string value/. And of course with the iteration protocol, rval could be any value -- it's just the next value in the iteration. /be

(In reply to comment #4) > Nice cleanup, separates XMLArray to JS Array from E4X guts, probably for net > code size savings -- true? jsxml.c compiled with GCC 4.0 and -Os: before the patch: 46220 bytes after the patch: 46156 bytes So the patch in fact saved the space indeed. But that was not the goal ;) > Nominate for 1.8.1 as soon as this lands -- there's a chance to get this into > rc2. Well, I would bet 500 USD that if this waits until FF 2.0.1, no harm would be done. yes, there is no regressions with e4x tests, but still it is not a one-line fix.

I committed the patch from comment 7 to the trunk: Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.127; previous revision: 3.126 done I keep the bug as assigned since the patch fixes only the tests from comment 0 and comment 1, as the problem from comment 3 is still not addressed.

Rooting rval is not enough since there is another pointer to unrooted location that is passed to js_CallIteratorNext. That is, idp pointer. Eventually this pointer will be passed to CheckKeyValueReturn. There either through overwriting Iterator.prototype.next or when !JS_HAS_LVALUE_RETURN, the code will execute: if (!OBJ_GET_PROPERTY(cx, obj, INT_TO_JSID(0), &idval)) return JS_FALSE; if (!JS_ValueToId(cx, idval, idp)) return JS_FALSE; if (flags & JSITER_KEYVALUE) return JS_TRUE; return OBJ_GET_PROPERTY(cx, obj, INT_TO_JSID(1), rval); If obj array here defines a getter for the second element, it can unroot and GC the first. Then the interpreter access the unrooted value. Again, this can be addressed in the iter.c, but that is sweeping dust under the carpet since the real problem is that idp points to unrooted location. As far as I can see there is already an extra slot besides the slot for the final true/false/what ever. That is, rval returned from js_CallIteratorNext after some processing ends up in various fp->argv[slot] or stack locations so why not use that for rval in the first place? Then the stack top can be used for rooting of fid. Of cause, that would require to change js_CallIteratorNext to accept jsval*, not jsid*, but that is minor.

Example exposing the problem from comment 12 (so there 4 such problems in total): ~/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 changing the title again to be specific for jsxml.c and will file a separated bug for jsiter.c since jsiter.c problems applies unly to 1.8.1 and later while jsxml.c bug also exists in 1.8.0.

I filed 354499 bug for iterating problems and copied the examples from comment 3 and comment 13 there. As such the bug is fixed now on the trunk since the fix for test cases from comment 0 and comment is 1 already committed.

(In reply to comment #15) > I filed 354499 bug I meant bug 354499 ;)

verified fixed 1.9 20061002 windows/linux no crash on either test. Note that the spider-based harness failed to report but there was _no crash_. Will investigate.

Comment on attachment 240186 [details] [diff] [review] Fix v1b This is a GC hazard fix

Brendan - do we need for 181?

Yes, sorry I missed this -- we should take it. It's a well-baked GC safety fix that is needed for the immutability-assumption-violation bug 354145 fix. We want the other dependency of 354145 too. /be

Comment on attachment 240186 [details] [diff] [review] Fix v1b we'll approve for 1.8.0.8 if this gets approved for rc3

Comment on attachment 240186 [details] [diff] [review] Fix v1b The patch does not apply on 1.8.0 branch

The patch for trunk/1.8.1 does not apply to 1.8.0 for the following reasons: 1. 1.8.0 does not have JS_PUSH_TEMP_ROOT_MARKER macro so the patch copies that code from 1.8.0 2. GC_MARK and namespace_mark_vector on the branch takes an extra parameter to support GC_MARK_DEBUG. I fixed that via passing NULL there. 3. The branch does not have js_EqualStrings() so I replaced that by calls to js_CompareString.

Comment on attachment 241757 [details] [diff] [review] Fix v1b for 1.8.0 branch I diff'ed the diffs to verify -- looks good, this should go in for rc3. /be

Comment on attachment 241757 [details] [diff] [review] Fix v1b for 1.8.0 branch a=beltzner on behalf of drivers for 1.8.1 RC 3

Comment on attachment 241757 [details] [diff] [review] Fix v1b for 1.8.0 branch Oops, assuming this is the 1.8.0 branch fix, not the 1.8 branch fix, so minusing. I'll plus the other one.

Comment on attachment 240186 [details] [diff] [review] Fix v1b a=beltzner on behalf of drivers for 1.8.1 RC 3

I committed the patch from comment 7, Fix v1b, to MOZILLA_1_8_BRANCH: Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.51; previous revision: 3.50.2.50 done

Comment on attachment 241757 [details] [diff] [review] Fix v1b for 1.8.0 branch approved for 1.8.0 branch, a=dveditz

I committed the patch from comment 25 to MOZILLA_1_8_0_BRANCH: Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.80.4.2.2.6; previous revision: 3.80.4.2.2.5 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.104.2.3.2.11; previous revision: 3.104.2.3.2.10 done Checking in jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.50.2.15.2.21; previous revision: 3.50.2.15.2.20 done

verified fixed 1.8 20061010 windows/mac*/linux

verified fixed 1.8.0.8 win/mac*/linux firefox_1.8.0.8pre_2006101114_dbg firefox_1.8.0.8pre_2006101114_opt

reset Array.prototype[0]

reset Array.prototype[0]

Bug not relevant to aviary/moz1.7 branches

RCS file: /cvsroot/mozilla/js/tests/e4x/Regress/regress-354151-01.js,v done Checking in regress-354151-01.js; /cvsroot/mozilla/js/tests/e4x/Regress/regress-354151-01.js,v <-- regress-354151-01.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/js/tests/e4x/Regress/regress-354151-02.js,v done Checking in regress-354151-02.js; /cvsroot/mozilla/js/tests/e4x/Regress/regress-354151-02.js,v <-- regress-354151-02.js initial revision: 1.1 done