Closed Bug 1041731 Opened 5 years ago Closed 5 years ago

Unforgeable Xrayed methods aren't getting cached on the holder

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 + fixed
firefox34 + fixed

People

(Reporter: gkrizsanits, Assigned: peterv)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

This is a follow up bug after: bug 1038432

the problem is:
content.document.location.toString != content.document.location.toString
Flags: needinfo?(bobbyholley)
Blocks: 1038432
Flags: needinfo?(bobbyholley)
Summary: Xrayed functions aren't getting cached on the holder → Unforgeable Xrayed methods aren't getting cached on the holder
This is a regression from bug 832014, with the potential to affect addon-compat. We should track.
Blocks: 832014
Keywords: regression
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?
Flags: needinfo?(peterv)
Regression + compat issue. Tracking
(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.
Flags: needinfo?(peterv)
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attached patch v1Splinter Review
Attachment #8475450 - Attachment is obsolete: true
Attachment #8475873 - Flags: review?(bobbyholley)
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.
Attachment #8475873 - Flags: review?(bobbyholley) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/634efd3b8784
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1059134
Peter, could you uplift this patch to 33? Thanks
Flags: needinfo?(peterv)
(request an uplift) :)
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
Attachment #8475873 - Flags: approval-mozilla-aurora?
Flags: needinfo?(peterv)
Attachment #8475873 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1086993
You need to log in before you can comment on or make changes to this bug.