Closed Bug 787070 Opened 12 years ago Closed 10 years ago

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

Categories

(Core :: XPConnect, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: gkrizsanits, Assigned: peterv)

References

(Depends on 1 open bug)

Details

Attachments

(17 files, 14 obsolete files)

8.14 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.32 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.63 KB, patch
efaust
: review+
Details | Diff | Splinter Review
10.47 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
14.90 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.40 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
7.84 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.76 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.83 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.16 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.06 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.90 KB, patch
bholley
: review+
Details | Diff | Splinter Review
10.78 KB, patch
bholley
: review+
Details | Diff | Splinter Review
39.53 KB, patch
peterv
: review+
Details | Diff | Splinter Review
29.12 KB, patch
bholley
: review-
Details | Diff | Splinter Review
38.17 KB, patch
bholley
: review+
Details | Diff | Splinter Review
9.53 KB, patch
bholley
: review+
Details | Diff | Splinter Review
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?
I thought prototype mirroring was supposed to make this work, actually...
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"
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"
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"
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?
> we don't have any kind of useful Xrays into DOM prototypes. 

I believe Peter plans to change that for Paris prototypes, fwiw.
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?
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)
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.
Depends on: 778152
ETA on nodes being on new bindings is probably ~6 months.
Attached patch v1 (obsolete) — Splinter Review
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).
Assignee: nobody → peterv
Status: NEW → ASSIGNED
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.
Attachment #8359730 - Flags: review?(efaustbmo)
Attached patch Move some code around v1 (obsolete) — Splinter Review
This is just to make the next patch slightly clearer.
Attachment #8359731 - Flags: review?(bobbyholley+bmo)
Attachment #8359731 - Flags: review?(bobbyholley+bmo)
Did you mean to cancel review on the patch Peter?
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.
Attachment #8359730 - Flags: review?(efaustbmo) → review+
Attachment #8359731 - Attachment is obsolete: true
Attachment #8364994 - Flags: review?(bobbyholley+bmo)
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.
Attachment #720272 - Attachment is obsolete: true
Attachment #8364995 - Flags: review?(bobbyholley+bmo)
Attachment #8364994 - Flags: review?(bobbyholley) → review+
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.
Attachment #8364995 - Flags: review?(bobbyholley) → feedback+
I split out the changes that I need for bug 914970 into separate patches, which I've stamped and will attach here now.
Attachment #8364994 - Attachment description: Move some code around v2 → Part 1 - Move some code around v2
Maybe something more like this.
Attachment #8375022 - Attachment is obsolete: true
Attachment #8375165 - Flags: review?(peterv)
Attachment #8375165 - Flags: review+
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.
Attachment #8375165 - Attachment is obsolete: true
Attachment #8375165 - Flags: review?(peterv)
Attachment #8375023 - Attachment is obsolete: true
No longer blocks: CVE-2014-8636
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.
Depends on: 987111
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.
Flags: needinfo?(peterv)
Yeah, I've been trying to make it work again, but not there yet.
Flags: needinfo?(peterv)
Depends on: 1033927
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.
Depends on: 1034682
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?
Flags: needinfo?(jorendorff)
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
Depends on: 1040579
No longer depends on: 1040579
Depends on: 965898
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 ?
Flags: needinfo?(jorendorff) → needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
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.
Ah, I see. Then yeah, we can just add a check for own readonly value-props in Proxy::set.
Blocks: 989426
Depends on: 1067501
Attachment #8359730 - Attachment is obsolete: true
Attachment #8364994 - Attachment is obsolete: true
Attachment #8364995 - Attachment is obsolete: true
Attachment #8489543 - Flags: review?(bzbarsky)
Attachment #8489552 - Flags: review?(bobbyholley)
Attachment #8489567 - Flags: review?(bobbyholley)
Comment on attachment 8489543 [details] [diff] [review]
Make GetNativePropertyHooks detect global objects v1

