665279 - XPathResult's constants are undefined on XrayWrappers

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Alexandre Poirot [:ochameau]]

I've just found another bug on XrayWrappers while working on bug 601295 (proxies on top of xraywrappers for jetpack content scripts).

Here is a sample snippet to verify this in a JS console:

XPathResult.NUMBER_TYPE
=> 1

But:
top.opener.gBrowser.contentDocument.defaultView.XPathResult.STRING_TYPE
=> undefined

(top.opener.gBrowser.contentDocument.defaultView allows to get a xraywrapper for the current content document, do you know any easier way to get one xraywrapper for future tests?)

Comment 1

[Alexandre Poirot [:ochameau]]

I filled bug 665280 for the jetpack temporary work around.

Comment 2

[Gabor Krizsanits (INACTIVE)]

What I figured out so far is that the reason why the XrayWrapper cannot find the STRING_TYPE property in the example is that, the wrapped XPathResult is a DOM Prototype, and as such storing the values for constants like STRING_TYPE in it's slots. But when the XrayWrapper tries to find it on the XOW that wraps that DOM Prototype, it acts like a NativeWrapper (using callcontext and XPCNativeInterface based lookup) instead of doing a regular lookup for a regular property (nativeSearch on the JSObject).

I understand why XrayWrappers are needed, but for this specific DOM Prototypes I'm not sure they are really needed, since you cannot fetch a reference to any other object from the other compartment/domain from them (am I right here?)

What I think is that these DOM Prototypes are probably safe enough to avoid XRayWrappers on top of their XOW, and I think it was already flagged as WaiveXrayWrapperWrapper but I don't know where is it wrapped in an XrayWrapper in the end...

Anyway if any expert could give me some feedback about this, that would be great, there are way too many new concepts for me to deal here to be sure about my assumtions/conclusions...


(btw this works fine: 
XPCNativeWrapper.unwrap(top.opener.gBrowser.contentDocument.defaultView.XPathResult).STRING_TYPE )

Comment 3

[Gabor Krizsanits (INACTIVE)]

The WaiveXrayWrapperWrapper came from the XPCNativeWrapper.unwrap call, so my assumption that DOM Prototypes are safe not to xraywrapp is probably wrong.

So the question is, if those values stored in the slots of the DOM Prototype object are always safe to access, and if they are then what is the proper way to do so?

And one more note, which made me worry a bit... In some rare cases, the original top.opener.gBrowser.contentDocument.defaultView.XPathResult.STRING_TYPE works just fine... I could not find the reason for it, but once it worked, it works always and avoid any kind of wrapper code. When that happens, the top.opener.gBrowser.contentDocument.defaultView.XPathResult object has no wrapper around it, (nor xraywrapper nor XOW). It is very rare and very random when this happens...

Comment 4

[Dave Townsend [:mossop]]

Blake, can you take a look and give guidance here

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

I hope Gabor doesn't mind me stealing this from him. This fix took peterv and I deep into the bowls of nsNameSpaceEntry to figure out how to get the right IID out of a constructor.

What this fix does is that for XrayWrapped constructors, it resolves the interface constants directly in its own NewResolve hook. For non-wrapped constructors, this is taken care of by the DefineInterfaceConstant calls scattered throughout nsWindowSH::GlobalResolve and ResolvePrototype. The NewResolve hook is a little ugly because we don't have any way of knowing whether or not the id being looked up is one of the constants until after they have all been defined.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 561641 [details] [diff] [review]
Proposed fix

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

::: dom/base/nsDOMClassInfo.cpp
@@ +5859,5 @@
> +  } else {
> +    return NS_OK;
> +  }
> +
> +  return DefineInterfaceConstants(cx, obj, class_iid);

Shouldn't we do the special stuff for Event and IDBKeyRange here (see http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp?mark=5938-5950#5930)

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/781ed55e16dc