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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Use the callee, not the irrelevant this value, to figure out the GlobalObject for a static WebIDL method.

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c35458a437cc

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Bobby Holley (:bholley)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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?

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

Filed bug 976938.

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Use the callee, not the irrelevant this value, to figure out the GlobalObject for a static WebIDL method.

Updated patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/164a75f89456

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/164a75f89456