Closed Bug 665279 Opened 12 years ago Closed 12 years ago

XPathResult's constants are undefined on XrayWrappers


(Core :: XPConnect, defect)

Not set





(Reporter: ochameau, Assigned: mrbkap)



(1 file)

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:

=> 1

=> 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?)
I filled bug 665280 for the jetpack temporary work around.
Assignee: nobody → gkrizsanits
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 )
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...
Blake, can you take a look and give guidance here
Attached patch Proposed fixSplinter Review
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.
Assignee: gkrizsanits → mrbkap
Attachment #561641 - Flags: review?(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
Attachment #561641 - Flags: review?(peterv) → review+
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.