Closed Bug 933681 Opened 6 years ago Closed 6 years ago

Resolve standard JS classes on Xrayed globals

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Depends on 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(8 files)

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?
Flags: needinfo?(jorendorff)
> 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.
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.
Depends on: 919095
(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.
Flags: needinfo?(jorendorff)
So what's the status here Bobby?  The NFC folks want bug 933497 soon.
Flags: needinfo?(bobbyholley+bmo)
(I'm actively working on this)
Depends on: 936232
I have this working locally and passing XPConnect tests. Let's see what try says:

https://tbpl.mozilla.org/?tree=Try&rev=f3c7450bd6db
Flags: needinfo?(bobbyholley+bmo)
Green modulo one test that I fixed. Uploading patches and flagging for review.
This is necessary to see this stuff over Xrays.
Attachment #831609 - Flags: review?(jorendorff)
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. :-(
Attachment #831612 - Flags: review?(jorendorff)
Attachment #831620 - Flags: review?(jorendorff)
Blocks a blocker.

We need this by December 9th, and ideally ASAP.
blocking-b2g: --- → 1.3+
Flags: needinfo?(jorendorff)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 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...
Attachment #831609 - Flags: review?(jorendorff) → review+
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;
Attachment #831611 - Flags: review?(jorendorff) → review+
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().
Attachment #831612 - Flags: review?(jorendorff) → review+
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.
Attachment #831614 - Flags: review?(jorendorff) → review+
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.
Attachment #831617 - Flags: review?(jorendorff) → review+
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.
Attachment #831618 - Flags: review?(jorendorff) → review+
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"? :) :) :)
Attachment #831619 - Flags: review?(jorendorff) → review+
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.
Attachment #831618 - Flags: review+
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
Attachment #831619 - Flags: review+
Blocks: 942037
(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.
(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.
(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.
(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.
Full try push, pending discussion with jorendorff tomorrow morning:

https://tbpl.mozilla.org/?tree=Try&rev=5a48dafb41cb
(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 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
Attachment #831618 - Flags: review+
Attachment #831620 - Flags: review?(jorendorff) → review+
Flags: needinfo?(jorendorff)
Whiteboard: [qa-]
Blocks: 986940
You need to log in before you can comment on or make changes to this bug.