Terrence, just want you for a sanity-check on the fromMarkedLocation bit

Try push at https://tbpl.mozilla.org/?tree=Try&rev=864ba4134be8 is showing just the expected orange from the patch for bug 860841.

Comment on attachment 740968 [details] [diff] [review] part 1. Root the global before calling WrapObject in XPCConvert. sort of wish GetGlobalJSObject just returned a handle. Review of attachment 740968 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCConvert.cpp @@ +838,5 @@ > if (!ccx.IsValid()) > return false; > > if (!flat) { > + JS::Rooted<JSObject*> global(lccx.GetJSContext(), Just pass ccx here and below. We've already unlazified it, and it'll auto-convert to JSContext*. @@ +840,5 @@ > > if (!flat) { > + JS::Rooted<JSObject*> global(lccx.GetJSContext(), > + xpcscope->GetGlobalJSObject()); > + flat = cache->WrapObject(lccx.GetJSContext(), global); Unrelated to your changes but when the heck do we have something where cache->IsDOMBinding() is true, but we don't want have anything in the cache? And why do we want to return the global in that case? Is this just belt-and-suspenders? If so, seems like we should just assert instead...

(In reply to Bobby Holley (:bholley) from comment #10) > Unrelated to your changes but when the heck do we have something where > cache->IsDOMBinding() is true, but we don't want have anything in the cache? > And why do we want to return the global in that case? Is this just > belt-and-suspenders? If so, seems like we should just assert instead... Per IRC, I was just confused here. WrapObject isn't wrapping the global. It's wrapping the binding object in the scope of the global. Nevermind me.

> Just pass ccx here and below. Looks like we have a "cx" around as well; I'll just pass that. ;)

Comment on attachment 740969 [details] [diff] [review] part 2. Pass a handle for the scope object to union conversions. Review of attachment 740969 [details] [diff] [review]: ----------------------------------------------------------------- I just noticed that the review combobox says "The patch has passed review by a module owner or peer." on hover... I guess my review will do, though.

Comment on attachment 740971 [details] [diff] [review] part 3. Pass a handle for the parent object to WebIDL dictionary ToObject. Review of attachment 740971 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +3643,5 @@ > sequenceWrapLevel -= 1 > innerTemplate = CGIndenter(CGGeneric(innerTemplate), 6).define() > return ((""" > uint32_t length = %s.Length(); > +JS::Rooted<JSObject*> returnArray(cx,JS_NewArrayObject(cx, length, NULL)); Space after comma. Feel free to make it nullptr, too.

> Space after comma. Feel free to make it nullptr, too. Done.

Comment on attachment 740972 [details] [diff] [review] part 4. Pass a handle for the scope object to all the various Wrap*Object stuff in BindingUtils. The JS::Rooted in CGWrapWithCacheMethod is just there until we start passing a handle to Wrap(). Review of attachment 740972 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextUtils.h @@ +25,2 @@ > JSAutoCompartment ac(cx, wrapper); > + if (!dom::WrapNewBindingObject(cx, wrapper, const_cast<WebGLObjectType*>(object), v.address())) { The vp argument to WrapNewBindingObject should probably turn into a MutableHandle<Value> at some point. Followup? ::: dom/bindings/Codegen.py @@ +2064,5 @@ > else: > assertISupportsInheritance = "" > return """%s > %s > + JS::Rooted<JSObject*> aScope(aCx, aScopeArg); // Temporary! Oh, good, I would have complained about the name otherwise :) @@ +8566,5 @@ > > bodyWithThis = string.Template( > setupCall+ > + "JSObject* thisObjJS =\n" > + " WrapCallThisObject(s.GetContext(), JS::Handle<JSObject*>::fromMarkedLocation(&mCallback), thisObj);\n" Is this the same mCallback as a few patches back? If so, please add a getter instead of doing this inline everywhere.

Comment on attachment 740973 [details] [diff] [review] part 5. Make all the WrapObject methods take a handle for the scope object. Review of attachment 740973 [details] [diff] [review]: ----------------------------------------------------------------- Only skimmed, didn't see anything.

Comment on attachment 740975 [details] [diff] [review] part 6. Make all the WrapNode methods take a handle for the scope object. Review of attachment 740975 [details] [diff] [review]: ----------------------------------------------------------------- Same

> The vp argument to WrapNewBindingObject should probably turn into a MutableHandle<Value> > at some point. Followup? It probably should, but it's a somewhat low priority for now, especially because it would be blocked on CallArgs-ification of the specialized natives. > Is this the same mCallback as a few patches back? Yes, it is. I guess I can change CallbackPreserveColor to return a Handle. Let me do that.

Actually, those consumers should be using Callback(), not CallbackPreserveColor() anyway. I made both return handles in any case.

> Actually, those consumers should be using Callback() And actually actually, they should not, since CallSetup handles the unmark-gray.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a67479723670 https://hg.mozilla.org/integration/mozilla-inbound/rev/27c89b2200ad https://hg.mozilla.org/integration/mozilla-inbound/rev/d13e71853e55 https://hg.mozilla.org/integration/mozilla-inbound/rev/3011f288bfe7 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b1381894c44 https://hg.mozilla.org/integration/mozilla-inbound/rev/e98c80465618 https://hg.mozilla.org/integration/mozilla-inbound/rev/74bd92b1c033

And http://hg.mozilla.org/integration/mozilla-inbound/rev/ac39efa583f7 to fix build bustage.

https://hg.mozilla.org/mozilla-central/rev/a67479723670 https://hg.mozilla.org/mozilla-central/rev/27c89b2200ad https://hg.mozilla.org/mozilla-central/rev/d13e71853e55 https://hg.mozilla.org/mozilla-central/rev/3011f288bfe7 https://hg.mozilla.org/mozilla-central/rev/1b1381894c44 https://hg.mozilla.org/mozilla-central/rev/e98c80465618 https://hg.mozilla.org/mozilla-central/rev/74bd92b1c033 https://hg.mozilla.org/mozilla-central/rev/ac39efa583f7