I'll do a bunch of small patches in this bug.

This one hazard showed up nine times in the analysis, due to different template instantiations.

I could have instead rooted |script| at the top, but I figured this function is called a lot.

We're still using static methods to avoid rooting |this|, right? We still have at least 40 occurrences of rooted |this| in the code...

Comment on attachment 711692 [details] [diff] [review] (part 3) - Some low-hanging exact rooting fruit. Review of attachment 711692 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/ParallelArray.cpp @@ +1423,4 @@ > if (args.length() >= 2) > defaultValue = args[1]; > else > defaultValue.setUndefined(); This could be RootedValue defaultValue(cx, args.get(1));

Comment on attachment 711686 [details] [diff] [review] (part 3) - Exactly root PropertyAccess(). Review of attachment 711686 [details] [diff] [review]: ----------------------------------------------------------------- Is the static analysis reporting hazards somewhere around here? PropertyAccess (and almost everything in jsinfer.cpp) is called with GC suppressed and does not need to be rooted.

> Is the static analysis reporting hazards somewhere around here? > PropertyAccess (and almost everything in jsinfer.cpp) is called with GC > suppressed and does not need to be rooted. It was this one: > Function 'jsinfer.cpp:void PropertyAccess(JSContext*, JSScript*, uint8*, js::types::TypeObject*, js::types::StackTypeSet*, jsid) [with PropertyAccessKind access = (PropertyAccessKind)2u; JSContext = JSContext; jsbytecode = unsigned char; js::RawId = jsid]' has unrooted 'script' live across GC call 'void js::types::TypeSet::addType(JSContext*, js::types::Type)' at js/src/jsinfer.cpp:1169 It occurs within PropertyAccess, which is listed as a CanGC function.

Hmm, looking at http://people.mozilla.org/~sfink/data/gcFunctions.txt I see 3173 functions that can GC and 476 suppressed functions. On my machine, with a slightly outdated tree, I get 4242 functions that can GC and 7745 suppressed functions. The latter numbers are much closer to being correct. I'll do a clean run off inbound tip to try to narrow down what's going on. For now I would just ignore anything in jsinfer, Ion and methodjit, as these are where most of those suppressed functions are going to be.

