864727 - Root WebIDL binding Wrap() methods

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Root the global before calling WrapObject in XPCConvert. sort of wish GetGlobalJSObject just returned a handle.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Pass a handle for the scope object to union conversions.

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Pass a handle for the parent object to WebIDL dictionary ToObject.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: 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().

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Make all the WrapObject methods take a handle for the scope object.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Make all the WrapNode methods take a handle for the scope object.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Make the WebIDL binding Wrap methods take a handle for the scope object and use a Rooted for the parent.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Perl regular expressions I used to generate most of the WrapObject and WrapNode patches

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Bobby Holley (:bholley)]

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...

Comment 11

[Bobby Holley (:bholley)]

(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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Just pass ccx here and below.

Looks like we have a "cx" around as well; I'll just pass that.  ;)

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Done.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Actually, those consumers should be using Callback()

And actually actually, they should not, since CallSetup handles the unmark-gray.

Comment 22

[Boris Zbarsky [:bzbarsky]]

   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

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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