787070 - Expandos on the xray of DOM prototypes should have effect on xrays of DOM nodes

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

So the problem is if you add an expando (from chrome side) on the xray of let's say Node.prototype (of the content) you won't find the property on the xray of any Node instances. Is there a security reason why expandos on xrays do not support prototypes, or is this just something that not trivial to implement and no one needed yet?

Comment 1

[Boris Zbarsky [:bzbarsky]]

I thought prototype mirroring was supposed to make this work, actually...

Comment 2

[Alexandre Poirot [:ochameau]]

Here is some test code snippets, working from scratchpad in chrome mode.

Here is a first example that highlights the "regular web behavior":
  var rv = content.wrappedJSObject.eval("(" + function SandboxContext() {
    window = window.wrappedJSObject;
    window.HTMLElement.prototype.foo = "bar";
    return window.document.body.foo;
  } + ")()");
  Components.utils.reportError(rv); // Prints "bar"

And a second one, with a Sandbox and xrays, that should work alike, but currently doesn't:
  var sb = Components.utils.Sandbox(content);
  sb.window = content;
  var rv = Components.utils.evalInSandbox("(" + function SandboxContext() {
    // It fails here as `prototype` is undefined:
    window.HTMLElement.prototype.foo = "bar";
    return window.document.body.foo;
  } + ")()", sb);
  Components.utils.reportError(rv); // Should print "bar"

Comment 3

[Alexandre Poirot [:ochameau]]

Similar example without using these HTML* globals:
  var sb = Components.utils.Sandbox(content);
  sb.window = content;
  var rv = Components.utils.evalInSandbox("(" + function SandboxContext() {
    Object.getPrototypeOf(window.document.createElement("div")).foo = "bar";
    return window.document.createElement("div").foo;
  } + ")()", sb);
  Components.utils.reportError(rv); // Should print "bar"

Comment 4

[Gabor Krizsanits (INACTIVE)]

This is more like what I meant:

var sb = Components.utils.Sandbox(content);
  sb.window = content;
  sb.htmlproto = window.HTMLElement.prototype;
  var rv = Components.utils.evalInSandbox("(" + function SandboxContext() {
    // It fails here as `prototype` is undefined:
    htmlproto.foo = "bar";
    return window.document.body.foo;
  } + ")()", sb);
  Components.utils.reportError(rv); // Should print "bar"

Comment 5

[Bobby Holley (:bholley)]

So, the problem here is that we don't have any kind of useful Xrays into DOM prototypes. Xrays are applied based on the JSClass of the underlying object, and things like Node.prototype are either sDOMConstructorProtoClass or one of the XPC_WN_*_*_Proto_JSClass variants. The details of when we do which seem pretty obscure.

Anyway, the point is that you don't get any kind of Xray wrapper when touching a content prototype - you just get a plain old CrossCompartmentWrapper. So your expando will actually end up in the content compartment. But this means that the expando will be ignored when using Xray delegation on an object inheriting that prototype. There's not a great way to do this differently with the current Xray architecture.

Also, Gabor, your example in comment 4 confuses me. It seems like you're having content muck with chromeWindow.HTMLElement.prototype?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> we don't have any kind of useful Xrays into DOM prototypes. 

I believe Peter plans to change that for Paris prototypes, fwiw.

Comment 7

[Gabor Krizsanits (INACTIVE)]

Based on the complications described in Comment 5 and because this will problem will just go away with the new bindings I tend to lean toward the 'won't fix' here. ochameau: how urgent this is for us? 
bz: do you have any guestimation when that might happen?

Comment 8

[Alexandre Poirot [:ochameau]]

Not urgent, we never had this feature in content script.
But really worth having it working as some JS framework depends on such feature.
(We have to patch prototype.js in order to make it work as a content script)
(chromium content scripts equivalent support this)

Comment 9

[Gabor Krizsanits (INACTIVE)]

I think it's an important feature for xrays anyway. I talked to peter and the patch mentioned in comment 6 is in bug 778152. So that solves one of the issues, that currently dom prototypes are wrapped in COWs instead of xrays. Once that is fixed I and the conversion to that new binding happened I might have a better chance at fixing this in a sane way.

Comment 10

[Boris Zbarsky [:bzbarsky]]

ETA on nodes being on new bindings is probably ~6 months.

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: v1

Green on try.

