933681 - Resolve standard JS classes on Xrayed globals

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

The basic idea is that you should be able to do |new contentWindow.Int32Array()| and get a typed array in the content scope, without ever having to waive Xrays.

I originally wanted to block this work on bug 914970, so that we could have real Xrays both to the constructor itself, and to the object that was created. However, khuey needs this work now for his ObjectWrapper.jsm changes for NFC (bug 933497), and I think we can still make a limited subset of functionality work properly. In particular, [[Call]] and [[Construct]] hooks cannot be redefined by content, so as long as we return the real content constructor, the caller can safely invoke it (though it can't safely do much else).

In the JS engine today, we have a table that maps standard property ids to functions that create the constructor, define it on the global, and stash it in a lot on the global for good measure. There are a lot of these functions, so it would be nice to avoid having to modify their semantics. For our purposes, we want to make sure that the constructor is set up and accessible via the slot. Once it is, implementing this is a simple matter of returning the contents of the proper slot on the global.

Currently, we usually do lazy resolution for standard class names (though not always - see bug 919095). If we continue with this, we may need to perform the resolve for a given global property being resolved over Xrays that the target hasn't resolved itself yet. This modifies the target JS object, which isn't really kosher per Xray semantics, but if it's undetectable by script, it's probably ok. So my two questions are:
* Is there any way for content to define a property with a standard name without triggering the resolve hook, such that running the resolve hook would result in redefining the property?
* If not, is there any way for content to detect whether a given property has been resolved without triggering the resolve hook in the process?

If the answer to both of those is 'no', then I think we can simply do the following:
(1) Look up the property in the table of standard property names. If there's no match, we're done.
(2) If there is a match, figure out the appropriate slot on the global, and see if the property has been resolved.
(3) If it hasn't been resolved, invoke the resolve hook.
(4) Return the contents of the slot to the caller.

The other option, of course, is to switch to eager resolution, which might make the above moot.

Any other thoughts or things that I'm missing?

Comment 1

[Till Schneidereit [:till]]

> The other option, of course, is to switch to eager resolution, which might make the above moot.

Given that we accidentally switched to eager resolution in (from what I can tell) almost all or all cases in the browser a year or longer ago, I think we should just rip out all the lazy resolution stuff.

I *also* think that the answer to both of your questions is indeed "no", but I'm not entirely sure.

Comment 2

[Bobby Holley (:bholley)]

Till and I decided to try to just flip on eager resolution, which he's going to take a crack at today. Tentatively blocking on bug 919095. If we succeed there, the patch here should be a fair bit simpler.

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to Bobby Holley (:bholley) from comment #0)
> Currently, we usually do lazy resolution for standard class names (though
> not always - see bug 919095). If we continue with this, we may need to
> perform the resolve for a given global property being resolved over Xrays
> that the target hasn't resolved itself yet. This modifies the target JS
> object, which isn't really kosher per Xray semantics, but if it's
> undetectable by script, it's probably ok.

It is definitely ok.

I was under the impression that lazily resolving globals and XPConnect prototype methods/accessors saved a huge amount of memory. Am I wrong?

> So my two questions are:
> * Is there any way for content to define a property with a standard name
> without triggering the resolve hook, such that running the resolve hook
> would result in redefining the property?

No.

> * If not, is there any way for content to detect whether a given property
> has been resolved without triggering the resolve hook in the process?

No.

It may surprise you to hear that the resolve hook was originally intended as a pure optimization. Wherever content can detect that it's observably different from eager definition, that is a bug.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So what's the status here Bobby?  The NFC folks want bug 933497 soon.

Comment 5

[Bobby Holley (:bholley)]

(I'm actively working on this)

Comment 6

[Bobby Holley (:bholley)]

I have this working locally and passing XPConnect tests. Let's see what try says:

https://tbpl.mozilla.org/?tree=Try&rev=f3c7450bd6db

Comment 7

[Bobby Holley (:bholley)]

Green modulo one test that I fixed. Uploading patches and flagging for review.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Always stash resolved constructors in global slots, and kill markStandardClassInitializedNoProto. v1

This is necessary to see this stuff over Xrays.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 2 - Factor JSStdName table iteration into a helper function. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Define JSStdName tables in terms of jsprototypes.h. v1

This makes sure everything is consistent, and lets us use JSProtoKeys to index
into the JSStdNames table.

This basically requires a wide terminal to look at jsprototypes.h,
unfortunately. :-(

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 4 - Rename the tables in jsapi.cpp to something that makes sense. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 5 - Introduce an API to map from names to JSProtoKeys. v1

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 6 - Add lookups for standard classes in XrayWrapper. v1

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 7 - Resolve canonical eval() onto Xrayed globals. v1

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 8 - Tests. v1

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Blocks a blocker.

We need this by December 9th, and ideally ASAP.

Comment 17

[Ken Chang[:kenkai]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #16)
> Blocks a blocker.
> 
> We need this by December 9th, and ideally ASAP.
Thanks Kyle. I wonder if it was possible to have it this week.

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 831609 [details] [diff] [review]
Part 1 - Always stash resolved constructors in global slots, and kill markStandardClassInitializedNoProto. v1

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

Sorry for the slow reviews. I'll continue with this tomorrow.

::: js/src/vm/GlobalObject.h
@@ +201,5 @@
>      /*
>       * Lazy standard classes need a way to indicate they have been initialized.
>       * Otherwise, when we delete them, we might accidentally recreate them via
>       * a lazy initialization. We use the presence of a ctor or proto in the
> +     * global object's slot to indicate that they've been constructed.

Thanks for remembering to update the comment. You're the best. A suggested change:

[...] We use the presence of an object in the getConstructor(key) reserved slot
to indicate that they've been initialized.  (A few builtin objects, like
JSON and Math, are not constructors, so getConstructor is a bit of a
misnomer.)

Or that last sentence could go in a new comment on the getConstructor method, whatever looks more helpful to future readers...

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 831611 [details] [diff] [review]
Part 2 - Factor JSStdName table iteration into a helper function. v1

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

::: js/src/jsapi.cpp
@@ +1496,3 @@
>          RootedObject proto(cx);
>          if (!JSObject::getProto(cx, obj, &proto))
>              return false;

This ancient code makes no sense. Please file a follow-up bug to remove object_prototype_names if you haven't done it already. It can be replaced with something like

    if (!obj->as<GlobalObject>().getOrCreateObjectPrototype(cx))
        return false;

Comment 20

[Jason Orendorff [:jorendorff]]

Comment on attachment 831612 [details] [diff] [review]
Part 3 - Define JSStdName tables in terms of jsprototypes.h. v1

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

The wide terminal thing is fine.

::: js/src/jsapi.cpp
@@ +1337,5 @@
>  
>  /*
> + * Table of standard classes, indexed by JSProtoKey. For entries where the
> + * JSProtoKey does not correspond to a class with a meaningful constructor, we
> + * insert a null entry into the table.

Why not just insert nothing into the table?

@@ +1342,3 @@
>   */
> +#define STD_NAME_ENTRY(name, code, init, clasp) { init, EAGER_CLASS_ATOM(name), clasp },
> +#define STD_DUMMY_ENTRY(name, code, init, dummy) { DummyInit, 0, nullptr },

In other words, make STD_DUMMY_ENTRY() expand to no tokens and delete DummyInit and isDummy().

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 831614 [details] [diff] [review]
Part 4 - Rename the tables in jsapi.cpp to something that makes sense. v1

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

::: js/src/jsapi.cpp
@@ +1348,5 @@
>  };
>  
>  /*
> + * Table of top-level function and constant names and the init function of the
> + * corresponding standard class that sets them up.

While you're renaming stuff... :)

I would love for these to be called "builtins" (which they are, and ECMA-262 refers to the standard library bits as "builtins" as well) rather than "standard classes" (some are not classes; others are not standard; at least uneval is neither).

Waldo or I (or whoever) can patch that in our copious free time though.

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 831617 [details] [diff] [review]
Part 5 - Introduce an API to map from names to JSProtoKeys. v1

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

OK.

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 831618 [details] [diff] [review]
Part 6 - Add lookups for standard classes in XrayWrapper. v1

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

Great!

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +816,3 @@
>      }
> +
> +    // Next, check for ES standard classes.

Just generally speaking, does this have to be in the XrayTraits abstract base class? Or can it go in one of the concrete classes (and spare the other)?

@@ +819,5 @@
> +    if (!found && JS_IsGlobalObject(target)) {
> +        JSAutoCompartment ac(cx, target);
> +        JSProtoKey key = JS_IdToProtoKey(cx, id);
> +        if (key != JSProto_Null) {
> +            MOZ_ASSERT(key < JSProto_LIMIT);

Could move the JSAutoCompartment into the inner block.

Comment 24

[Jason Orendorff [:jorendorff]]

Comment on attachment 831619 [details] [diff] [review]
Part 7 - Resolve canonical eval() onto Xrayed globals. v1

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

::: js/src/jsfriendapi.cpp
@@ +538,5 @@
>      return true;
>  }
>  
> +JS_FRIEND_API(bool)
> +js::GetOriginalEval(JSContext *cx, HandleObject scope, MutableHandleObject eval)

Make it a Handle<GlobalObject*>?

Or just rename the argument to HandleObject global, and use global->as<GlobalObject>() rather than scope->global().

One reason is that scope->global() doesn't do what the caller would expect if scope is, say, an outer window, or a CCW for one.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +814,5 @@
>              return false;
>          found = !!desc.object();
>      }
>  
> +    // Next, check for ES standard classes and |eval|.

"builtins"? :) :) :)

Comment 25

[Jason Orendorff [:jorendorff]]

Comment on attachment 831618 [details] [diff] [review]
Part 6 - Add lookups for standard classes in XrayWrapper. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +834,1 @@
>          if (!JS_WrapPropertyDescriptor(cx, desc))

Second thoughts: I don't honestly know what wrapping a JSNative constructor function object actually does. Or the Math or JSON object, or other random JS objects, for that matter.

I'd like to talk it through with you before I stamp this whole stack. I don't think we want iwin.Array.of(obj) to possibly call a content-hacked 'Array.of' function. Likewise iwin.JSON.parse(s) or iwin.Function.call.call(...). Apologies if we've already discussed this endlessly and agreed this is an acceptable risk; I just don't remember a conversation like that...

Of course if the implementation here is already sufficient to make those well-behaved, let's just test that.

Retracting r+ for now, out of caution. I expect to stamp it again tomorrow once we've talked.

Comment 26

[Jason Orendorff [:jorendorff]]

Comment on attachment 831619 [details] [diff] [review]
Part 7 - Resolve canonical eval() onto Xrayed globals. v1

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

same comment

Comment 27

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #19)

> This ancient code makes no sense. Please file a follow-up bug to remove
> object_prototype_names if you haven't done it already. It can be replaced
> with something like
> 
>     if (!obj->as<GlobalObject>().getOrCreateObjectPrototype(cx))
>         return false;

Filed bug 942037.

Comment 28

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #20)
> Why not just insert nothing into the table?

As noted on IRC, we need JSProtoKeys to properly index into the table.

Comment 29

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #23)
> Just generally speaking, does this have to be in the XrayTraits abstract
> base class? Or can it go in one of the concrete classes (and spare the
> other)?

It needs to go there. We don't want to have to move this when Window moves from an XPCWN to a WebIDL DOM object. Moreover, once we have Xrays to things like sandboxes, this stuff will need to be general.

Comment 30

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #24)
> > +JS_FRIEND_API(bool)
> > +js::GetOriginalEval(JSContext *cx, HandleObject scope, MutableHandleObject eval)
> 
> Make it a Handle<GlobalObject*>?

GlobalObject is not exposed outside of SM AFAIK.


> Or just rename the argument to HandleObject global, and use
> global->as<GlobalObject>() rather than scope->global().
> 
> One reason is that scope->global() doesn't do what the caller would expect
> if scope is, say, an outer window, or a CCW for one.

It works for an outer window. And the behavior with respect to CCWs is identical to that of all the other APIs like this. Even if you force it to be a global object, it doesn't help the consumer avoid CCW confusion - JS_GetGlobalForObject(cx, ccw) is still going to get them the wrong global.

The basic reason that all the DOM APIs take |scope| rather than |global| is that getting a global outside of SM is more of a mouthful than it is inside the JS engine - you either need to enter a compartment, or use js::GetGlobalForObjectCrossCompartment. The |aScope| pattern pervades the dom, so I'm not worried.

Comment 31

[Bobby Holley (:bholley)]

Full try push, pending discussion with jorendorff tomorrow morning:

https://tbpl.mozilla.org/?tree=Try&rev=5a48dafb41cb

Comment 32

[Bobby Holley (:bholley)]

(In reply to Jason Orendorff [:jorendorff] from comment #23)
> Could move the JSAutoCompartment into the inner block.

I now recall that I didn't do this because it leads to a compartment mismatch with the next patch.

Cancelled the try push in comment 31 and repushed to try with that fixed:

https://tbpl.mozilla.org/?tree=Try&rev=3c683bf3ef8d

Comment 33

[Jason Orendorff [:jorendorff]]

Comment on attachment 831618 [details] [diff] [review]
Part 6 - Add lookups for standard classes in XrayWrapper. v1

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

r=me after IRC discussion with bholley

Comment 34

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ad39d10f98
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/30bbe45c775e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/72db5a6ae5c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/497280442d1d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/98881f65a665
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/93cead0610da
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/176caf61a975
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/061c63d261e5

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b5ad39d10f98
https://hg.mozilla.org/mozilla-central/rev/30bbe45c775e
https://hg.mozilla.org/mozilla-central/rev/72db5a6ae5c8
https://hg.mozilla.org/mozilla-central/rev/497280442d1d
https://hg.mozilla.org/mozilla-central/rev/98881f65a665
https://hg.mozilla.org/mozilla-central/rev/93cead0610da
https://hg.mozilla.org/mozilla-central/rev/176caf61a975
https://hg.mozilla.org/mozilla-central/rev/061c63d261e5