706301 - calling Object.getOwnPropertyDescriptor(xraywrapped_htmlcollection, 'length') makes 'length' subsequently unresolvable

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I ran into this over in my wrapper in bug 702353. Test coming right up.

Comment 1

[Bobby Holley (:bholley)]

Attachment: tests v1

Attaching a test case. Note that things work just fine when content inspects its own object, meaning that the nastiness probably lies in the intersection of the two proxies (Xray Wrapper and nodelist).

Comment 2

[Boris Zbarsky [:bzbarsky]]

So what's the failure mode?  Does nodelist['length'] end up throwing in getLengthInChrome or something?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> So what's the failure mode?  Does nodelist['length'] end up throwing in
> getLengthInChrome or something?

It returns 'undefined'. If I don't call Object.getOwnPropertyDescriptor beforehand, it returns 0.

This might be related to something in bug 520882, where the prototype shows up as having value: undefined rather than having a getter. Or it might be something completely different.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, OK.

So we're looking at XrayProxy here, not XrayWrapper.  

I would think we're landing in XrayProxy::getOwnPropertyDescriptor on the first call and then in XrayProxy::getPropertyDescriptor on the second call.  The main difference seems to be whether we cache the desc on the holder, and the handling of the Transparent() case.  Well, and the weird separate call to getOwnPropertyDescriptor in the getPropertyDescriptor code.

If you have this applied in your tree already, could you make sure we do in fact call those two methods in this order and check whether we're following the Transparent() codepath or not here?

Comment 5

[Bobby Holley (:bholley)]

So, the bug here is that XrayProxy::getOwnPropertyDescriptor defines the property descriptor onto the holder even if the property descriptor wasn't found:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp?rev=d252e090c6cf#1210

The simple fix here is to prevent the property from being defined on the holder if desc->obj == NULL. But this caused bz to wonder why we even have the holder at all.


From what I can tell, we don't need it. We do need some sort of expando object for defineProperty purposes, but it should be called expandoObject, not holder, to be consistent with the usage in XrayWrapper.

I need to think for a bit about whether there's a good way to make all of this stuff more sane.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Also, I was wondering why getPropertyDescriptor and getOwnPropertyDescriptor always return a descriptor with a non-null obj, even when the property is not found...

Comment 7

[Bobby Holley (:bholley)]

Attachment: part 1 - Don't define proxy-resolved properties onto the XrayProxy holder. v1

This patch stops us from caching property lookups onto the holder.

This patch, understandably, causes us to fail this test: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/tests/chrome/test_nodelists.xul#26

More on that below.

Comment 8

[Bobby Holley (:bholley)]

Attachment: part 2 - Rename confusing XrayProxy 'holder' to 'proxy expando'. v1

The name 'holder' is very confusing here, because it's used in a very different context in XrayWrapper. Our use of this object is much more akin to the use of the 'expando' object in XrayWrapper, especially given the preceding patch.

Comment 9

[Bobby Holley (:bholley)]

Attachment: part 3 - Respect the 'create' flag, which is used in delete_. v1

This one is very straightforward.

Comment 10

[Bobby Holley (:bholley)]

So here's the state of affairs:

Currently, the 'holder' object of XrayProxy is a bit of a confusing case. It's somewhere between the 'holder' and 'expando' properties of XrayWrapper. We use it as a receptacle for XrayProxy::defineProperty (the 'expando' role, which makes total sense), and we also use it to store properties resolved off of the referent.

The second role has two problems. First, there's the very basic bug I described in comment 5. This is trivial to fix without any restructuring. Secondly though, bz pointed out that this is likely to do the wrong thing when the DOM changes: nodeList[n] is a value prop, rather than an accessor prop, so caching it means that we're blind to future updates in the underlying nodelist. I haven't verified this with a testcase, so it's possible that there's something bz and I missed.

Regardless, it seemed like it was worth a try to rip out the caching, which is the attachment in comment 7. Unfortunately, this caused a test failure, which seems to be specifically checking for this kind of caching: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/tests/chrome/test_nodelists.xul#26

What's strange to me about this test is that it only seems to exist as a chrome test, meaning that we test only the XrayProxy behavior and not the behavior seen by the web. This seems bad from a performance standpoint, since we probably care a lot about this sort of caching on the web, but maybe not at all for chrome.

I tried changing the above test to use waivers (via contentWindow.wrappedJSObject), and I got two failures:

6 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_nodelists.xul | list[2] exists
8 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/js/xpconnect/tests/chrome/test_nodelists.xul | remove last paragraph element - got [object HTMLParagraphElement @ 0x1184d3ef0 (native @ 0x118f33500)], expected [object HTMLParagraphElement @ 0x1184d3ef0 (native @ 0x118f33500)]

I'm hoping that the above failures have to do with CrossOriginWrapper (waived Xray), rather than the new nodelist bindings themselves, but it's probably worth looking into either way.

Also, XrayProxy doesn't currently support .wrappedJSObject or any other bits of magic we get from XrayWrapper::resolveOwnProperty, which is probably something we want to fix on the soon side to avoid totally confusing firefox/extension developers.

Anyway, I've hit the point where I have a lot of questions that need answering before I can proceed. Since peterv's out, I'm really hoping that mrbkap can weigh in on this stuff:

* In general, what's the way forward? Should we take my patches and remove that line of the test? Should we implement this kind of caching in the underlying nodelist implementation in dombindings.cpp? Should we add test coverage to test what content sees?

* Is there any reason for the XrayProxy caching other than optimization? How much do we care about performance for XrayProxy, and how much difference does it make?

* Do we plan on using proxies for the rest of the new DOM bindings, or will it be restricted to a few special cases like nodelist?