r=me
Attachment #8489543 - Flags: review?(bzbarsky) → review+
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
Attachment #8489564 - Flags: review?(bzbarsky) → review+
So that we can easily look them up for Xrays.
Attachment #8489577 - Flags: review?(bzbarsky)
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.
Attachment #8489581 - Flags: review?(bobbyholley)
This is still rather big, but I don't think I can easily make it smaller.
Attachment #8489583 - Flags: review?(bobbyholley)
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.
Attachment #8489553 - Flags: review?(efaustbmo) → review+
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?
Attachment #8489572 - Flags: review?(bzbarsky) → review-
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?
Attachment #8489573 - Flags: review?(bzbarsky) → review+
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.
Attachment #8489574 - Flags: review?(bzbarsky) → review+
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.
Attachment #8489576 - Flags: review?(bzbarsky) → review+
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?
Attachment #8489577 - Flags: review?(bzbarsky) → review-
(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.
> 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 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.
Attachment #8489552 - Flags: review?(bobbyholley) → review+
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.
Attachment #8489567 - Flags: review?(bobbyholley) → review+
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.
Attachment #8489581 - Flags: review?(bzbarsky)
Attachment #8489581 - Flags: review?(bobbyholley)
Attachment #8489581 - Flags: review+
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.
Attachment #8489582 - Flags: review?(bobbyholley) → review+
Blocks: 1068614
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?
Attachment #8489581 - Flags: review?(bzbarsky) → review-
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/
Attachment #8489583 - Flags: review?(bobbyholley) → review-
(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|?
> I guess you mean |Object.getPrototypeOf(A) == B|?

Er, yes.
(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.
(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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Oh, and I'd rather we didn't cache the JS protos in ProtoAndIfaceCache, I think.
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.
Flags: needinfo?(bobbyholley)
Attachment #8489577 - Attachment is obsolete: true
Attachment #8493722 - Flags: review?(bzbarsky)
(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().
Attachment #8489581 - Attachment is obsolete: true
Attachment #8493724 - Flags: review?(bobbyholley)
Attachment #8493724 - Flags: review?(bzbarsky)
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.
Attachment #8493724 - Attachment is obsolete: true
Attachment #8493724 - Flags: review?(bzbarsky)
Attachment #8493724 - Flags: review?(bobbyholley)
(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.
Attachment #8489583 - Attachment is obsolete: true
Attachment #8493760 - Flags: review?(bobbyholley)
(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.
DOMXrayTraits::set is unused because DOMXrayTraits always have HasPrototype == 1 now.
Attachment #8493763 - Flags: review?(bobbyholley)
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].
Attachment #8493765 - Flags: review?(bobbyholley)
(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 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()?
Attachment #8493722 - Flags: review?(bzbarsky) → review+
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
Attachment #8493726 - Flags: review?(bzbarsky) → review+
(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?
Flags: needinfo?(bobbyholley)
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?
Attachment #8493726 - Flags: review?(bobbyholley) → review+
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).
Attachment #8493760 - Flags: review?(bobbyholley)
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?
Attachment #8493763 - Flags: review?(bobbyholley) → review+
Comment on attachment 8493765 [details] [diff] [review]
Remove obsolete code v1

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

\o/
Attachment #8493765 - Flags: review?(bobbyholley) → review+
(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.
Depends on: 1072482
Operations are mostly how WebIDL calls what you and I call methods :-).
Attachment #8493760 - Attachment is obsolete: true
Attachment #8494706 - Flags: review?(bobbyholley)
Attachment #8494700 - Flags: review+
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.
Attachment #8494706 - Flags: review?(bobbyholley) → review-
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.
Attachment #8495968 - Flags: review?(bobbyholley)
Attachment #8495969 - Flags: review?(bobbyholley)
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 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.
(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 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.
Attachment #8495968 - Flags: review?(bobbyholley) → review+
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?
Attachment #8495969 - Flags: review?(bobbyholley) → review+
(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!
Depends on: 1074863
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: