Stop abusing WrapNativeParent in WrapCallThisObject/WrapCallThisValue

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

Trunk
mozilla40
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments)

Once bug 1146234 lands (or in lieu of it, as preferred), we should consider doing the following:

1) Moving WrapCallThisObject/WrapCallThisValue to a different header that
   includes CallbackObject.h (because we don't want to include it in
   BindingUtils.h).
2) Make it use ToJSValue instead of WrapNativeParent.

We'd need to add a ToJSValue specialization for Rooted<JSObject*>, but that should be easy enough.  And we could remove most of the special-casing in WrapCallThisObject/Value.  Specifically, using ToJSValue would automatically handle the case of the thisval being a callback itself correctly, and automatically handle the Value/Object overloads.  In fact, we may be able to eliminate WrapCallThisObject/Value altogether and just use ToJSValue here.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8584914 [details] [diff] [review]
Get rid of WrapCallThisValue and just use ToJSValue, now that we have it

Review of attachment 8584914 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8584914 - Flags: review?(peterv) → review+
Bug 1144397, bug 1146333, and bug 1148973 backed out for widespread test bustage:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=501a8f687a91
(the Linux opt build failures were from glandium's prior push, the rest is bz)

   https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdd3bb8d11c
Flags: needinfo?(bzbarsky)
Yeah, I thought I'd run this patch through try but apparently not.

Found two problems so far in the test failures:

1)  There was no Rooted<JSObject*> specialization for ToJSValue, and the failure mode is that the type gets treated as a boolean (ouch).  This has been a long-standing footgun, and I propose to fix it so it stops hitting us.  Plus, of course, adding the Rooted<JSObject*> specialization.

2)  The nsISupports wrapping in ToJSValue was mainthread-only.  But event handlers have an nsISupports mTarget that they try to use as the this.  So we need to change the nsISupports bits to take a codepath that works on workers as well.
Flags: needinfo?(bzbarsky)
Comment on attachment 8586140 [details] [diff] [review]
This seems to fix the orange

Review of attachment 8586140 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I missed the JS::Rooted<JSObject*> thing last time.

::: dom/bindings/ToJSValue.h
@@ +193,5 @@
>  {
>    // Make sure we're called in a compartment
>    MOZ_ASSERT(JS::CurrentGlobalOrNull(aCx));
>  
> +  qsObjectHelper helper(ToSupports(&aArgument), nullptr);

I think that technically you don't need the ToSupports.
Attachment #8586140 - Flags: review?(peterv) → review+
> I think that technically you don't need the ToSupports.

I think I do if T multiply-inherits from nsISupports.
https://hg.mozilla.org/mozilla-central/rev/266e68d9b1ce
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.