Closed Bug 976305 Opened 6 years ago Closed 6 years ago

Use the callee, not the this value, for the GlobalObject for WebIDL static methods/attributes

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Per spec, the this object for these is generally ignored.  We should be using the callee instead to create the GlobalObject, since that's something sensible to do.  In particular, that lets the C++ figure out what the callee compartment was if it wants to.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Whiteboard: [need review]
Comment on attachment 8380935 [details] [diff] [review]
Use the callee, not the irrelevant this value, to figure out the GlobalObject for a static WebIDL method.

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

::: dom/bindings/Codegen.py
@@ +5938,5 @@
>          CGAbstractStaticMethod.__init__(self, descriptor, name, "bool", args)
>  
>      def definition_body(self):
>          unwrap = CGGeneric("""JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
> +JS::Rooted<JSObject*> obj(cx, &args.callee());""")

It would be awesome if we could call this |global|. Please do that if it isn't too much work, and don't worry about it if it is.
Attachment #8380935 - Flags: review?(bobbyholley) → review+
That would require allowing CGMethodCall to accept variable scope arg names, etc.  Definitely a whole bunch of work.

And |global| isn't even quite right, since this is the callee, not the callee's global...
https://hg.mozilla.org/integration/mozilla-inbound/rev/c35458a437cc
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla30
And backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/104e8ce657cd because it makes dom/datastore/tests/test_readonly.html time out somehow.
Whiteboard: [leave open]
Target Milestone: mozilla30 → ---
OK, this is failing because DataStore.jsm has this code:

46 function throwReadOnly(aWindow) {
47   return aWindow.Promise.reject(
48     new aWindow.DOMError("ReadOnlyError", "DataStore in readonly mode"));
49 }

Here aWindow.Promise is an Xray.  What used to happen is that we landed in the static reject() method, took the "this" object (that being the Xray), unwrapped it, and used the global of the resulting object to create the Promise wrapper in.  So this created a Promise in the content scope.

But with this patch we use the callee.  The callee is just a Function, not an Xray, and it's a function in the chrome global.  So we create the Promise wrapper in the chrome global, then hand it to content, get a COW, and content can't do anything useful with the object.  The test times out because it doesn't treat uncaught exceptions inside dom/datastore/tests/file_readonly.html as fatal end-the-test things, which is probably a bug in the test.

In any case, we need to figure out what we want to do here for the Xray case...  Maybe we need to hold off on this until we have Xrays to functions or something.  :(
Flags: needinfo?(bobbyholley)
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #6)
> OK, this is failing because DataStore.jsm has this code:
> 
> 46 function throwReadOnly(aWindow) {
> 47   return aWindow.Promise.reject(
> 48     new aWindow.DOMError("ReadOnlyError", "DataStore in readonly mode"));
> 49 }

Oh hm, that's an interesting problem. 

> In any case, we need to figure out what we want to do here for the Xray
> case...  Maybe we need to hold off on this until we have Xrays to functions
> or something.  :(

Xrays to Functions won't help, because it's unlikely that we'll ever have an identity mapping between xray.fooMethod and xray.wrappedJSObject.fooMethod. It's just too difficult to compute the former from the latter at wrap time.

We could, however, easily check whether a given callee JSFunction comes from an Xray, by determining whether an Xray function is parented to an XrayWrapper or XrayHolder (we currently have some sloppy combination of both, and would need to pick one). If so, we could unwrap it in the binding layer to get the global. Thoughts Boris?
Flags: needinfo?(bobbyholley)
Hmm.  Are we guaranteed that the parent of an Xray function is going to be some random non-global object?  I thought we were going to remove function parents.
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #8)
> Hmm.  Are we guaranteed that the parent of an Xray function is going to be
> some random non-global object?

At the moment it's kind of a smorgasbord of things, but I could pretty easily write a patch to make that guarantee, I think.

> I thought we were going to remove function parents.

When we do, we can just switch to a reserved slot on the Function.
OK.  That seems reasonable to me.  So xray getters/setters/methods will be able to reach the Xray itself, and then unwrap it to get the "real" callee global.

Want to write up the prereqs for that?
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #10)
> OK.  That seems reasonable to me.  So xray getters/setters/methods will be
> able to reach the Xray itself, and then unwrap it to get the "real" callee
> global.

Yeah. Note that once peter lands bug 787070, the functions will live on the Xray for the prototype, not the instance. But unwrapping the parent should still get you to the right global.
 
> Want to write up the prereqs for that?

Sure. Can you file a blocking bug and assign it to me? I'll try to get to it in the next day or two.
> But unwrapping the parent should still get you to the right global.

As long as the xray consumer hasn't messed with the proto chains of the xrays, right?
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #12)
> > But unwrapping the parent should still get you to the right global.
> 
> As long as the xray consumer hasn't messed with the proto chains of the
> xrays, right?

Well, it shouldn't matter, really. The point is that the Xray methods will know which global they were created for (by virtue of which Xrayed prototype or constructor they live in). Someone can certainly mess with the prototype of an individual wrapper. But we'll still get the semantics right. Setting xray.__proto__ = someOtherXrayToSomeOtherScope.__proto__ will cause you to inherit methods whose [[Global]] is computed as someOtherScope.
Depends on: 976938
Whiteboard: [leave open]
Whiteboard: [need review]
Blocks: 983300
Attachment #8390282 - Flags: review?(bobbyholley) → review+
Attachment #8380935 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/164a75f89456
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.