Closed Bug 950909 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- fixed
firefox-esr24 --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files)

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.
Assignee: nobody → bobbyholley+bmo
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
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.
Summary: Investigation bug for AggregatedQueryInterface failures when backporting bug 911864 to esr24 and b2g26 → Native aggregation is silently dropped when an XPCWrappedJS already exists for the target JSObject
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.
Green. Flagging mccr8 for the nitty-gritty, since he's been poking at this
stuff lately. Flagging bz for the concept.
Attachment #8349764 - Flags: superreview?(bzbarsky)
Attachment #8349764 - Flags: review?(continuation)
Attached patch Tests. v1Splinter Review
Attachment #8349765 - Flags: review?(continuation)
Comment on attachment 8349764 [details] [diff] [review]
Forward native aggregation to the root XPCWrappedJS. v1

Actually, smaug is probably better.
Attachment #8349764 - Flags: superreview?(bzbarsky) → superreview?(bugs)
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.
Attachment #8349764 - Flags: review?(continuation) → review+
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.
(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?
Attachment #8349765 - Flags: review?(continuation) → review?(bugs)
(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.
I _think_ that native->forget(&mRoot->mOuter); will fail when mOuter is a COMPtr, right?
Er, fail to _compile_.
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.
Attachment #8349764 - Flags: superreview?(bugs) → superreview+
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)
Attachment #8349765 - Flags: review?(bugs) → review+
(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.
Running the b2g26 backport through try:

https://tbpl.mozilla.org/?tree=Try&rev=6fd9208826ac
https://hg.mozilla.org/mozilla-central/rev/ca675cefb7e4
https://hg.mozilla.org/mozilla-central/rev/c11565395751
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(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
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.
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
Depends on: 937152
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
Attachment #8349764 - Flags: approval-mozilla-beta?
Attachment #8349764 - Flags: approval-mozilla-aurora?
Attachment #8349764 - Flags: approval-mozilla-beta?
Attachment #8349764 - Flags: approval-mozilla-beta+
Attachment #8349764 - Flags: approval-mozilla-aurora?
Attachment #8349764 - Flags: approval-mozilla-aurora+
Awesome, thanks Ryan!
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.