950909 - Native aggregation is silently dropped when an XPCWrappedJS already exists for the target JSObject

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I have a decent idea of what's going on here. Filing a tracking bug for now - I'll rename it once I've got it all sorted out.

Comment 1

[Bobby Holley (:bholley)]

Writing correct tests for this took me forever and a day. I should have everything sorted out now, so I'm pushing to try before grabbing dinner. I'll update the bug with more details later tonight.

https://tbpl.mozilla.org/?tree=Try&rev=55ada2aa1d88

Comment 2

[Bobby Holley (:bholley)]

Ugh, still some oranges to work through. I'm too tired tonight, I'll have to look at it in the morning.

Ryan, feel free to back out bug 911864 if at any point it starts to feel like it's time.

Comment 3

[Bobby Holley (:bholley)]

So.

When an XBL binding's <implementation> tag has an attribute |implements="nsIFoo"|, the bound element must QI to nsIFoo (such that the object returned from QI is the bound object's reflector). This must happen for QIs from both C++ and JS.

To implement this, Element has a custom QI hook that calls into nsBindingManager::GetBindingImplementation(). This, in turn, checks for interfaces that the binding is supposed to implement. If the IIDs match, it tells XPConnect to aggregate the reflector and the reflectee into a new object, implemented with an XPCWrappedJS. The key point here is that the reflector has more functionality than the reflectee, because of the methods it inherits from its spliced-in XBL prototype. This is effectively the inverse of double-wrapped JS objects. Instead of JS->XPCWN->XPCWJS->JS, we have C++->XPCWJS->XPCWN->C++.

XPConnect needs to know about this aggregation because:
* Various CC heuristics depend on it.
* If it didn't, it might otherwise try to remove the double-wrapper and just return the underlying native Element back to C++, which wouldn't implement the appropriate interface.
* XPConnect needs to know how to respond to a QI on the aggregated XPCWrappedJS. The initial QI will forward to the aggregated native if it exists, which will land us in nsBindingMananger::GetBindingImplementation(). This, in turn, will invoked AggregatedQueryInterface() on the XPCWrappedJS, which (in contrast to what its name suggests) _ignores_ the aggregated native, and delegates to the XPCWrappedJSClass.

In nsBindingManager::GetImplementation(), we invoked nsIXPConnect->WrapJSAggregatedToNative(), which ends up invoking nsXPCWrappedJS::GetNewOrUsed() with the reflector as the JSObject, and the reflectee as the native. This either finds an existing XPCWrappedJS for |aIID|, or creates a new one. But before doing so, it first needs a "root" XPCWrappedJS for the underlying object, which it obtains via standard GetNewOrUsed semantics.

We only allow the storage of an aggregated native (mOuter) on root nsXPCWrappedJS instances (which makes sense, and simplifies invariants). This means that aOuter is ignored when creating non-root nsXPCWrappedJS instances. Moreover, if a root already exists, aOuter is totally ignored as well (in nsXPCWrappedJS::GetNewOrUsed()).

This seems totally broken until you recognize that the first WrapJSAggregatedToNative() call is _guaranteed_ to hit the "new" (rather than "used") path. This is because it's creating an nsXPCWrappedJS for a reflector. In all the other code paths that eventually funnel into nsXPCWrappedJS::GetNewOrUsed(), we detect reflectors and just unwrap them to their underlying C++ object. So we only create an XPCWrappedJS if we enter through an API that unconditionally invokes nsXPCWrappedJS::GetNewOrUsed(), even in the reflector case - and that's exactly what WrapJSAggregatedToNative() is.

However, this all changed in bug 911864. With that bug, by default, the <implementation> API is not exposed to untrusted bound content, and is only visible on the XrayWrapper. So to avoid breaking things, I needed to pass the XBL scope's XrayWrapper, rather than the reflector itself, to WrapJSAggregatedToNative(). And _this_ object isn't caught by the reflector check. So it's possible for there to be a existing nsXPCWrappedJS in the map by the time we invoke WrapJSAggregatedToNative(). Et hop, stuff falls apart.

But it gets even more devious. This triggered assertions in esr24 and b2g26, but not on trunk or aurora. The reason, I think, is that event listeners were moved to WebIDL, which significantly reduced the chances that in-content XBL might cause an XPCWrappedJS to be generated for its Xrayed reflector.

Writing a testcase for this that will last through WebIDL conversions turned out to be a massively-involved many-hour process, eventually involving a special Cu API and a helper class. Probably not worth the time in retrospect, but that's water under the bridge at this point.

The solution, as I see it, is to hoist up the logic that stores the aggregated native on the XPCWrappedJS, and make sure it happens unconditionally, regardless of whether the XPCWrappedJS itself was new or used. This is what my patch does - it appears to have some oranges, which I'll investigate now.

Comment 4

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=2a730db1738c

Comment 5

[Bobby Holley (:bholley)]

Attachment: Forward native aggregation to the root XPCWrappedJS. v1

Green. Flagging mccr8 for the nitty-gritty, since he's been poking at this
stuff lately. Flagging bz for the concept.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8349764 [details] [diff] [review]
Forward native aggregation to the root XPCWrappedJS. v1

