So the general issue I discovered is described here: https://bugzilla.mozilla.org/show_bug.cgi?id=658909#c18 A simple way to reproduce is a plain html page with a script like: alert(document.documentElement.getAttributeNode.call( document.createElement('div'))); ^ this will throw since it won't be able to find the nsIDOMHHTMLhtmlElement on the div and xpc thinks that the method is implemented by that interface. getAttributeNode can be any non quickstubbed nsIDOMElement function (there aren't many) like mozRequestFullScreen or mozRequestPointerLock Quick stubbing these would fix this problem for now, but the problem is that xraywrapper do not support quick stubs, but it's falling back the the default xpc call path even for quick stubbed methods.

Yes, this is one of the known issues with XPConnect that we're fixing by moving to new DOM bindings...

(In reply to Boris Zbarsky (:bz) from comment #1) > Yes, this is one of the known issues with XPConnect that we're fixing by > moving to new DOM bindings... That's great news, I was hoping that the new bindings are aware of this flaw. Can you guestimate when will the new dom bindings used for element? (so when can we use it for mozMatchesSelector) Because I honestly don't think I can just fix this one that easily in a sane way (not loosing a lot of performance and what not)... so we likely have to wait for the new DOM bindings.

For Element, the timeframe is probably a few months... as in, I doubt it will happen this month, and I really hope it's done before end of summer. However note that the code quoted in comment 0 relies on it being done not just for Element but for HTMLHTMLElement as well. Converting all element subclasses is likely to take longer. Is the basic issue we're trying to deal with just the one described in bug 658909?

(In reply to Boris Zbarsky (:bz) from comment #3) > Is the basic issue we're trying to deal with just the one described in bug > 658909? Yes. Except, that it's the last remaining xray issue I need to get rid of from the ones listed in bug 738244. And then we could remove a big proxy layer from the addon sdk that is introduced to work around all kind of bugs/issues in xray wrappers. That layer is a huge performance bottle neck for addons unfortunately. So I might look into this if I can find something that solves our problem and don't cause any big issues (and I don't have to reimplement xpc call mechanism...), but also will try to find a js based workaround only for mozMatchesSelector in the meanwhile.

Is the issue that the sdk itself is grabbing methods from some nodes and applying them to other nodes? Or does it need to support consumer scripts doing that?

(In reply to Boris Zbarsky (:bz) from comment #5) > Is the issue that the sdk itself is grabbing methods from some nodes and > applying them to other nodes? Or does it need to support consumer scripts > doing that? The later one. Actually it needs to support external libraries and what not that does that. I'll CC Alex he knows more about the details.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6) > (In reply to Boris Zbarsky (:bz) from comment #5) > > Is the issue that the sdk itself is grabbing methods from some nodes and > > applying them to other nodes? Or does it need to support consumer scripts > > doing that? > > The later one. Actually it needs to support external libraries and what not > that does that. I'll CC Alex he knows more about the details. +1 We, in sdk codebase, are not doing this. The issue is for content scripts. (you know our equivalent of greasemonkey scripts) These scripts can include any possible JS web framework. And a well known JS framework like sizzle, which is used by jquery was doing crazy things like this: https://github.com/jquery/sizzle/blob/423f35af8bf43f3f07bb17284e21608f08372c22/sizzle.js#L1250-1274 It works in the web page, but not in content scripts as they are using xraywrappers. (We fixed this issue by using JS proxies on top of xrays which traps mozMatchesSelector method. So that we fixed this xpconnect issue only for this particular method)

Ah, I see. Yeah, I'm not sure I see a sane short-term solution here. :(

Today, we are only fixing this issue for mozMatchesSelector in our js proxies, so I was wondering if we can do the same here? We would like to get rid of this complex/slow-as-hell JS proxy code and this issue is a blocker. So it would be really cool if we can come up with a specific workaround for this method. This workaround can be made by patching c++ code, or using another kind of workaround in our JS code, but without involving these proxies. I have absolutely no idea what new DOM bindings are, but I was wondering if we can move this particular method to new DOM bindings??

(In reply to Alexandre Poirot (:ochameau) from comment #9) > Today, we are only fixing this issue for mozMatchesSelector in our js > proxies, so I was wondering if we can do the same here? > We would like to get rid of this complex/slow-as-hell JS proxy code and this > issue is a blocker. So it would be really cool if we can come up with a > specific workaround for this method. This workaround can be made by patching > c++ code, or using another kind of workaround in our JS code, but without > involving these proxies. Most realistically we need to find another JS based workaround only for this one. I will look into this, but the last time I checked I saw many ways to fix this but all of them either caused serious performance drop in xpc calling which is not acceptable, or requires a new call mechanism model. Which would be a huge project and luckily the new DOM binding is exactly that. > I have absolutely no idea what new DOM bindings are, but I was wondering if > we can move this particular method to new DOM bindings?? So DOM binding is the code path between a javascript based method call on a DOM node and the actual corresponding c++ implementation of that method. The naive way would require a lot of query interface and look-ups, but there is a lot of optimization, code generation and what not involved to make this code faster. So as far as I understood it, applying the new version on mozMatchesSelector, would require to apply it on Element and Node which is a lot of work and will take quite some time.

You only need this for this one method? You can't move a single method to new bindings: a binding is the JS implementation of the entire object and its proto chain. But what you _could_ do is special-case this one method in XrayWrapper, I would think. We already have a few properties that are implemented solely in XrayWrapper (baseURIObject, nodePrincipal, documentURIObject). You could add a similar branch for mozMatchesSelector which simply QIs the C++ object to the right thing and makes the call. Gabor, want to give that a shot?

(In reply to Boris Zbarsky (:bz) from comment #11) > Gabor, want to give that a shot? Sure, I think it worth a try, thanks for the idea!

So this is what I could come up with. It's a bit different from the other cases since this one is a method and not a property. So I implemented a native helper function that does the QI, then wrap that into a js function object and return that from resolveOwnProperty (ofc only if the wrapped object QIs to a DOMElement). Boris, does this look ok? Also, do you want to review this or shall I ask bholley?

Comment on attachment 633983 [details] [diff] [review] Bug 763897 - Incorrect behaviour of mozMatchesSelector.call through xray >+ XPCWrappedNative *wn = NULL; Just declare it on the assignment line. >+ JSObject *scope = JS_GetGlobalForScopeChain(cx); This is unused. Looks generally ok to me, but check with bholley whether creating a new function object on every get is ok? Again, it seems fine to me for now.

So I tested the previous version and it turned out that we need this in the content vs content case as well (so not only for chrome vs content xray wrrappers).

Comment on attachment 635301 [details] [diff] [review] Incorrect behaviour of mozMatchesSelector.call through xray Didn't look at this very closely, but seems reasonable in general. Needs some comments explaining what's going on, though.

I did a few changes. I moved the whole logic to resolveNativeProperty, so now it should not create a function object at each call, and the property can be shadowed with an expendo on the xray (which is expected). I had to move the "Is" function a few lines up for this. Added some more tests and comments.

Comment on attachment 637115 [details] [diff] [review] Incorrect behaviour of mozMatchesSelector.call through xray This looks nice and cleanly done. I didn't look too much at the guts (since I'm assuming Blake did), but from an architectural/future-of-xray-wrappers perspective, it looks fine. r=bholley >+ // especially through xray wrappers. This is a temoorary work around for 'temporary'.

Er, whoops, looks like I was confused. It was bz who looked at this before, not blake.

Comment on attachment 637115 [details] [diff] [review] Incorrect behaviour of mozMatchesSelector.call through xray >+ JSFunction *fun = JS_NewFunction(cx, mozMatchesSelectorStub, >+ 1, 0, JS_GetPrototype(wrapper), >+ "mozMatchesSelector"); Also, why are we parenting to the prototype of the wrapper here? If this is a per-wrapper function (and not JSPROP_SHARED), seems like we should parent it directly to the wrapper. And yeah, looks good otherwise.

(In reply to Bobby Holley (:bholley) from comment #20) > Also, why are we parenting to the prototype of the wrapper here? If this is > a per-wrapper function (and not JSPROP_SHARED), seems like we should parent > it directly to the wrapper. That is actually a great thing that you noticed and brought that up, I should have talked about that one just forgot. This is a long story. So there are two reasons. One that the wrapper there is not the wrapper for the function object but the wrapper for the object the method belongs. If I do var mozMSF = someElement.mozMatchesSelector, the mozMSF should not really depend on the wrapper of the someElement imo. If this is not convincing enough here is the practical reason. It all starts at the XPCWrappedNative::GetWrappedNativeOfJSObject... Every helper function we create in xray wrapper, if you check it, is parented to the wrapper. The wrapper of the owner object. And because of this in GetWrappedNativeOfJSObject it will as a side effect bound to this wrapped object we fetched it from. So if you call the method with .call or .bind and passing a different |this| like mozMSF.call(someOtherObj), someOtherObj will be simply ignored and the method will be called on the wrapped object. http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp#1785 Here it will fall into the first case if we pass the proto, into the second one if we pass the wrapper... It is a hack that should be fixed, I haven't yet, because fixing it didn't solve my original problem since xpc calling mechanism is the other blocker in this bug report... I had some talks about Blake about this and his idea was to pass in the proto of the wrapper instead, which can probably work, and after this bug I might go and give it a try to fix that bug as well. (This was my first starter bug when I came here...)

Ok, let's make an effort to fix that bug. In the mean time, please add a comment that links to your post above.

https://tbpl.mozilla.org/?tree=Try&rev=4ae76842adf1 https://hg.mozilla.org/integration/mozilla-inbound/rev/d36a98542b01