I'm not sure I'm happy with the split between code in XrayWrapper.cpp and code in BindingUtils.cpp. Because I've made everything happen from resolveOwnProperty I've made XrayResolveNativeProperty deal with all properties, indexed and named properties and others. Because we don't want to cache indexed and named properties I've had to move the logic of looking up and defining stuff on the holder into XrayResolveNativeProperty. On the one hand this seems ugly, but on the other hand this could make it easier to deal with OverrideBuiltins when we start supporting that (because I think we don't want OverrideBuiltins support on an Xray).

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Pass through the right receiver if a property is living on a proxy on the prototype chain v1

JSObject::setGeneric calls the setGeneric hook if there is one, but that hook doesn't take a receiver. Proxies have a setGeneric hook (proxy_SetGeneric) which just calls Proxy::set. This patch makes us call that directly on the proxy.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Move some code around v1

This is just to make the next patch slightly clearer.

Comment 14

[Bobby Holley (:bholley)]

Did you mean to cancel review on the patch Peter?

Comment 15

[Eric Faust [:efaust]]

Comment on attachment 8359730 [details] [diff] [review]
Pass through the right receiver if a property is living on a proxy on the prototype chain v1

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

This is ugly, but unavoidable. r=me

::: js/src/jsproxy.cpp
@@ +2553,5 @@
>                  Rooted<PropertyDescriptor> desc(cx);
>                  if (!JS_GetPropertyDescriptorById(cx, proto, id, 0, &desc))
>                      return false;
> +                if (desc.object() && desc.setter()) {
> +                    if (IsProxy(proto)) {

nit: if (proto->is<ProxyObject>()), I think.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: Part 1 - Move some code around v2

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: Mirror DOM prototype chain on Xrays v2

Sorry that this is one big patch, but it looked hard to split up. I'll try to catch you on irc to discuss it first.

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8364995 [details] [diff] [review]
Mirror DOM prototype chain on Xrays v2

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

Sorry for the delay here - I am very excited about this.

IIUC, we still need separate tests to verify the behavior we're targeting with this patch. Is that correct? I think it's definitely worth spending at least a little bit of time on them to make sure they're thorough, given that the setHasPrototype stuff doesn't have an extensive amount of in-tree usage.

It would be really nice if we could split the toString stuff out into its own patch, for ease of reviewing and regression-finding (Xray stringification changes always break stuff). I won't insist on it though, especially if you think it would significantly slow down getting this landed.

I agree that the ResolveNativeProperty/ResolveOwnProperty is kind of a toss-up semantically, but I think we should probably go with ResolveOwnProperty, since that matches the naming the traps in the Xray traits.

As discussed last week, I think we should keep the holder manipulation (and XBL secret sauce) in XrayWrapper.cpp, and pass in |bool *defineOnHolder| to XrayResolveOwnProperty.

::: js/xpconnect/wrappers/XrayWrapper.h
@@ +132,5 @@
> +                        JS::HandleObject target, JS::MutableHandleObject protop)
> +    {
> +        return getPrototypeOf<Traits::HasPrototype>(cx, wrapper, target,
> +                                                    protop);
> +    }

Seems like this whole collection of things should be called |getPrototypeOfInternal| or something. The currently argument-based overloading is kind of incidental, and I don't think we should rely on it.

Comment 19

[Bobby Holley (:bholley)]

I split out the changes that I need for bug 914970 into separate patches, which I've stamped and will attach here now.

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add some machinery to allow Traits to specify whether they want to use hasPrototype or not. v1 r=bholley

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 3 - Pass the real receiver when using HasPrototype. v1 r=bholley

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 2 - Add some machinery to allow Traits to specify whether they want to use hasPrototype or not. v2 r=bholley

Maybe something more like this.

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8375165 [details] [diff] [review]
Part 2 - Add some machinery to allow Traits to specify whether they want to use hasPrototype or not. v2 r=bholley

I'm moving these patches to bug 975277.

Comment 24

[Bobby Holley (:bholley)]

I just realized that this needs to block on bug 987111, because otherwise the last item in the prototype chain (Object.prototype) will be spoofable by content.

Comment 25

[Bobby Holley (:bholley)]

Are you likely to get back to this soon Peter? I know you're busy, but I'm just curious where this sits in your pile of things in a post-Window-WebIDL world.

Comment 26

[Peter Van der Beken [:peterv]]

Yeah, I've been trying to make it work again, but not there yet.

Comment 27

[Bobby Holley (:bholley)]

Note that with bug 1033927, we shouldn't need the custom ToString stuff here at all anymore.

The one gotcha I ran into when playing with this is the custom toString we install on constructors to handle the fact that they aren't actually functions. The correct way to fix this is to just make those toString implementations resolvable over Xrays, at which point everything should Just Work once HasPrototype=1.

Comment 28

[Peter Van der Beken [:peterv]]

I've run into an issue, now that Location is on WebIDL. Location has properties that are cross-origin-settable but not -gettable. Proxy::set for proxies with a prototype calls Proxy::getPropertyDescriptor to call a setter if there is one (http://hg.mozilla.org/mozilla-central/annotate/75db55f6fd2c/js/src/jsproxy.cpp#l2319). The problem is that this will end up checking the policy for GET, and that'll fail for those properties that are not cross-origin-gettable. I could make this call a policy-check-skipping equivalent of Proxy::getPropertyDescriptor, but the problem then is how to propagate that 'skipping the policy check' when walking the prototype chain. We currently use JS_GetPropertyDescriptorById on the protos, and I'm not really in the mood to write a policy-check-skipping version of that :-/. Any ideas?

Comment 29

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #28)
> I've run into an issue, now that Location is on WebIDL. Location has
> properties that are cross-origin-settable but not -gettable. Proxy::set for
> proxies with a prototype calls Proxy::getPropertyDescriptor to call a setter
> if there is one
> (http://hg.mozilla.org/mozilla-central/annotate/75db55f6fd2c/js/src/jsproxy.
> cpp#l2319). The problem is that this will end up checking the policy for
> GET, and that'll fail for those properties that are not
> cross-origin-gettable. I could make this call a policy-check-skipping
> equivalent of Proxy::getPropertyDescriptor, but the problem then is how to
> propagate that 'skipping the policy check' when walking the prototype chain.
> We currently use JS_GetPropertyDescriptorById on the protos, and I'm not
> really in the mood to write a policy-check-skipping version of that :-/. Any
> ideas?

I've actually been in the process of solving this problem at the spec level for about a year now, and I was actually working on the Gecko patch for this particular issue today in bug 965898. :-)

Since it'll presumably unblock you, I'll split the fix out for that piece into a separate bug.

Comment 30

[Peter Van der Beken [:peterv]]

Ran into another issue. Bobby, this is related to a change you made to Proxy::set, shouldn't we throw for readonly properties here: https://hg.mozilla.org/integration/mozilla-inbound/rev/5db44a9eece2#l1.48 ?

Comment 31

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #30)
> Ran into another issue. Bobby, this is related to a change you made to
> Proxy::set, shouldn't we throw for readonly properties here:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5db44a9eece2#l1.48 ?

I would think that would be the responsibility of the defineProperty call. It has to handle this case anyway, because Object.defineProperty sinks straight to it.

Comment 32

[Peter Van der Beken [:peterv]]

Well, Object.defineProperty doesn't throw if you call it for an existing property with a descriptor that has identical values as the descriptor for the existing property. We still want Set to throw if it's used for setting a readonly property to the same value. I don't see how to get that behaviour without throwing from Proxy::set before it calls defineProperty.

Comment 33

[Bobby Holley (:bholley)]

Ah, I see. Then yeah, we can just add a check for own readonly value-props in Proxy::set.

Comment 34

[Peter Van der Beken [:peterv]]

Attachment: Make GetNativePropertyHooks detect global objects v1

Comment 35

[Peter Van der Beken [:peterv]]

Attachment: Make global properties own on Xrays v1

Comment 36

[Peter Van der Beken [:peterv]]

Attachment: Make Proxy::set throw for read-only properties v1

Comment 37

[Peter Van der Beken [:peterv]]

Attachment: Make CreateInterfaceObjects take a JS::Class instead of a JSClass v1

Comment 38

[Peter Van der Beken [:peterv]]

Attachment: Move some code around v1

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8489543 [details] [diff] [review]
Make GetNativePropertyHooks detect global objects v1

r=me

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8489564 [details] [diff] [review]
Make CreateInterfaceObjects take a JS::Class instead of a JSClass v1

Commit comment should have "js::Class", not "JS::Class".

r=me

Comment 41

[Peter Van der Beken [:peterv]]

Attachment: Give the WindowNamedPropertiesHandler a DOMIfaceAndProtoJSClass v1

Comment 42

[Peter Van der Beken [:peterv]]

Attachment: Annotate Window as having named properties v1

Comment 43

[Peter Van der Beken [:peterv]]

Attachment: Calculate parent prototype names in one place v1

Comment 44

[Peter Van der Beken [:peterv]]

Attachment: Create the named properties object from the binding code v1

Comment 45

[Peter Van der Beken [:peterv]]

Attachment: Cache named properties objects v1

So that we can easily look them up for Xrays.

Comment 46

[Peter Van der Beken [:peterv]]

Attachment: Store whether to call resolveOwnProperty and how to get the parent prototype object in DOMIfaceAndProtoJSClass v1

This allows us to resolve stuff on the Xray for the named properties object. mCallResolveOwnProperty is there because we use the same native property hooks for instances and interface objects. So interface objects might have native property hooks with a non-null mResolveOwnProperty but should have mCallResolveOwnProperty set to null.

Comment 47

[Peter Van der Beken [:peterv]]

Attachment: Add resolve and enumerate native property hooks for the Window named properties object v1

Comment 48

[Peter Van der Beken [:peterv]]

Attachment: Make Xrays walk the prototype chain when resolving DOM properties v1

This is still rather big, but I don't think I can easily make it smaller.

Comment 49

[Eric Faust [:efaust]]

Comment on attachment 8489553 [details] [diff] [review]
Make Proxy::set throw for read-only properties v1

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

r=me with error message changed.

::: js/src/proxy/Proxy.cpp
@@ +323,5 @@
>  
> +    if (desc.isReadonly()) {
> +        unsigned errorNumber = desc.object() == proxy ?
> +                               JSMSG_READ_ONLY :
> +                               JSMSG_CANT_REDEFINE_PROP;

This should just unconditionally be JSMSG_READ_ONLY. We are trying to mimic the behavior of baseops::SetPropertyHelper. JSMSG_CANT_REDEFINE_PROP appears to be used for defineProperty, not set.

The relatively similar-looking code in BaseProxyHandler::set is equally confused. That is a bug.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8489572 [details] [diff] [review]
Give the WindowNamedPropertiesHandler a DOMIfaceAndProtoJSClass v1

>-                           JS::NullHandleValue, protoProto,
>+                           JS::NullHandleValue, aProto,

This change doesn't make sense to me.

Also, shouldn't we be passing JSCLASS_IS_DOMIFACEANDPROTOJSCLASS to PROXY_CLASS_DEF here?

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8489573 [details] [diff] [review]
Annotate Window as having named properties v1

r=me assuming this is a no-op on the codegen... is it?  If not, what does it actually change?

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8489574 [details] [diff] [review]
Calculate parent prototype names in one place v1

Why make _make_name a static method?

Though given that it is, why not use the .name of the various descriptors involved instead of calling _make_name on them, since they have the relevant string in .name already?

r=me with this fixed up.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8489576 [details] [diff] [review]
Create the named properties object from the binding code v1

Ah, here's where you actually want aProto instead of protoProto!

>+class CGGetNamedPropertiesObjectMethod(CGAbstractStaticMethod):
>+            ifaceName=self.descriptor.name,

Not needed.

r=me with that removed.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8489577 [details] [diff] [review]
Cache named properties objects v1

>+         /* Check to see whether the interface objects are already installed */

Make that comment reflect the code it's about?

>+            namedPropertiesObject = ${nativeType}::CreateNamedPropertiesObject(aCx, ${parentProto});

This creates a new one each time, no?  Where is the caching?

Or is the idea that we never have one when we hit this code?  If so, should we be asserting that?

Comment 55

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #50)
> Comment on attachment 8489572 [details] [diff] [review]
> Give the WindowNamedPropertiesHandler a DOMIfaceAndProtoJSClass v1
> 
> >-                           JS::NullHandleValue, protoProto,
> >+                           JS::NullHandleValue, aProto,
> 
> This change doesn't make sense to me.

Yeah, it doesn't to me either, it should just have been in attachment 8489576 [details] [diff] [review].

> Also, shouldn't we be passing JSCLASS_IS_DOMIFACEANDPROTOJSCLASS to
> PROXY_CLASS_DEF here?

Not here, no. Attachment 8489583 [details] [diff] adds that, when we actually hook everything up.

(In reply to Boris Zbarsky [:bz] from comment #51)
> r=me assuming this is a no-op on the codegen... is it?  If not, what does it
> actually change?

Yeah, no-op here, but we start using it in attachment 8489576 [details] [diff] [review].

(In reply to Boris Zbarsky [:bz] from comment #52)
> Though given that it is, why not use the .name of the various descriptors
> involved instead of calling _make_name on them, since they have the relevant
> string in .name already?

I missed that, will use .name instead.

(In reply to Boris Zbarsky [:bz] from comment #54)
> Comment on attachment 8489577 [details] [diff] [review]
> This creates a new one each time, no?  Where is the caching?

I seem to have lost that code somehow. I'll fix this patch.

Comment 56

[Boris Zbarsky [:bzbarsky]]

> Yeah, it doesn't to me either, it should just have been in attachment 8489576 [details] [diff] [review]

OK.  r=me on attachment 8489572 [details] [diff] [review] with that change undone.  Thanks!

Comment 57

[Bobby Holley (:bholley)]

Comment on attachment 8489552 [details] [diff] [review]
Make global properties own on Xrays v1

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

When did we end up with two different functions called XrayResolveNativeProperty? :-(

::: dom/bindings/BindingUtils.cpp
@@ +1371,5 @@
> +    // XrayResolveOwnProperty, so skip those.
> +    if ((isGlobal && GlobalPropertiesAreOwn()) &&
> +        !(nativePropertyHooks = nativePropertyHooks->mProtoHooks)) {
> +      return true;
> +    }

I think it would be clearer to just set nativePropertyHooks in the {}, skip the |return true|, and make the |do| loop a |while| loop. The assignment in the condition took me a long time to notice.

@@ +1548,5 @@
>                                         isGlobal, obj, flags, props)) {
>        return false;
>      }
>  
> +    if (!(isGlobal && GlobalPropertiesAreOwn()) && (flags & JSITER_OWNONLY)) {

This is going to incorrectly report things from EventTarget.prototype when doing Object.getOwnPropertyNames(xrayedWindow), right? That's probably fine given that this is a temporary state of affairs, but please add a comment to that effect.

Comment 58

[Bobby Holley (:bholley)]

Comment on attachment 8489567 [details] [diff] [review]
Move some code around v1

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

r=me assuming that the moves don't change any code.

Comment 59

[Bobby Holley (:bholley)]

Comment on attachment 8489581 [details] [diff] [review]
Store whether to call resolveOwnProperty and how to get the parent prototype object in DOMIfaceAndProtoJSClass v1

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

bz should probably look at the bindings changes too.

::: dom/bindings/BindingUtils.cpp
@@ +1325,5 @@
>  
> +  bool callResolveOwnProperty;
> +  if (type == eInstance) {
> +    callResolveOwnProperty = true;
> +  } else {

Shouldn't this apply only to the prototype case, and not the constructor case?

@@ +1329,5 @@
> +  } else {
> +    const js::Class* clasp = js::GetObjectClass(obj);
> +    if (IsDOMIfaceAndProtoClass(clasp)) {
> +      callResolveOwnProperty =
> +        DOMIfaceAndProtoJSClass::FromJSClass(clasp)->mCallResolveOwnProperty;

This should really be called mCallResolveOwnPropertyOnPrototype, right?

@@ +1342,5 @@
> +      return false;
> +    }
> +
> +    if (desc.object()) {
> +      // None of these should be cached on the holder, since they're dynamic.

Which? Properties from mResolveOwnProperty? Is that documented somewhere?

::: dom/bindings/BindingUtils.h
@@ +2373,5 @@
> +      const js::Class* clasp = js::GetObjectClass(obj);
> +      if (IsDOMIfaceAndProtoClass(clasp)) {
> +        protoGetter = DOMIfaceAndProtoJSClass::FromJSClass(clasp)->mGetParentProto;
> +      } else {
> +        protoGetter = nullptr;

Can't we MOZ_ASSERT(false) here?

::: dom/bindings/Codegen.py
@@ +631,5 @@
>                ${hooks},
>                "[object ${name}Prototype]",
>                ${prototypeID},
> +              ${depth},
> +              false,

Please add /* mCallResolveOwnProperty (or whatever we end up naming it) = */

@@ +700,5 @@
>                ${hooks},
>                "function ${name}() {\\n    [native code]\\n}",
>                ${prototypeID},
> +              ${depth},
> +              false,

And here.

Comment 60

[Bobby Holley (:bholley)]

Comment on attachment 8489582 [details] [diff] [review]
Add resolve and enumerate native property hooks for the Window named properties object v1

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

r=me with that.

::: dom/base/WindowNamedPropertiesHandler.cpp
@@ +234,5 @@
> +                               JS::Handle<JSObject*> aObj,
> +                               JS::AutoIdVector& aProps)
> +{
> +  JSAutoCompartment ac(aCx, aObj);
> +  return js::GetProxyHandler(aObj)->getOwnPropertyNames(aCx, aObj, aProps);

Need to scope the ac and wrap the AutoIdVector to handle non-interned strings and (now) symbols.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8489581 [details] [diff] [review]
Store whether to call resolveOwnProperty and how to get the parent prototype object in DOMIfaceAndProtoJSClass v1

>+      callResolveOwnProperty = nullptr;

You mean false, right?

>+  if (callResolveOwnProperty && nativePropertyHooks->mResolveOwnProperty) {

It might be nice to reverse this check and early return true, then outdent the actual calling code (and hoist the "return true" in the desc.object() case up a level so it's unconditional.

On a more substantive note:

1)  If we have "interface A : B {}" then Object.getPrototypeOf(A.prototype) == B.prototype.  So you need to have a proto getter on CGInterfaceObjectJSClass, I believe.

2)  For root interfaces, the proto of the interface object is Function.prototype, not Object.prototype.  Shouldn't this be reflected via Xrays?

3)  For root interfaces, the proto of the interface prototype object is not necessarily Object.prototype; it can be Array.prototype (for [ArrayClass]) or Error.prototype (for [ExceptionClass]).  Shouldn't this be reflected via Xrays?

Comment 62

[Bobby Holley (:bholley)]

Comment on attachment 8489583 [details] [diff] [review]
Make Xrays walk the prototype chain when resolving DOM properties v1

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

r- because of the AccessCheck.cpp stuff. Everything else looks good with comments addressed.

It seems like there should be no more need after this patch to expose a separate XrayResolveNativeProperty. Can we add a patch on top if this one to make that private to BindingUtils.cpp and potentially clean that stuff up a little bit? It's a clumsy abstraction, and I'd like to get rid of it.

Thanks for pushing this through!

::: dom/base/WindowNamedPropertiesHandler.cpp
@@ +249,5 @@
>  
>  static const DOMIfaceAndProtoJSClass WindowNamedPropertiesClass = {
>    PROXY_CLASS_DEF("WindowProperties",
>                    DOM_INTERFACE_PROTO_SLOTS_BASE, /* extra slots */
> +                  JSCLASS_IS_DOMIFACEANDPROTOJSCLASS,

Is this really the right patch for this change?

::: dom/bindings/BindingUtils.cpp
@@ +1403,5 @@
> +                                           0, desc, cacheOnHolder);
> +    }
> +
> +    // The properties for globals live on the instance, so return here as there
> +    // are no properties on their interface prototype object.

Rather than having 3 different early-returns, I think it would be cleaner to just make the final ResolveOwnProperty call conditional on (eIsInstance && isGlobal && GlobalPropertiesAreOwn()).

@@ +1530,5 @@
>  {
>    if (type == eInstance) {
>      ENUMERATE_IF_DEFINED(unforgeableMethod);
>      ENUMERATE_IF_DEFINED(unforgeableAttribute);
> +    if (isGlobal && GlobalPropertiesAreOwn()) {

Can we cache this pair as a bool at the top of this function rather than inlining it three times?

@@ +1538,4 @@
>    } else if (type == eInterface) {
>      ENUMERATE_IF_DEFINED(staticMethod);
>      ENUMERATE_IF_DEFINED(staticAttribute);
> +  } else if (!(isGlobal && GlobalPropertiesAreOwn())) {

I think it would be clearer to move this to a separate if() {} block within this one. Right now, it appears that the MOZ_ASSERT(type == eInterfacePrototype) is dependent on this condition, which it isn't.

@@ +1614,5 @@
>    const NativePropertyHooks* nativePropertyHooks =
>      GetNativePropertyHooks(cx, obj, type, isGlobal);
>  
>    if (type == eInstance) {
> +    // FIXME Should do something about XBL properties too.

Yeah, though it's probably not super urgent. At least file a bug and and put the bug number here?

@@ +1621,5 @@
>                                                        props)) {
>        return false;
>      }
> +  } else if (type == eInterfacePrototype && isGlobal &&
> +             GlobalPropertiesAreOwn()) {

Similarly, I'd rather make the ensuing XrayEnumerateNativeProperties conditional on the converse of this and avoid non-exceptional early-returns.

::: dom/bindings/DOMJSClass.h
@@ +156,5 @@
>    // constructors::id::_ID_Count.
>    constructors::ID mConstructorID;
>  
> +  // The NativePropertyHooks instance for the parent interface (for
> +  // ShimInterfaceInfo).

?

::: dom/tests/mochitest/chrome/test_sandbox_bindings.xul
@@ +126,5 @@
>        } catch (e) {
>          ok(false, "XMLHttpRequest.prototype manipulation via an Xray shouldn't throw" + e);
>        }
>        try {
> +        Components.utils.evalInSandbox("XMLHttpRequest.prototype.a = false", sandbox);

Can you use strings or something else that undefined won't automatically be coerced to?

@@ +134,5 @@
> +        is(xhr.a, undefined, "'XMLHttpRequest()' in a sandbox should not have expandos from inside the sandbox");
> +        ok(xhr.b, "'new XMLHttpRequest()' in a sandbox should have Xray expandos");
> +      } catch (e) {
> +        ok(false, "'new XMLHttpRequest()' shouldn't throw in a sandbox");
> +      }

Please add some more involved tests for stuff like:

* Properties inherited from Object.prototype.
* Window named properties handler.
* Making sure that the target doesn't see the Xray expandos.
* toString on interface objects.

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +140,5 @@
>  
>      return domwin != nullptr;
>  }
>  
> +enum GetPropertyPermitted {

Call this PropertyAccessLevel.

@@ +142,5 @@
>  }
>  
> +enum GetPropertyPermitted {
> +    eNothing,
> +    eIndexedAndNonShadowedNamedProperties, 

Nit - trailing whitespace.

@@ +147,5 @@
> +    eNamedProperties
> +};
> +
> +static GetPropertyPermitted
> +GetPermitted(const char *name)

Call this IsNamedAndIndexedAccessPermitted.

@@ +157,5 @@
> +            // properties.
> +            return eIndexedAndNonShadowedNamedProperties;
> +        }
> +
> +        if (!strcmp(rest, "Prototype")) {

I don't see how this can possibly work. The filtering policy is operating on a CrossOriginXrayWrapper, which flattens everything onto the object and has a null prototype. So |name| here should never be anything other than "Window" or "Location".

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1244,5 @@
>  XrayTraits::resolveOwnProperty(JSContext *cx, const Wrapper &jsWrapper,
>                                 HandleObject wrapper, HandleObject holder, HandleId id,
>                                 MutableHandle<JSPropertyDescriptor> desc)
>  {
> +    // Only chrome wrappers and same-origin xrays (used by jetpack sandboxes)

This isn't really accurate. Same-origin xrays are deprecated, and jetpack uses nsExpandedPrincipal (which are neither chrome nor same-origin). I'd just say "Only subsuming callers get .wrappedJSObject".

@@ +1246,5 @@
>                                 MutableHandle<JSPropertyDescriptor> desc)
>  {
> +    // Only chrome wrappers and same-origin xrays (used by jetpack sandboxes)
> +    // get .wrappedJSObject. We can check this by determining if the compartment
> +    // of the wrapper subsumes that of the wrappee.

Don't we need to get rid of the corresponding code at the bottom of this function?

@@ +1544,5 @@
> +    // Xrays for DOM binding objects have a prototype chain that consists of
> +    // Xrays for the prototypes of the DOM binding object (ignoring changes in
> +    // the prototype chain made by script, plugins or XBL). All properties for
> +    // these Xrays are really own properties, either of the instance object or
> +    // of the prototypes.

This function will never be called, and can just MOZ_ASSERT(false), right? Probably worth inlining it into the traits definition like we do for JSXrayTraits.

@@ +1750,5 @@
>      }
>  
>      RootedObject obj(cx, XrayTraits::getTargetObject(wrapper));
>      if (UseDOMXray(obj)) {
> +        JS_ReportError(cx, "XrayToString called on an incompatible object");

I think we can get rid of XrayToString now. File a followup bug on doing that?

::: js/xpconnect/wrappers/XrayWrapper.h
@@ +153,5 @@
>  class DOMXrayTraits : public XrayTraits
>  {
>  public:
>      enum {
> +        HasPrototype = 1

\o/

Comment 63

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #61)
> Comment on attachment 8489581 [details] [diff] [review]
> Store whether to call resolveOwnProperty and how to get the parent prototype
> object in DOMIfaceAndProtoJSClass v1
> 
> >+      callResolveOwnProperty = nullptr;
> 
> You mean false, right?
> 
> >+  if (callResolveOwnProperty && nativePropertyHooks->mResolveOwnProperty) {
> 
> It might be nice to reverse this check and early return true, then outdent
> the actual calling code (and hoist the "return true" in the desc.object()
> case up a level so it's unconditional.
> 
> On a more substantive note:
> 
> 1)  If we have "interface A : B {}" then Object.getPrototypeOf(A.prototype)
> == B.prototype.  So you need to have a proto getter on
> CGInterfaceObjectJSClass, I believe.

I guess you mean |Object.getPrototypeOf(A) == B|?

Comment 64

[Boris Zbarsky [:bzbarsky]]

> I guess you mean |Object.getPrototypeOf(A) == B|?

Er, yes.

Comment 65

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #62)
> @@ +156,5 @@
> >    // constructors::id::_ID_Count.
> >    constructors::ID mConstructorID;
> >  
> > +  // The NativePropertyHooks instance for the parent interface (for
> > +  // ShimInterfaceInfo).
> 
> ?

ShimInterfaceInfo::GetConstantCount is the only user of mProtoHooks after this change.

I have two more patches that remove a bunch of unused code, but I'm going to attach those when I've dealt with all the review comments for the patches that they apply on top of.

Comment 66

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #61)
> 2)  For root interfaces, the proto of the interface object is
> Function.prototype, not Object.prototype.  Shouldn't this be reflected via
> Xrays?
> 
> 3)  For root interfaces, the proto of the interface prototype object is not
> necessarily Object.prototype; it can be Array.prototype (for [ArrayClass])
> or Error.prototype (for [ExceptionClass]).  Shouldn't this be reflected via
> Xrays?

The signature mismatch between JS_Get*Prototype (returning JSObject*) and DOM bindings' GetProto (returning JS::Handle<JSObject*>) makes this complicated :-(. Did we make GetProto return JS::Handle<JSObject*> for performance reasons? I guess I could cache the results of calling JS_Get*Prototype in the ProtoAndIfaceCache too.

Comment 67

[Boris Zbarsky [:bzbarsky]]

> Did we make GetProto return JS::Handle<JSObject*> for performance reasons?

Yes, to avoid having to sprinkle Rooted about.

The only reason JS_Get*Prototype doesn't return a Handle is that it's stored as a Value, not a JSObject*....

One option would be to create GetProtoObjectPtr methods in the bindings that return JSObject* and just call GetProtoObject.

Then your next problem will be that JS_GetErrorPrototype (which is newer than the other JSAPI getters) doesn't take a useless HandleObject arg.  So we might need to have a wrapper for that one too, or fix the other JS_Get*Prototype APIs.

Comment 68

[Boris Zbarsky [:bzbarsky]]

Oh, and I'd rather we didn't cache the JS protos in ProtoAndIfaceCache, I think.

Comment 69

[Peter Van der Beken [:peterv]]

Comment on attachment 8489583 [details] [diff] [review]
Make Xrays walk the prototype chain when resolving DOM properties v1

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

::: dom/bindings/BindingUtils.cpp
@@ +1403,5 @@
> +                                           0, desc, cacheOnHolder);
> +    }
> +
> +    // The properties for globals live on the instance, so return here as there
> +    // are no properties on their interface prototype object.

I don't understand what you mean here.

Comment 70

[Peter Van der Beken [:peterv]]

Attachment: Cache named properties objects v2

Comment 71

[Peter Van der Beken [:peterv]]

Attachment: Add a named properties object type to DOMObjectType and how to get the parent prototype object in DOMIfaceAndProtoJSClass v2

(In reply to Bobby Holley (:bholley) from comment #59)
> Comment on attachment 8489581 [details] [diff] [review]

> Shouldn't this apply only to the prototype case, and not the constructor
> case?

> This should really be called mCallResolveOwnPropertyOnPrototype, right?

> Which? Properties from mResolveOwnProperty? Is that documented somewhere?

Your questions made me realize that mResolveOwnProperty isn't the best solution to this problem, since what we really want is just to treat named properties objects specially. So I've redone this and added a new type for them to the DOMObjectType enum, and the Xray code just calls the mResolveOwnProperty/mEnumerateOwnProperties hooks for them and ignores all the other stuff in their NativePropertyHooks.

(In reply to Boris Zbarsky [:bz] from comment #61)
> It might be nice to reverse this check and early return true, then outdent
> the actual calling code (and hoist the "return true" in the desc.object()
> case up a level so it's unconditional.

Didn't do this, we'd have to undo it in a later patch where we don't return if !desc.object().

Comment 72

[Peter Van der Beken [:peterv]]

Comment on attachment 8493724 [details] [diff] [review]
Add a named properties object type to DOMObjectType and how to get the parent prototype object in DOMIfaceAndProtoJSClass v2

Huh, wrong patch.

Comment 73

[Peter Van der Beken [:peterv]]

Attachment: Add a named properties object type to DOMObjectType and how to get the parent prototype object in DOMIfaceAndProtoJSClass v2

Comment 74

[Peter Van der Beken [:peterv]]

Attachment: Make Xrays walk the prototype chain when resolving DOM properties v2

(In reply to Bobby Holley (:bholley) from comment #62)
> Comment on attachment 8489583 [details] [diff] [review]

> r- because of the AccessCheck.cpp stuff. Everything else looks good with
> comments addressed.

I reverted the AccessCheck stuff, it predates the flattening of properties on Xrays for global and is unneeded now.

> ::: dom/base/WindowNamedPropertiesHandler.cpp

> >  static const DOMIfaceAndProtoJSClass WindowNamedPropertiesClass = {
> >    PROXY_CLASS_DEF("WindowProperties",
> >                    DOM_INTERFACE_PROTO_SLOTS_BASE, /* extra slots */
> > +                  JSCLASS_IS_DOMIFACEANDPROTOJSCLASS,
> 
> Is this really the right patch for this change?

I think it is, I don't want to have to think about the consequences of exposing this through Xrays until this patch.

> @@ +1246,5 @@
> >                                 MutableHandle<JSPropertyDescriptor> desc)
> >  {
> > +    // Only chrome wrappers and same-origin xrays (used by jetpack sandboxes)
> > +    // get .wrappedJSObject. We can check this by determining if the compartment
> > +    // of the wrapper subsumes that of the wrappee.
> 
> Don't we need to get rid of the corresponding code at the bottom of this
> function?

Yeah, bad merge. This code can go now.

> @@ +1544,5 @@
> > +    // Xrays for DOM binding objects have a prototype chain that consists of
> > +    // Xrays for the prototypes of the DOM binding object (ignoring changes in
> > +    // the prototype chain made by script, plugins or XBL). All properties for
> > +    // these Xrays are really own properties, either of the instance object or
> > +    // of the prototypes.
> 
> This function will never be called, and can just MOZ_ASSERT(false), right?
> Probably worth inlining it into the traits definition like we do for
> JSXrayTraits.

It will be called by HasNativeProperty.

Comment 75

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #60)
> Comment on attachment 8489582 [details] [diff] [review]

> Need to scope the ac and wrap the AutoIdVector to handle non-interned
> strings and (now) symbols.

Bug 645416 actually removed JS_WrapAutoIdVector because ids don't need to be wrapped anymore.

Comment 76

[Peter Van der Beken [:peterv]]

Attachment: Stop forwarding sets to Traits v1

DOMXrayTraits::set is unused because DOMXrayTraits always have HasPrototype == 1 now.

Comment 77

[Peter Van der Beken [:peterv]]

Attachment: Remove obsolete code v1

I'll merge this into attachment 8493760 [details] [diff] [review] before landing, otherwise we might get link errors when building with just that patch, but I wanted to separate this out because the removals were clogging up the diff for attachment 8493760 [details] [diff] [review].

Comment 78

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #62)
-> ::: dom/tests/mochitest/chrome/test_sandbox_bindings.xul
> @@ +126,5 @@
> >        } catch (e) {
> >          ok(false, "XMLHttpRequest.prototype manipulation via an Xray shouldn't throw" + e);
> >        }
> >        try {
> > +        Components.utils.evalInSandbox("XMLHttpRequest.prototype.a = false", sandbox);
> 
> Can you use strings or something else that undefined won't automatically be
> coerced to?
> 
> @@ +134,5 @@
> > +        is(xhr.a, undefined, "'XMLHttpRequest()' in a sandbox should not have expandos from inside the sandbox");
> > +        ok(xhr.b, "'new XMLHttpRequest()' in a sandbox should have Xray expandos");
> > +      } catch (e) {
> > +        ok(false, "'new XMLHttpRequest()' shouldn't throw in a sandbox");
> > +      }
> 
> Please add some more involved tests for stuff like:
> 
> * Properties inherited from Object.prototype.
> * Window named properties handler.
> * Making sure that the target doesn't see the Xray expandos.
> * toString on interface objects.

Still working on these.

Comment 79

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493722 [details] [diff] [review]
Cache named properties objects v2

The $*{getParentProto} should move into the !namedPropertiesObject.get() clause, right?  r=me with that fixed.

Also, do you really need that .get()?

Comment 80

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493726 [details] [diff] [review]
Add a named properties object type to DOMObjectType and how to get the parent prototype object in DOMIfaceAndProtoJSClass v2

>+        if parentDesc.workers:
>+            parentIfaceName += "_workers"
>+        prefix = toBindingNamespace(parentIfaceName)

This should really be:

  prefix = toBindingNamespace(parentDesc.name)

and yes I know you copied this code...

r=me

Comment 81

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #69)
> I don't understand what you mean here.

It looks like you've redone these patches anyway. I'll see what they look like.

(In reply to Peter Van der Beken [:peterv] from comment #74)
> > This function will never be called, and can just MOZ_ASSERT(false), right?
> > Probably worth inlining it into the traits definition like we do for
> > JSXrayTraits.
> 
> It will be called by HasNativeProperty.

That kind of violates the whole HasPrototype abstraction, right? Seems like we should find a way to sidestep that. Maybe we could just check HasOwnProperty on the Window and its prototype?

Comment 82

[Bobby Holley (:bholley)]

Comment on attachment 8493726 [details] [diff] [review]
Add a named properties object type to DOMObjectType and how to get the parent prototype object in DOMIfaceAndProtoJSClass v2

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

r=bholley if the BindingUtils.cpp comes out looking more or less like what I suggest below. If there need to be differences then we should talk about it some more.

::: dom/bindings/BindingUtils.cpp
@@ +1263,5 @@
> +      if (!XrayResolveNativeProperty(cx, wrapper, nativePropertyHooks, type,
> +                                     obj, id, desc, cacheOnHolder)) {
> +        return false;
> +      }
> +  

Whitespace

@@ +1267,5 @@
> +  
> +      // For non-global non-instance Xrays there are no other properties, so
> +      // return here for them whether we resolved the property or not.
> +      if (!isGlobal || desc.object()) {
> +        cacheOnHolder = true;

Hm, why do we need this explicit cacheOnHolder override here? Shouldn't XrayResolveNativeProperty give us the right answer?

@@ +1274,2 @@
>      }
> +  

Here too

@@ +1274,3 @@
>      }
> +  
> +    {

We only want to do unforgeable stuff for instances, right?

@@ +1306,5 @@
> +        // None of these should be cached on the holder, since they're dynamic.
> +        cacheOnHolder = false;
> +
> +        return true;
> +      }

I think all this logic would be cleaner like this:

ResolveOwnProperty resolveOwnProperty = nativePropertyHooks->mResolveOwnProperty;

// Handle this special case and early-return.
if (type == eNamedPropertiesObject) {
  cacheOnHolder = false;
  return resolveOwnProperty(cx, wrapper, obj, id, desc);
}

// Resolve unforgeable properties first.
if (type == eInstance) {
  // Resolve unforgeable stuff.
  // Return if found (and MOZ_ASSERT(cacheOnHolder), I'd think).
}

// Resolve native properties. For globals, this needs to happen first, so that
// native properties take precedence over named properties.
if (type != isInstance || (isGlobal && GlobalPropertiesAreOwn()) {
  // ResolveNativeProperty stuff
  // If found, return.
}

// Resolve named properties and such.
if (type == eInstance) {
  cacheOnHolder = false;
  return resolveOwnProperty(cx, wrapper, obj, id, desc);
}

@@ +1582,2 @@
>  
> +  if (type == eNamedPropertiesObject) {

Here again, I think it'd be simpler to leave the code how it is and hoist the eNamedPropertiesObject handling into a special case on the top:

auto enumerateOwnProperties = nativePropertyHooks->mEnumerateOwnProperties;
if (type == eNamedPropertiesObject) {
  return enumerateOwnProperties(cx, wrapper, obj, props);
}

// Rest of the code stays how it is.

::: dom/bindings/BindingUtils.h
@@ +2373,5 @@
> +      } else {
> +        protop.set(JS_GetObjectPrototype(cx, global));
> +      }
> +    } else {
> +      const js::Class* clasp = js::GetObjectClass(obj);

Can we assert IsDOMIfaceAndProtoClass here just to be super explicit?

Comment 83

[Bobby Holley (:bholley)]

Comment on attachment 8493760 [details] [diff] [review]
Make Xrays walk the prototype chain when resolving DOM properties v2

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

I want to look at the updated BindingUtils changes. Everything else looks good modulo the comments below.

::: dom/bindings/BindingUtils.h
@@ +2294,5 @@
>  
>  // Implementation of the bits that XrayWrapper needs
>  
>  /**
> + * This resolves operations, attributes and constants of the interfaces for obj.

What is an operation?

::: js/xpconnect/wrappers/XrayWrapper.h
@@ +167,5 @@
> +        // Xrays for the prototypes of the DOM binding object (ignoring changes
> +        // in the prototype chain made by script, plugins or XBL). All properties for
> +        // these Xrays are really own properties, either of the instance object or
> +        // of the prototypes.
> +        return true;

Please make a note that we could MOZ_ASSERT(false) modulo XrayUtils::HasNativeProperty, and file a followup bug for the strategy proposed in comment 81 (or a counter-proposal).

Comment 84

[Bobby Holley (:bholley)]

Comment on attachment 8493763 [details] [diff] [review]
Stop forwarding sets to Traits v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ -1550,5 @@
> -    if (IsDOMProxy(obj)) {
> -        const DOMProxyHandler* handler = GetDOMProxyHandler(obj);
> -
> -        bool done;
> -        if (!handler->setCustom(cx, obj, id, vp, &done))

So the idea is that we basically never want OverrideBuiltins over Xrays, so we don't want to do the setCustom stuff at all?

Comment 85

[Bobby Holley (:bholley)]

Comment on attachment 8493765 [details] [diff] [review]
Remove obsolete code v1

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

\o/

Comment 86

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #84)
> So the idea is that we basically never want OverrideBuiltins over Xrays, so
> we don't want to do the setCustom stuff at all?

Proxy::set calls getPropertyDescriptor(...)/defineProperty(...) for proxies with a prototype, never set(...), so the code in DOMXrayTraits::set becomes unused. The getPropertyDescriptor() call also makes us ignore OverrideBuiltins.

Comment 87

[Peter Van der Beken [:peterv]]

Attachment: Add a named properties object type to DOMObjectType and how to get the parent prototype object in DOMIfaceAndProtoJSClass v2

Addresses comment 82, so has r=bz and r=bholley.

Comment 88

[Peter Van der Beken [:peterv]]

Attachment: Make Xrays walk the prototype chain when resolving DOM properties v2.1

Operations are mostly how WebIDL calls what you and I call methods :-).

Comment 89

[Bobby Holley (:bholley)]

Comment on attachment 8494706 [details] [diff] [review]
Make Xrays walk the prototype chain when resolving DOM properties v2.1

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

I might be misunderstanding something. Feel free to re-request if I've got it wrong.

::: dom/bindings/BindingUtils.cpp
@@ +1288,1 @@
>    if (type == eInstance) {

For globals, I think we still want to be resolving native properties first before resolving own properties, right? Otherwise the target could shadow arbitrary native properties.

@@ +1510,5 @@
>      ENUMERATE_IF_DEFINED(staticMethod);
>      ENUMERATE_IF_DEFINED(staticAttribute);
>    } else {
>      MOZ_ASSERT(type == eInterfacePrototype);
> +    if (!isGlobalAndPropertiesAreOwn) {

Isn't isGlobal only true if type == eInstance? In the first patch, we have:

isGlobal = (clasp->flags & JSCLASS_DOM_GLOBAL) != 0;

That isn't true for DOMIfaceAndProtoJSClass AFAICT.

@@ +1601,2 @@
>      if (enumerateOwnProperties &&
>          !enumerateOwnProperties(cx, wrapper, obj, props)) {

Does this cover unforgeable properties now? If not, where do those get enumerated?

@@ +1605,3 @@
>    }
>  
> +  return (type == eInterfacePrototype && isGlobal && GlobalPropertiesAreOwn()) ||

Same question here. I don't understand how the first part can ever be true.

Comment 90

[Peter Van der Beken [:peterv]]

Attachment: Make Xrays walk the prototype chain when resolving DOM properties v2.2

This undoes most of attachment 8489543 [details] [diff] [review], but I'm not going to remove that from the queue because it means changing too many of the other patches.

Comment 91

[Peter Van der Beken [:peterv]]

Attachment: Add tests for the prototype chain v1

Comment 92

[Peter Van der Beken [:peterv]]

Comment on attachment 8495968 [details] [diff] [review]
Make Xrays walk the prototype chain when resolving DOM properties v2.2

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

::: dom/bindings/BindingUtils.cpp
@@ -1492,2 @@
>      ENUMERATE_IF_DEFINED(unforgeableMethod);
>      ENUMERATE_IF_DEFINED(unforgeableAttribute);

> Does this cover unforgeable properties now? If not, where do those get
> enumerated?

Here.

Comment 93

[Peter Van der Beken [:peterv]]

Comment on attachment 8494706 [details] [diff] [review]
Make Xrays walk the prototype chain when resolving DOM properties v2.1

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

::: dom/bindings/BindingUtils.cpp
@@ +1288,1 @@
>    if (type == eInstance) {

Actually, for globals the named properties live on the named properties object, not on the instance, so this should deosn't return the named properties. Window has a resolveOwnProperty hook too, but it's used for resolving the DOM classes which is totally fine.
For non-globals we explicitly filter out stuff that shadows WebIDL properties in their getPropertyDescriptor hook when called with an Xray.

I don't think I really changed this part of the logic.

Comment 94

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #89)
> Isn't isGlobal only true if type == eInstance? In the first patch, we have:

I dealt with this by adding another DOMObjectType.

Comment 95

[Bobby Holley (:bholley)]

Comment on attachment 8495968 [details] [diff] [review]
Make Xrays walk the prototype chain when resolving DOM properties v2.2

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

::: dom/bindings/BindingUtils.cpp
@@ -1494,5 @@
>    } else if (type == eInterface) {
>      ENUMERATE_IF_DEFINED(staticMethod);
>      ENUMERATE_IF_DEFINED(staticAttribute);
> -  } else {
> -    MOZ_ASSERT(type == eInterfacePrototype);

Can we preserve this assert as, IIUC, MOZ_ASSERT(IsInterfacePrototype(type))?

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1719,5 @@
>          return false;
>      }
>  
>      RootedObject obj(cx, XrayTraits::getTargetObject(wrapper));
>      if (UseDOMXray(obj)) {

Please be careful rebasing this part on my recent patch.

Comment 96

[Bobby Holley (:bholley)]

Comment on attachment 8495969 [details] [diff] [review]
Add tests for the prototype chain v1

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

Land-ho!

::: dom/bindings/test/test_dom_xrays.html
@@ +80,5 @@
> +  // Named properties live on the named properties object for global objects.
> +  checkWindowXrayProperty(win, "iframe", undefined, undefined, doc.getElementById("iframe").contentWindow);
> +  ok("iframe" in win, "Named properties should be exposed through Xrays");
> +
> +  // WebIDL-defined properties live on the instance, shadowing the properties of the named property object.

I'd say "Window properties"

@@ +99,5 @@
> +  // Named properties shouldn't shadow WebIDL- or ECMAScript-defined properties.
> +  checkWindowXrayProperty(win, "addEventListener", undefined, undefined, undefined, eventTargetProto.addEventListener);
> +  ise(win.addEventListener, eventTargetProto.addEventListener, "Named properties shouldn't shadow WebIDL-defined properties");
> +
> +  ise(win.toString, win.Object.prototype.toString, "Named properties shouldn't shadow ECMAScript-defined properties");

Hm how does this actually end up getting enforced? Via the HasPropertyOnPrototype stuff?

Comment 97

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #96)
> Hm how does this actually end up getting enforced? Via the
> HasPropertyOnPrototype stuff?

Yes, exactly. Thanks for your patience and all the reviews!

Comment 98

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c47153a1dd1
https://hg.mozilla.org/integration/mozilla-inbound/rev/adfe4ac95437
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8655117616
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2258c53e599
https://hg.mozilla.org/integration/mozilla-inbound/rev/b46397f84e13
https://hg.mozilla.org/integration/mozilla-inbound/rev/666f2599f996
https://hg.mozilla.org/integration/mozilla-inbound/rev/591364ca5f0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/024db535ce4d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2d0c477f64
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7cb38405fc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef7fe194800
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee4fe6bc73be
https://hg.mozilla.org/integration/mozilla-inbound/rev/194b0d93d32a
https://hg.mozilla.org/integration/mozilla-inbound/rev/73f22a0e55cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7efcdcec493b
https://hg.mozilla.org/integration/mozilla-inbound/rev/090b62fdfd21

Comment 99

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/adfe4ac95437
https://hg.mozilla.org/mozilla-central/rev/6e8655117616
https://hg.mozilla.org/mozilla-central/rev/f2258c53e599
https://hg.mozilla.org/mozilla-central/rev/b46397f84e13
https://hg.mozilla.org/mozilla-central/rev/666f2599f996
https://hg.mozilla.org/mozilla-central/rev/591364ca5f0a
https://hg.mozilla.org/mozilla-central/rev/024db535ce4d
https://hg.mozilla.org/mozilla-central/rev/7f2d0c477f64
https://hg.mozilla.org/mozilla-central/rev/d7cb38405fc2
https://hg.mozilla.org/mozilla-central/rev/3ef7fe194800
https://hg.mozilla.org/mozilla-central/rev/ee4fe6bc73be
https://hg.mozilla.org/mozilla-central/rev/194b0d93d32a
https://hg.mozilla.org/mozilla-central/rev/73f22a0e55cb
https://hg.mozilla.org/mozilla-central/rev/7efcdcec493b
https://hg.mozilla.org/mozilla-central/rev/090b62fdfd21