1041731 - Unforgeable Xrayed methods aren't getting cached on the holder

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

This is a follow up bug after: bug 1038432

the problem is:
content.document.location.toString != content.document.location.toString

Comment 1

[Bobby Holley (:bholley)]

This is a regression from bug 832014, with the potential to affect addon-compat. We should track.

Comment 2

[Bobby Holley (:bholley)]

So the general issue here is that the current mechanism for deciding whether or not to cache something on the holder is broken. We currently decide it based on whether the property is |own| or not, but that's wrong in the case of Unforgeable attributes.

Peter, I know you and I talked about this back in our vidyo call in January, and that you presumably have a fix for this embedded in your patches in bug 787070. Do you have a subset of that you could split out and land here (for m-c) and uplift to aurora?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Regression + compat issue. Tracking

Comment 4

[Peter Van der Beken [:peterv]]

(In reply to Bobby Holley (:bholley) from comment #2)
> Peter, I know you and I talked about this back in our vidyo call in January,
> and that you presumably have a fix for this embedded in your patches in bug
> 787070. Do you have a subset of that you could split out and land here (for
> m-c) and uplift to aurora?

I'll take a look at this today.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8475873 [details] [diff] [review]
v1

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

I'd ordinarily like to look at this again, but I'm about to head off on PTO, and this has some urgency. r=bholley with comments addressed, and another review from bz if the fix for the dynamic properties thing doesn't turn out to be as simple as I suggest.

::: dom/bindings/BindingUtils.cpp
@@ +989,1 @@
>  {

We should initialize cacheOnHolder to false here.

@@ +1034,5 @@
>        size_t i = attributes->specs - attributeSpecs;
>        for ( ; attributeIds[i] != JSID_VOID; ++i) {
>          if (id == attributeIds[i]) {
> +          cacheOnHolder = true;
> +          

Whitespace error.

@@ +1090,5 @@
>        size_t i = method->specs - methodSpecs;
>        for ( ; methodIds[i] != JSID_VOID; ++i) {
>          if (id == methodIds[i]) {
> +          cacheOnHolder = true;
> +          

Same here.

@@ +1278,5 @@
>                            const NativePropertyHooks* nativePropertyHooks,
>                            DOMObjectType type, JS::Handle<JSObject*> obj,
>                            JS::Handle<jsid> id,
> +                          JS::MutableHandle<JSPropertyDescriptor> desc,
> +                          bool& cacheOnHolder)

Here too.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +2107,5 @@
> +        return false;
> +    if (desc.object()) {
> +        desc.object().set(wrapper);
> +        return true;
> +    }

Doesn't this break nodelists and things with dynamic properties? If a dynamic property appears with the same name as one from the prototype (that was previously resolved), it needs to shadow it. With this, we won't, right? Please add some test coverage that would fail here.

The best approach I can think of is to move the lookup below the call to XrayResolveOwnProperty. If cacheOnHolder comes back true, we first do a lookup on the holder, and use any pre-existing property. If there isn't one, we cache the one we got.

It's super weird, messy, and inefficient, but thankfully this will all go away when we make the HasPrototype=1 switch.

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to (PTO 8/22 - 9/1) from comment #7)
> Doesn't this break nodelists and things with dynamic properties? If a
> dynamic property appears with the same name as one from the prototype (that
> was previously resolved), it needs to shadow it. With this, we won't, right?

It shouldn't shadow it, that would defeat the purpose of Xrays, wouldn't it? Afaict shadowing would allow content to control the view that chrome has through an Xray? We do explicit tests for !HasPropertyOnPrototype in the binding if we're resolving on an Xray to avoid shadowing WebIDL properties by named or indexed properties.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Peter Van der Beken [:peterv] from comment #8)
> It shouldn't shadow it, that would defeat the purpose of Xrays, wouldn't it?
> Afaict shadowing would allow content to control the view that chrome has
> through an Xray? We do explicit tests for !HasPropertyOnPrototype in the
> binding if we're resolving on an Xray to avoid shadowing WebIDL properties
> by named or indexed properties.

Oh hm that's a good point! Ignore that comment then.

Comment 10

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/634efd3b8784

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/634efd3b8784

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Peter, could you uplift this patch to 33? Thanks

Comment 13

[Sylvestre Ledru [:Sylvestre]]

(request an uplift) :)

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 8475873 [details] [diff] [review]
v1

Ugh, forgot that this needs to be in 33 (asking for aurora approval since we haven't merged yet afaict).

Approval Request Comment
[Feature/regressing bug #]: bug 832014
[User impact if declined]: possible addon breakage
[Describe test coverage new/current, TBPL]: has been on trunk for a while and has an automated testcase
[Risks and why]: low-risk
[String/UUID change made/needed]: None

Comment 15

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/073eb01d99b7