Actually, smaug is probably better.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8349764 [details] [diff] [review]
Forward native aggregation to the root XPCWrappedJS. v1

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

This patch was a lot simpler than your comment about what is actually happening. ;)

::: js/xpconnect/src/XPCConvert.cpp
@@ +1034,5 @@
>      if (NS_SUCCEEDED(rv) && wrapper) {
> +        // If the caller wanted to aggregate this JS object to a native,
> +        // attach it to the wrapper. Note that we allow a maximum of one
> +        // aggregated native for a given XPCWrappedJS;
> +        if (aOuter && aOuter != wrapper->GetAggregatedNativeObject())

So, if we set the aggregated native to the same value multiple times, that's okay, we just won't do anything on further attempts, but if we set it to a different aggregated native, we'll go ahead and do it, but in debug builds we'll assert?  I guess the later isn't actually a concern?

::: js/xpconnect/src/xpcprivate.h
@@ -2473,5 @@
>  
>      static nsresult
>      GetNewOrUsed(JS::HandleObject aJSObj,
>                   REFNSIID aIID,
> -                 nsISupports* aOuter,

Yay for not having arguments that are only rarely used!

@@ +2510,5 @@
>      nsISupports* GetAggregatedNativeObject() const {return mRoot->mOuter;}
> +    void SetAggregatedNativeObject(nsISupports *aNative) {
> +        MOZ_ASSERT(!mRoot->mOuter, "A wrappedJS can only be aggregated to one native");
> +        nsCOMPtr<nsISupports> native = aNative;
> +        mRoot->mOuter = native.forget().get();

Please just do NS_ADDREF here?  I assume that's what you are doing, and I appreciate your attempt to avoid it, but this is rather non-standard and hard to understand.

Comment 9

[Andrew McCreight [:mccr8]]

I won't be able to review the test until Friday.  Feel free to ask smaug instead, as I don't actually know XBL, but the test looks small enough I can probably figure it out.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #8)

> So, if we set the aggregated native to the same value multiple times, that's
> okay, we just won't do anything on further attempts, but if we set it to a
> different aggregated native, we'll go ahead and do it, but in debug builds
> we'll assert?  I guess the later isn't actually a concern?

Well, we do sometimes set it to the same value multiple times in various situations (in the implements="nsIFoo,nsIBar" case). We should never set it to a different value, which is what the assert is about.

> @@ +2510,5 @@
> >      nsISupports* GetAggregatedNativeObject() const {return mRoot->mOuter;}
> > +    void SetAggregatedNativeObject(nsISupports *aNative) {
> > +        MOZ_ASSERT(!mRoot->mOuter, "A wrappedJS can only be aggregated to one native");
> > +        nsCOMPtr<nsISupports> native = aNative;
> > +        mRoot->mOuter = native.forget().get();
> 
> Please just do NS_ADDREF here?  I assume that's what you are doing, and I
> appreciate your attempt to avoid it, but this is rather non-standard and
> hard to understand.

It's my understanding that using COMPtr + forget() is preferred over manual NS_ADDREF going forward. It should actually be native.forget(&mRoot->mOuter) (I didn't do this originally for some reasons that don't apply to the patch anymore).

This pattern is used pretty extensively in new code - I don't have a super strong preference, but if you want me to use NS_ADDREF, can you bring it up on dev-platform so that we can get something added to the style guide one way or the other?

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Bobby Holley (:bholley) from comment #10)
> This pattern is used pretty extensively in new code - I don't have a super
> strong preference, but if you want me to use NS_ADDREF, can you bring it up
> on dev-platform so that we can get something added to the style guide one
> way or the other?

Well, my thinking was that eventually mOuter will become a smart pointer (bug 947336), then this all can just become an assignment, and it will be easier to find with NS_ADDREF.  In some sense you are writing new code, but this is also kind of old code.  But whatever you want is fine.  I'll add a note in the other bug about it so it won't get forgotten if somebody does the conversion.

Comment 12

[Bobby Holley (:bholley)]

I _think_ that native->forget(&mRoot->mOuter); will fail when mOuter is a COMPtr, right?

Comment 13

[Bobby Holley (:bholley)]

Er, fail to _compile_.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Attempted b2g26 rebase: https://tbpl.mozilla.org/?tree=Try&rev=49283944f9f3
Attempted esr24 rebase: https://tbpl.mozilla.org/?tree=Try&rev=6d7fde331b6b

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8349764 [details] [diff] [review]
Forward native aggregation to the root XPCWrappedJS. v1

>     if (NS_SUCCEEDED(rv) && wrapper) {
>+        // If the caller wanted to aggregate this JS object to a native,
>+        // attach it to the wrapper. Note that we allow a maximum of one
>+        // aggregated native for a given XPCWrappedJS;
>+        if (aOuter && aOuter != wrapper->GetAggregatedNativeObject())
>+            wrapper->SetAggregatedNativeObject(aOuter);
Couldn't you just change SetAggregatedNativeObject to
        MOZ_ASSERT(aNative);
        MOZ_ASSERT(!mRoot->mOuter || mRoot->mOuter == aNative, "A wrappedJS can only be aggregated to one native");
        if (!mRoot->mOuter) {
          NS_ADDREF(mRoot->mOuter = aNative);
        }
And then remove
aOuter != wrapper->GetAggregatedNativeObject() 
from the if.

>+        nsCOMPtr<nsISupports> native = aNative;
>+        mRoot->mOuter = native.forget().get();
This is not a common pattern.
forget is used for example for out params when one wants to avoid extra addref/release.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8349765 [details] [diff] [review]
Tests. v1


>+        // Generate an XPCWrappedJS for the reflector. We need to do this
>+        // explicitly with a testing helper, because we're converging on
>+        // removing XPConnect from the web, which means that it won't be
>+        // possible to generate an XPCWrappedJS from content (or XBL) code.
>+        var scope = {};
>+        var wjs = SpecialPowers.Cu.generateXPCWrappedJS(this, scope);
Don't call this wjs, since the return value isn't wrapped JS, but a holder.

>+  <iframe src="http://example.com/tests/content/xbl/test/file_bug950909.html"/>
Shouldn't this have type="content" attribute, so that the docshell for the iframe
is actually a content docshell.

>+     * Forces the generation of an XPCWrappedJS for a given object. For internal
>+     * and testing use only. This is only useful to set up wrapper map conditions
>+     * for a testcase.
>+     *
>+     * if |scope| is passed, the XPCWrappedJS is generated in the scope of that object.
>+     */
>+    [implicit_jscontext]
>+    nsISupports generateXPCWrappedJS(in jsval obj, [optional] in jsval scope);
Please document what the return value is. (that is *not* the generated wrappedjs)

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #16) 
> >+  <iframe src="http://example.com/tests/content/xbl/test/file_bug950909.html"/>
> Shouldn't this have type="content" attribute, so that the docshell for the
> iframe
> is actually a content docshell.
Oh, you don't need type="content", since we run the test itself in a content docshell.

Comment 18

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=1bddc14c21a2

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ca675cefb7e4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c11565395751

Comment 20

[Bobby Holley (:bholley)]

Running the b2g26 backport through try:

https://tbpl.mozilla.org/?tree=Try&rev=6fd9208826ac

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca675cefb7e4
https://hg.mozilla.org/mozilla-central/rev/c11565395751

Comment 22

[Ryan VanderMeulen [:RyanVM]]

(In reply to Bobby Holley (:bholley) from comment #20)
> Running the b2g26 backport through try:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=6fd9208826ac

The Aurora uplift looks OK on Try with minimal effort needed. Looks like Beta is hitting the same issues as b2g26.

Aurora: https://tbpl.mozilla.org/?tree=Try&rev=c150b7fbe6fe
Beta: https://tbpl.mozilla.org/?tree=Try&rev=18cef7847569

Comment 23

[Bobby Holley (:bholley)]

I finally debugged the b2g26 failure (browser_trigger_redirect.js). The basic problem is that, by setting aOuter slightly later during XPCWrappedJS creation, we end up having a non-aggregated XPCWrappedJS during the call to CheckMainThreadOnly, which does a scripted QI on the XPCWrappedJS. This QI fails when the aggregation isn't there, which throws an exception and ends up causing the test to fail.

We can fix this by backporting bug 937152, which removes the CheckMainThreadOnly call altogether, which I'll do now.

Comment 24

[Bobby Holley (:bholley)]

The tests for this bug are also timing out on branch in a manner that I can't reproduce locally, which is almost certainly related to the test (which is just setting up a bunch of complicated conditions that might trigger an assertion) and not the patch at hand. This patch actually tests itself on branch, since we're currently hitting assertions there. As such, I'm landing this fix without the tests on b2g26. If all goes well there, I'll backport everywhere else as well.

remote:   https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/0dda1af03e99
remote:   https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/af535805291f

Comment 25

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-esr24/rev/c101eaeb660d
remote:   https://hg.mozilla.org/releases/mozilla-esr24/rev/1efe77eee0fb

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 8349764 [details] [diff] [review]
Forward native aggregation to the root XPCWrappedJS. v1

We've had to land this on esr24 and b2g26 already, so we should get it on aurora and beta to keep the trees consistent. The auora backport is just the version that's already on central, and the beta backport (which I have in my tree) is more or less the version that landed on b2g26.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding
User impact if declined: likely nothing
Testing completed (on m-c, etc.): Landed on m-c, esr24, and b2g26.
Risk to taking this patch (and alternatives if risky): medium risk, but we've already swallowed that risk by taking this patch on esr24 and b2g26. 
String or IDL/UUID changes made by this patch: None

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84b87ea8c0cc
https://hg.mozilla.org/releases/mozilla-aurora/rev/9bb7691ca2c0

https://hg.mozilla.org/releases/mozilla-beta/rev/e33d85e6e12a

Comment 28

[Bobby Holley (:bholley)]

Awesome, thanks Ryan!