* For XrayWrapper, why do we even have the holder vs expando distinction?

Comment 11

[Boris Zbarsky [:bzbarsky]]

(In reply to Bobby Holley (:bholley) from comment #10)
> Regardless, it seemed like it was worth a try to rip out the caching, which
> is the attachment in comment 7. Unfortunately, this caused a test failure,

Aha!  That's why the caching is there!  Hmm...  So the idea is that .item should not change under you, which is entirely sensible, not just a performance optimization.

> What's strange to me about this test is that it only seems to exist as a
> chrome test, meaning that we test only the XrayProxy behavior and not the
> behavior seen by the web. This seems bad from a performance standpoint,
> since we probably care a lot about this sort of caching on the web

On the web, things like .item are "cached" on the prototype.

But for the special case of an XrayProxy around a nodelist, we end up in ListBase<LC>::resolveNativeName (only called for the proxy case!) which always creats a new object.  So we need to cache that somewhere to pass that test assertion.

What we should probably do here is only cache non-own properties we get off the underlying nodelist from the holder.  That depends on the assumption that any own properties the Xray sees are either accessors or value props where the value is not created anew every time.

The problem is that resolveNativeName always claims to find props on the nodelist proxy itself...  Maybe that's what the getOwnPropertyDescriptor bit in XrayProxy::getPropertyDescriptor is about?  We want to do that, and if we hit not cache it on the holder.  Otherwise, do the get and cache it.  The caching would only need to happen in XrayProxy::getPropertyDescriptor, not in XrayProxy::getOwnPropertyDescriptor.

The other option would be for the nodelist to cache the functions somewhere, but that needs an extra slot to store the object to cache them on in, right?

> I'm hoping that the above failures have to do with CrossOriginWrapper
> (waived Xray)

Yeah, I'd think so.  Still kinda weird.

So I think the above answers some of your questions, if imperfectly.

> * Do we plan on using proxies for the rest of the new DOM bindings, or will
> it be restricted to a few special cases like nodelist?

The latter.  It's only needed for things that have indexgetters and maybe namegetters.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Don't cache own properties on XrayProxy. v1

Thanks for the analysis bz! I've attached such a patch. Flagging mrbkap for review.

> That depends on the assumption that any own properties the Xray
> sees are either accessors or value props where the value is not
> created anew every time.

Though I'm taking your word for it with the patch, I'm curious about this assumption. In particular, you mentioned that nodeList[n] is a value prop. Does that mean that it's not 'own'? Where is this specified? Is there a standardization of what lives where on the prototype chain?

Also, I'm still curious about the holder/expando distinction on XrayWrapper.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Does that mean that it's not 'own'?

It's 'own', but the value is a DOM node, not a new object every time.  The .item case ends up creating a new function object every time in ListBase<LC>::resolveNativeName.

> Where is this specified?

http://dev.w3.org/2006/webapi/WebIDL/#indexed-and-named-properties in this case.  Well, that and the IDL for nodelists at http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#interface-nodelist

> Is there a standardization of what lives where on the prototype chain?

At this point, yes.  The combination of the WebIDL spec and the actual IDL for the interface should fully specify this.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Bobby Holley (:bholley) from comment #10)
> Should we add test coverage to test what content sees?

Yes :)

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bobby Holley (:bholley) from comment #10)
> * For XrayWrapper, why do we even have the holder vs expando distinction?

expando objects stay around after the XrayWrapper itself might have died. This is so that expandos set on content objects "come back" even if the security wrapper itself got collected. This is actually important since it's a case that happens often enough that when we broke it, someone actually filed a bug on us.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 578175 [details] [diff] [review]
Don't cache own properties on XrayProxy. v1

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1145,1 @@
>      if (!js::GetProxyHandler(obj)->getOwnPropertyDescriptor(cx, wrapper, id, set, desc))

Given this change, calling JS_GetPropertyDescriptorById on the holder is no longer correct if it happens before the call to getOwnPropertyDescriptor (otherwise, a prototype property would be able to shadow an own property).

So I think that the check for properties on holder needs to move down after this.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Don't cache own properties on XrayProxy. v2

Good catch. Attaching an updated patch, reflagging for review.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #15)
> (In reply to Bobby Holley (:bholley) from comment #10)
> > * For XrayWrapper, why do we even have the holder vs expando distinction?
> 
> expando objects stay around after the XrayWrapper itself might have died.
> This is so that expandos set on content objects "come back" even if the
> security wrapper itself got collected. This is actually important since it's
> a case that happens often enough that when we broke it, someone actually
> filed a bug on us.

Ok, that makes sense. So what are the special needs of the holder object that prevent us from using the expando object for both expando and holder duties?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bobby Holley (:bholley) from comment #18)
> Ok, that makes sense. So what are the special needs of the holder object
> that prevent us from using the expando object for both expando and holder
> duties?

Neither gal nor I remember. We think that it might have to do with avoiding nasty cycle collector loops through the holder (allowing the wrapper to get cleaned up properly but allowing us to maintain expando properties anyway). You're welcome to try to merge them and see what happens, though!

Comment 20

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #19)

> You're welcome to try to merge them and see what happens, though!

Maybe one of these days... ;-)

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d6049bb1770d
http://hg.mozilla.org/integration/mozilla-inbound/rev/1e14abc06ad7

Comment 21

[Bobby Holley (:bholley)]

Also, for reference, this had a green try run: https://tbpl.mozilla.org/?tree=Try&rev=73a6ab6c9e91

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1e14abc06ad7
https://hg.mozilla.org/mozilla-central/rev/d6049bb1770d