Last Comment Bug 763897 - general XPC calling mechanism cannot handle interface inheritance in some cases (call, bind)
: general XPC calling mechanism cannot handle interface inheritance in some cas...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 791845 798011
Blocks: 658909
  Show dependency treegraph
 
Reported: 2012-06-12 06:16 PDT by Gabor Krizsanits [:krizsa :gabor]
Modified: 2012-10-04 12:52 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug 763897 - Incorrect behaviour of mozMatchesSelector.call through xray (8.94 KB, patch)
2012-06-18 03:45 PDT, Gabor Krizsanits [:krizsa :gabor]
bzbarsky: feedback+
Details | Diff | Splinter Review
Incorrect behaviour of mozMatchesSelector.call through xray (9.09 KB, patch)
2012-06-21 07:25 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: feedback+
Details | Diff | Splinter Review
Incorrect behaviour of mozMatchesSelector.call through xray (11.27 KB, patch)
2012-06-27 07:34 PDT, Gabor Krizsanits [:krizsa :gabor]
bobbyholley: review+
Details | Diff | Splinter Review

Description Gabor Krizsanits [:krizsa :gabor] 2012-06-12 06:16:20 PDT
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.
Comment 1 Boris Zbarsky [:bz] (TPAC) 2012-06-12 06:41:23 PDT
Yes, this is one of the known issues with XPConnect that we're fixing by moving to new DOM bindings...
Comment 2 Gabor Krizsanits [:krizsa :gabor] 2012-06-12 08:11:08 PDT
(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.
Comment 3 Boris Zbarsky [:bz] (TPAC) 2012-06-12 11:18:28 PDT
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?
Comment 4 Gabor Krizsanits [:krizsa :gabor] 2012-06-12 12:05:10 PDT
(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.
Comment 5 Boris Zbarsky [:bz] (TPAC) 2012-06-12 12:22:02 PDT
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?
Comment 6 Gabor Krizsanits [:krizsa :gabor] 2012-06-12 13:16:35 PDT
(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.
Comment 7 Alexandre Poirot [:ochameau] 2012-06-12 13:34:48 PDT
(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)
Comment 8 Boris Zbarsky [:bz] (TPAC) 2012-06-12 18:22:35 PDT
Ah, I see.  Yeah, I'm not sure I see a sane short-term solution here.  :(
Comment 9 Alexandre Poirot [:ochameau] 2012-06-13 00:35:37 PDT
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??
Comment 10 Gabor Krizsanits [:krizsa :gabor] 2012-06-13 00:56:40 PDT
(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.
Comment 11 Boris Zbarsky [:bz] (TPAC) 2012-06-13 08:03:19 PDT
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?
Comment 12 Gabor Krizsanits [:krizsa :gabor] 2012-06-13 09:43:46 PDT
(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!
Comment 13 Gabor Krizsanits [:krizsa :gabor] 2012-06-18 03:45:26 PDT
Created attachment 633983 [details] [diff] [review]
Bug 763897 - Incorrect behaviour of mozMatchesSelector.call through xray

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 14 Boris Zbarsky [:bz] (TPAC) 2012-06-18 09:02:29 PDT
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.
Comment 15 Gabor Krizsanits [:krizsa :gabor] 2012-06-21 07:25:29 PDT
Created attachment 635301 [details] [diff] [review]
Incorrect behaviour of mozMatchesSelector.call through xray

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 16 Bobby Holley (:bholley) (busy with Stylo) 2012-06-21 07:30:59 PDT
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.
Comment 17 Gabor Krizsanits [:krizsa :gabor] 2012-06-27 07:34:03 PDT
Created attachment 637115 [details] [diff] [review]
Incorrect behaviour of mozMatchesSelector.call through xray

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 18 Bobby Holley (:bholley) (busy with Stylo) 2012-06-29 02:47:06 PDT
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'.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-06-29 02:53:27 PDT
Er, whoops, looks like I was confused. It was bz who looked at this before, not blake.
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2012-06-29 02:56:55 PDT
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.
Comment 21 Gabor Krizsanits [:krizsa :gabor] 2012-06-29 03:49:02 PDT
(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...)
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-06-29 03:56:55 PDT
Ok, let's make an effort to fix that bug. In the mean time, please add a comment that links to your post above.
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-08-27 12:18:39 PDT
https://hg.mozilla.org/mozilla-central/rev/d36a98542b01

Note You need to log in before you can comment on or make changes to this bug.