jsstr.cpp is in my sights.

[Sorry, I forgot the patch description.] > Function 'jsstr.cpp:uint8 FindReplaceLength(JSContext*, js::RegExpStatics*, ReplaceData*, uint64*)' has unrooted 'lambda' live across GC call 'uint8 js::ContextStack::pushInvokeArgs(JSContext*, uint32, js::InvokeArgsGuard*, uint32)' at js/src/jsstr.cpp:2066 Easy fix, done. > Function 'jsstr.cpp:uint8 FindReplaceLength(JSContext*, js::RegExpStatics*, ReplaceData*, uint64*)' has unrooted 'staticsGuard' live across GC call 'uint8 js::ContextStack::pushInvokeArgs(JSContext*, uint32, js::InvokeArgsGuard*, uint32)' at js/src/jsstr.cpp:2066 I don't understand this one. > Function 'jsstr.cpp:uint8 str_replace_flat_lambda(JSContext*, JS::CallArgs, ReplaceData*, FlatMatch*)' has unrooted 'matchStr' live across GC call 'uint8 js::ContextStack::pushInvokeArgs(JSContext*, uint32, js::InvokeArgsGuard*, uint32)' at js/src/jsstr.cpp:2519 Easy fix, done. > Function 'int32 js::str_split(JSContext*, uint32, JS::Value*)' has unrooted 'sepstr' live across GC call 'uint8 js::RegExpToShared(JSContext*, JSObject*, js::RegExpGuard*)' at js/src/jsstr.cpp:2954 Easy fix, done. > Function 'jsstr.cpp:int32 str_decodeURI_Component(JSContext*, uint32, JS::Value*)' has unrooted 'result' live across GC call 'jsstr.cpp:int32 Decode(JSContext*, JSString*, uint16*, JS::Value*)' at js/src/jsstr.cpp:4299 I think this is a false positive -- result isn't set before the call to Decode, but Decode() itself sets it. I could just root it anyway? Alternatively, Decode only GCs when it issues an error message, so I could just suppress GC for that. > Function 'jsstr.cpp:int32 str_encodeURI(JSContext*, uint32, JS::Value*)' has unrooted 'result' live across GC call 'jsstr.cpp:int32 Encode(JSContext*, JSString*, uint16*, uint16*, JS::Value*)' at js/src/jsstr.cpp:4315 Ditto. > Function 'jsstr.cpp:int32 str_encodeURI_Component(JSContext*, uint32, JS::Value*)' has unrooted 'result' live across GC call 'jsstr.cpp:int32 Encode(JSContext*, JSString*, uint16*, uint16*, JS::Value*)' at js/src/jsstr.cpp:4331 Ditto. > Function 'JSString* js::ValueToSource(JSContext*, JS::Value*)' has unrooted 'rval' live across GC call 'int32 js::GetMethod(JSContext*, class JS::Handle<JSObject*>, JSHandleId, uint32, class JS::MutableHandle<JS::Value>)' at js/src/jsstr.cpp:3703 Ditto.

This patch fixes this hazard: > Function 'JSString* js::ToStringSlow(JSContext*, JS::Value*) [with js::AllowGC allowGC = (js::AllowGC)1u; JSContext = JSContext]' has unrooted 'v' live across GC call 'jsobjinlines.h:uint8 js::ToPrimitive(JSContext*, uint32, JS::Value*)' at js/src/jsstr.cpp:3651 I had to change ToPrimitive to take a MutableHandleValue argument. Please check the following parts closely. - The |args0| case in jsdate.cpp. - |v2| in jsnum.cpp (esp. w.r.t. the SkipRoots at the top of ToNumberSlow) - |tmp| in DirectProxyHandler::defaultValue() - All the fromMarkedLocation() examples in JaegerMonkey.

Comment on attachment 711151 [details] [diff] [review] (part 1) - Fix rooting hazards in jsstr.cpp identified by static analysis. Review of attachment 711151 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, apparently Ms2ger got here first. 2 of the 3 changes in this patch just landed this morning.

Comment on attachment 711173 [details] [diff] [review] (part 2) - Fix rooting hazards in jsstr.cpp identified by static analysis. Review of attachment 711173 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdate.cpp @@ +3057,2 @@ > return false; > + args[0] = args0; args.handleAt(0), maybe ::: js/src/jsobjinlines.h @@ +1471,1 @@ > return true; Could be "return JSObject::..." @@ +1484,1 @@ > return true; Same here

Comment on attachment 711173 [details] [diff] [review] (part 2) - Fix rooting hazards in jsstr.cpp identified by static analysis. Review of attachment 711173 [details] [diff] [review]: ----------------------------------------------------------------- I want to re-review whatever you do for the ToStringSlow case. Thanks! ::: js/src/jsdate.cpp @@ +3057,2 @@ > return false; > + args[0] = args0; What Ms2ger said (args.handleAt(0)), even though it gives me the creeps since I don't think it's currently safe to assume that vp points to rooted memory. (It usually does, but sometimes the caller is constructing a bogus chunk of Values. Maybe we should require them to root those?) ::: js/src/jsobjinlines.h @@ +1465,5 @@ > } > } > > RootedObject objRoot(cx, obj); > + if (!JSObject::defaultValue(cx, objRoot, JSTYPE_VOID, vp)) just return this now @@ +1480,3 @@ > return true; > + RootedObject obj(cx, &vp.toObject()); > + if (!JSObject::defaultValue(cx, obj, preferredType, vp)) just return this now ::: js/src/jsstr.cpp @@ +3644,5 @@ > { > /* As with ToObjectSlow, callers must verify that |arg| isn't a string. */ > JS_ASSERT(!arg.isString()); > > + RootedValue v(cx, arg); Sadly, I don't think we want this root when allowGC is false. It's only a tiny bit of perf loss, but it seems likely to trip up an analysis where we check that we're not rooting in allowGC=false situations. It only needs to be rooted for ToPrimitive, right? So maybe this could be: Value v = arg; if (!arg.isPrimitive()) { ... RootedValue root(cx, arg) ToPrimitive(&root); v = root; } ?

(In reply to Steve Fink [:sfink] from comment #6) > ::: js/src/jsdate.cpp > @@ +3057,2 @@ > > return false; > > + args[0] = args0; > > What Ms2ger said (args.handleAt(0)), even though it gives me the creeps > since I don't think it's currently safe to assume that vp points to rooted > memory. (It usually does, but sometimes the caller is constructing a bogus > chunk of Values. Maybe we should require them to root those?) The caller should be required to root all values that are passed to Invoke or to Natives. I don't known if all callers currently have this property though.

Is this what you wanted with the handleAt()?

Comment on attachment 711661 [details] [diff] [review] (part 2) - Fix rooting hazards in jsstr.cpp identified by static analysis. Review of attachment 711661 [details] [diff] [review]: ----------------------------------------------------------------- Yes! Just so. Thanks.

Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6da9f02606

https://hg.mozilla.org/mozilla-central/rev/d87f3b860eaa https://hg.mozilla.org/mozilla-central/rev/2c6da9f02606