Comment on attachment 711692 [details] [diff] [review] (part 3) - Some low-hanging exact rooting fruit. Review of attachment 711692 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. ::: js/src/builtin/Eval.cpp @@ +163,5 @@ > JS_ASSERT_IF(evalType == INDIRECT_EVAL, scopeobj->isGlobal()); > AssertInnerizedScopeChain(cx, *scopeobj); > > + Rooted<GlobalObject*> scopeObjGlobal(cx, &scopeobj->global()); > + if (!GlobalObject::isRuntimeCodeGenEnabled(cx, scopeObjGlobal)) { Hm... you could probably use fromMarkedLocation here (scopeobj is rooted, and you're just temporarily using the global), but who cares? I don't think this is very hot, and your way is simpler.

Oh, and my gcFunctions.txt should be fixed now. It was mainly a problem of bad error handling.

Parts 1, 2 and 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/29d3b2c3d8b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b3c6d4ee0ae https://hg.mozilla.org/integration/mozilla-inbound/rev/c4562906fc61

The one is GlobalObject.cpp is this: > Function 'uint8 js::GlobalObject::isRuntimeCodeGenEnabled(JSContext*, class JS::Handle<js::GlobalObject*>)' has unrooted '__temp_4' live across GC call *allows at js/src/vm/GlobalObject.cpp:494 I just moved the BooleanValue() call out of line. Not sure if that'll fix it, but at the very least it should make the hazard message easier to understand. (That explains the "four or five" in the patch description.)

Comment on attachment 712744 [details] [diff] [review] (part 4) - Fix four or five more rooting hazards. Review of attachment 712744 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/GlobalObject.cpp @@ +491,5 @@ > * and that it permits runtime code generation, then cache the result. > */ > JSCSPEvalChecker allows = cx->runtime->securityCallbacks->contentSecurityPolicyAllows; > + Value boolValue = BooleanValue(!allows || allows(cx)); > + v.set(global, HeapSlot::Slot, RUNTIME_CODEGEN_ENABLED, boolValue); Hm. I can't decide whether the hazard was real or not, but we definitely want your change. The issue is whether the compiler could put some sort of GC-movable thing from invoking the first half of v.set() onto the stack, then call allows(cx) which triggers a moving GC, and then write into the thing stored on the stack. I don't see how that could happen in this case, but it smells unsafe. And thinking about it makes my head hurt. Your change adds a sequence point between allows(cx) and the v.set call, so nothing fishy can happen.

This fixes two hazard reports (two instances of the same thing, AFAICT) in json.cpp.

Part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0846d180af

Comment on attachment 712792 [details] [diff] [review] (part 6) - Fix five more easy rooting hazards. Review of attachment 712792 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/Eval.cpp @@ +30,2 @@ > if (JSObjectOp op = o->getClass()->ext.innerObject) { > Rooted<JSObject*> obj(cx, o); Still need this?

https://hg.mozilla.org/mozilla-central/rev/29d3b2c3d8b3 https://hg.mozilla.org/mozilla-central/rev/1b3c6d4ee0ae https://hg.mozilla.org/mozilla-central/rev/c4562906fc61 https://hg.mozilla.org/mozilla-central/rev/9a0846d180af

Comment on attachment 712768 [details] [diff] [review] (part 5) - Make ObjectClassIs take a HandleObject. Review of attachment 712768 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 712792 [details] [diff] [review] (part 6) - Fix five more easy rooting hazards. Review of attachment 712792 [details] [diff] [review]: ----------------------------------------------------------------- r+ with Ms2ger's comment taken into account.

Comment on attachment 712792 [details] [diff] [review] (part 6) - Fix five more easy rooting hazards. Review of attachment 712792 [details] [diff] [review]: ----------------------------------------------------------------- r+ with Ms2ger's comment taken into account.

Comment on attachment 712796 [details] [diff] [review] (part 7) - Fix seven more easy rooting hazards. Review of attachment 712796 [details] [diff] [review]: ----------------------------------------------------------------- Awesome progress! ::: js/src/jsobj.cpp @@ +2903,5 @@ > objp.set(NULL); > return true; > } > > + RootedObject cobj(cx); I'd prefer this to be explicitly NULL to show that its initial value is meaningful. RootedObject cobx(cx, NULL); ::: js/src/vm/ObjectImpl.cpp @@ +685,5 @@ > NEW_OBJECT_REPRESENTATION_ONLY(); > > Rooted<ObjectImpl*> current(cx, obj); > > + RootedValue get(cx); I know this is pre-existing, but this sounds funny. Could it be 'getter' instead of 'get'? I didn't read the code to see, but I notice that 'setter' is used later.

https://hg.mozilla.org/integration/mozilla-inbound/rev/560ede42c3f6 https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8f0f701666 https://hg.mozilla.org/integration/mozilla-inbound/rev/e650f1bab42b

I made JSObject::getType() self-root because (a) it's a case where it's really easy to see that we don't use |this| again, and (b) getType() is called in lots of places.

https://hg.mozilla.org/mozilla-central/rev/560ede42c3f6 https://hg.mozilla.org/mozilla-central/rev/ba8f0f701666 https://hg.mozilla.org/mozilla-central/rev/e650f1bab42b

Comment on attachment 713264 [details] [diff] [review] (part 8) - Fix another seven rooting hazards. Review of attachment 713264 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I like the getType() part. ::: js/src/jsobj.h @@ +284,5 @@ > friend class js::NewObjectCache; > > /* Make the type object to use for LAZY_TYPE objects. */ > + static js::types::TypeObject *makeLazyType(JSContext *cx, > + js::HandleObject obj); Why is this wrapped? Surely that fits in 99 columns?

Part 8: https://hg.mozilla.org/integration/mozilla-inbound/rev/d771cec55373

The AutoSuppressGC in RenewProxyObject is ugly, but I didn't want to root |obj| and |priv| just for DEBUG builds, especially when isOuterWindow() almost certainly cannot GC in practice (it's marked as a GC function only because it's virtual).

I think the first one is a false positive due to the |RootedFunction fun| shadowing a |JSFunction *fun| parameter. Hopefully changing its name to |f| will fix it.

Comment on attachment 713795 [details] [diff] [review] (part 11) - Fix two more easy rooting hazards. Review of attachment 713795 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdbgapi.cpp @@ +511,5 @@ > JS_GetFunctionScript(JSContext *cx, JSFunction *fun) > { > if (fun->isNative()) > return NULL; > + RootedScript script(cx); Uhg. This one is actually a performance concern. Please move the Rooted variant below isInterpretedLazy().

Comment on attachment 713796 [details] [diff] [review] (part 12) - Fix two easy rooting hazards in shell/js.cpp. Review of attachment 713796 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/shell/js.cpp @@ +1873,2 @@ > RootedScript script(cx); > + JSFunction::maybeGetOrCreateScript(cx, f, &script); Why the name change? |f| seems worse in almost every way than |fun|.

Comment on attachment 713801 [details] [diff] [review] (part 13) - Fix ten easy rooting hazards in vm/Debugger.cpp. Review of attachment 713801 [details] [diff] [review]: ----------------------------------------------------------------- Weird. I'd expect that Invoke would set those after all GCs, but I guess even one counterexample would ruin it.

(In reply to Terrence Cole [:terrence] from comment #36) > ::: js/src/shell/js.cpp > @@ +1873,2 @@ > > RootedScript script(cx); > > + JSFunction::maybeGetOrCreateScript(cx, f, &script); > > Why the name change? |f| seems worse in almost every way than |fun|. Nevermind, my question is already answered in comment 32.

Parts 9--13: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b029c64d2a https://hg.mozilla.org/integration/mozilla-inbound/rev/e2843bae075f https://hg.mozilla.org/integration/mozilla-inbound/rev/fb11273bd848 https://hg.mozilla.org/integration/mozilla-inbound/rev/a3697783abdf https://hg.mozilla.org/integration/mozilla-inbound/rev/d30c69e19fa0

https://hg.mozilla.org/mozilla-central/rev/e7b029c64d2a https://hg.mozilla.org/mozilla-central/rev/e2843bae075f https://hg.mozilla.org/mozilla-central/rev/fb11273bd848 https://hg.mozilla.org/mozilla-central/rev/a3697783abdf https://hg.mozilla.org/mozilla-central/rev/d30c69e19fa0

I just made ToStringHelper::mStr a RootedString. I think this is safe because ToStringHelper and IdStringifier (a sub-class) objects are only used on the stack.

Comment on attachment 718819 [details] [diff] [review] (part 14) - More exact rooting in shell/js.cpp. Review of attachment 718819 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/shell/js.cpp @@ +1909,5 @@ > Sprinter sprinter(cx); > if (!sprinter.init()) > return false; > + RootedFunction nullFun(cx); > + bool ok = DisassembleScript(cx, script, nullFun, p.lines, p.recursive, &sprinter); Can't you use NullPtr() instead of nullFun?

Part 14: https://hg.mozilla.org/integration/mozilla-inbound/rev/6cddf5c045d5

And exact rooting is (finally) done.