QName in jsxml.c does not root uri value which is a result of js_ValueToString before passing it to js_NewXMLQName which can cause GC. The bug is visible in the following example when run in js shell compiled with TOO_MUCH_GC. On my box it asserts with: Assertion failure: flags != GCF_FINAL, at jsgc.c:1040 Trace/breakpoint trap var str = " foo:bar".substring(1); var expected = new QName(" foo:bar".substring(2), "a").uri; var likeString = { toString: function() { var tmp = str; str = null; return tmp; } }; var actual = new QName(likeString, "a").uri; print(actual.length); print(expected === actual);

js_ValueToString result is rooted by argv[1] right after successful return, and I don't see a crash or assertbotch. What am I missing? /be

(In reply to comment #1) > js_ValueToString result is rooted by argv[1] right after successful return, and > I don't see a crash or assertbotch. What am I missing? This is from line 933, http://lxr.mozilla.org/mozilla/source/js/src/jsxml.c#933 There uri is not rooted before calling js_NewXMLQName.

D'oh -- I stopped early in the debugger. Easily fixed, but what else is lurking, deracinated and deadly? /be

There are 16 hazards to do with js_ValueTo* calls, and possibly more involving callers of js_ValueTo* that propagate the return value. I'm trying to come up with a minimal patch. /be

(In reply to comment #4) > There are 16 hazards to do with js_ValueTo* calls, and possibly more involving > callers of js_ValueTo* that propagate the return value. I'm trying to come up > with a minimal patch. One of the 16 was fixed easily using an explicit local root -- argv[0] was going into js_ValueToString, so the result on success goes back to argv[0]. Indeed the signature should be changed to require a jsval *vp out parameter (I think without changing the return type, so we can relieve callers from JSVAL_TO_STRING usage). But the rest are fixable only with something like the cx->lastInvokeResult root that Igor proposed and patched in bug 312278 comment 6. Patch based on that work coming up. /be

Based on Igor's patch, so requesting r/sr=mrbkap/shaver -- Igor please review too. /be

Comment on attachment 200959 [details] [diff] [review] proposed fix >Index: jsapi.c >+ cx->lastInternalResult = JSVAL_NULL; Why not JSVAL_VOID? r=mrbkap

(In reply to comment #7) > (From update of attachment 200959 [details] [diff] [review] [edit]) > >Index: jsapi.c > >+ cx->lastInternalResult = JSVAL_NULL; > > Why not JSVAL_VOID? Either will do, and JSVAL_NULL is 0, faster and more compact code to store here. Also, stylistically highlights how this root is scanned if it holds a reference to a GC-thing: null is for references, undefined is for any uninitialized value. /be

Comment on attachment 200959 [details] [diff] [review] proposed fix sr=shaver

What about storing last invoke result in local scope if one is present? With such change JS_EnterLocalRootScope would take care about those cut-from-the-graph oldborns in the same way as it takes care about newborns. It would also make sure that if somebody discovers a way to overwrite toString for xml objects, then code like the following lines in jsxml.c would still be bug free: ok = JS_EnterLocalRootScope(cx); if (ok) { str = js_ValueToString(cx, OBJECT_TO_JSVAL(obj)); vstr = js_ValueToString(cx, v); ok = str && vstr; if (ok) *bp = !js_CompareStrings(str, vstr); JS_LeaveLocalRootScope(cx);

Here is a proof that the last patch is not enough based on the previous comment idea of overwriting xml.toString and cutting object from the graph to escape local root scope box. Even with the patch it still segfaults. var str = String(1); var expected = String(1); var x = new XML("text"); x.function::toString = function() { var tmp = str; str = null; return tmp; } var TWO = 2.0; var likeString = { toString: function() { var tmp = new XML(""); tmp = (tmp == "string"); if (typeof gc == "function") gc(); for (var i = 0; i != 40000; ++i) { tmp = 1e100 * TWO; tmp = null; } return expected; } } print(x == likeString);

Yes, those double js_ValueToString calls are a problem, and I spotted them when counting the 16 js_ValueToString hazards, but forgot to go back and do something about them. I like the idea of using the local root scope if possible, instead of cx->lastInternalResult. Patch soon. /be

Comment on attachment 201085 [details] [diff] [review] proposed fix, v2 >+ cx->lastInternalResult = JSVAL_NULL; So if we're going to leave this being JSVAL_NULL... >+ if (JSVAL_IS_GCTHING(acx->lastInternalResult)) { Why not null-check acx->lastInternalResult here so we don't mark NULL every time someone's using a local root scope. Or is it not worth it? Your call. r=mrbkap

Comment on attachment 201085 [details] [diff] [review] proposed fix, v2 sr=shaver

mrbkap, just for you: diff -u jsgc.c jsgc.c --- jsgc.c 28 Oct 2005 04:34:35 -0000 +++ jsgc.c 28 Oct 2005 18:00:57 -0000 @@ -1778,8 +1778,9 @@ if (acx->lastAtom) GC_MARK_ATOM(cx, acx->lastAtom, NULL); if (JSVAL_IS_GCTHING(acx->lastInternalResult)) { - GC_MARK(cx, JSVAL_TO_GCTHING(acx->lastInternalResult), - "lastInternalResult", NULL); + thing = JSVAL_TO_GCTHING(acx->lastInternalResult); + if (thing) + GC_MARK(cx, thing, "lastInternalResult", NULL); } #if JS_HAS_EXCEPTIONS if (acx->throwing && JSVAL_IS_GCTHING(acx->exception)) Fixed on trunk for baking. This should ride along for any 1.8rc2. /be

-01 passes with a current trunk cvs build, but -02 fails with actual == 1, expected == null. Did I botch the test?

(In reply to comment #19) > -02 fails with actual == 1, expected == null. The last line in test, TEST(1, x.toString(), likeString.toString());, is wrong: x.toString() at this stage would return null. You can replace that and the pevious line by TEST(1, x == likeString, true) since "==" already would expose the bug. Plus you do NOT need TOO_MUCH_GC since the test handles forcing of GC on its own.

moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.

Fixed on the 1.8 branch. /be

verified firefox 1.5 rc2 linux/win32 2005-11-07

testcase+ to get this off my radar. when this is made public, i will check in the test.

jsxml doesn't exist on the older branches

verified fixed trunk 20060328 win/mac/linux

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