Bug 516832 added a conservative stack scanner. Time to start using it.

Some gczeal fuzzing for this would be great.

As I wrote in bug 516832 comment 220 lets wait at least couple of weeks after the patch hits mozilla central and the corresponding nighties and we know for sure that there are no hard to fix leaks and request model violations (the patch strengthen the asserts in that area).

Is gczeal enough for testing this well, or do we need bug 569451?

(In reply to comment #4) > Is gczeal enough for testing this well, or do we need bug 569451? gczeal should be enough, but what is really desirable is testing in a browser. I suppose gczeal does not make that fast.

Comment on attachment 457217 [details] [diff] [review] patch 2ms speedup on SS, measured with 100 runs and a couple times. Its small but its there.

Lets wait with this until at least bugs, exposed by the initial patch for the bug 574313, are fixed. The weak roots may mitigate missing request calls so until we have sound enforcement of that we should not remove them IMO.

We shouldn't forget about this bug! Igor do you think it's time now?

(In reply to comment #10) > We shouldn't forget about this bug! Igor do you think it's time now? Yes, lets do it now. I have much more confidence in FF codebase following the request model.

Comment on attachment 457217 [details] [diff] [review] patch Few nits: >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp >--- a/js/src/jsapi.cpp >+++ b/js/src/jsapi.cpp > JS_PUBLIC_API(void) > JS_ClearNewbornRoots(JSContext *cx) > { >- JS_CLEAR_WEAK_ROOTS(&cx->weakRoots); > } Lets make JS_ClearNewbornRoots a no-op macro in jsapi.h. >@@ -4647,17 +4644,17 @@ JS_CompileUCFunctionForPrincipals(JSCont > > out: >- cx->weakRoots.finalizableNewborns[FINALIZE_FUNCTION] = fun; >+ ; > } > > out2: > LAST_FRAME_CHECKS(cx, fun); > return fun; > } Remove out and rename out2 into out here. > > JS_PUBLIC_API(JSFunction *) >diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp >--- a/js/src/jsarray.cpp >+++ b/js/src/jsarray.cpp >@@ -3183,19 +3183,16 @@ js_NewArrayObject(JSContext *cx, jsuint > { > AutoValueRooter tvr(cx, obj); > if (!InitArrayObject(cx, obj, length, vector, holey)) > obj = NULL; > } >- >- /* Set/clear newborn root, in case we lost it. */ >- cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = obj; > return obj; Remove an extra block scope here together with its scope so the code becomes just return InitArrayObject(...) ? obj : NULL; >@@ -3302,18 +3299,16 @@ js_NewArrayObjectWithCapacity(JSContext > AutoValueRooter tvr(cx, obj); > if (!obj->ensureDenseArrayElements(cx, capacity, JS_FALSE)) > obj = NULL; > >- /* Set/clear newborn root, in case we lost it. */ >- cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = obj; > if (!obj) > return NULL; Remove that obj = NULL and just return NULL.

rebase and address comments.

> > > >@@ -3302,18 +3299,16 @@ js_NewArrayObjectWithCapacity(JSContext > > AutoValueRooter tvr(cx, obj); > > if (!obj->ensureDenseArrayElements(cx, capacity, JS_FALSE)) > > obj = NULL; > > > >- /* Set/clear newborn root, in case we lost it. */ > >- cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = obj; > > if (!obj) > > return NULL; > > Remove that obj = NULL and just return NULL. This function is gone anyway. I get now assertions in the jsapi-tests. Igor feel free to steal the bug if you know what's going on. Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000 0x000000010015eb89 in JS_Assert (s=0x100221b98 "(addr & GC_CHUNK_MASK) < GC_MARK_BITMAP_ARRAY_OFFSET", file=0x1002219c8 "../jsgc.cpp", ln=417) at ../jsutil.cpp:80 80 *((int *) NULL) = 0; /* To continue from here in GDB: "return" then "continue". */ (gdb) bt #0 0x000000010015eb89 in JS_Assert (s=0x100221b98 "(addr & GC_CHUNK_MASK) < GC_MARK_BITMAP_ARRAY_OFFSET", file=0x1002219c8 "../jsgc.cpp", ln=417) at ../jsutil.cpp:80 #1 0x0000000100088e33 in CheckValidGCThingPtr (thing=0x7fff5fbfda90) at ../jsgc.cpp:417 #2 0x0000000100088eff in JSGCArena::fromGCThing (thing=0x7fff5fbfda90) at ../jsgc.cpp:438 #3 0x000000010007ecf3 in js_GetGCThingRuntime (thing=0x7fff5fbfda90) at ../jsgc.cpp:901 #4 0x000000010013c254 in js_GetStringBytes (cx=0x0, str=0x7fff5fbfda90) at ../jsstr.cpp:4144 #5 0x0000000100010c60 in JS_GetStringBytes (str=0x7fff5fbfda90) at ../jsapi.cpp:5092

yeah the pitfalls of saving some lines :) - if (str->isAtomized()) { - cx->weakRoots.lastAtom = *atomp = STRING_TO_ATOM(str); + if (str->isAtomized()) return true; - }

Comment on attachment 467094 [details] [diff] [review] patch >diff -r 61e1128fb57b js/src/jsinterp.cpp > BEGIN_CASE(JSOP_ENDINIT) > { > /* Re-set the newborn root to the top of this object tree. */ > JS_ASSERT(regs.sp - fp->base() >= 1); > const Value &lref = regs.sp[-1]; > JS_ASSERT(lref.isObject()); >- cx->weakRoots.finalizableNewborns[FINALIZE_OBJECT] = &lref.toObject(); > } Nit: replace the comment with FIXME pointing to the bug 588522 and remove lref local using regs.sp[-1] in the assertion directly.