Last Comment Bug 734475 - Take the full union of native sets when bringing non-PreCreate XPWNs across compartments
: Take the full union of native sets when bringing non-PreCreate XPWNs across c...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on:
Blocks: cpg
  Show dependency treegraph
 
Reported: 2012-03-09 12:39 PST by Bobby Holley (busy)
Modified: 2012-03-18 12:00 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Take the full union of native sets when bringing non-PreCreate XPWNs across compartments. v1 (6.28 KB, patch)
2012-03-09 12:41 PST, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
Statistics instrumentation. (5.63 KB, patch)
2012-03-09 13:14 PST, Bobby Holley (busy)
no flags Details | Diff | Review

Description Bobby Holley (busy) 2012-03-09 12:39:58 PST
With compartment-per-global, there are now many more chrome compartments.

Whenever we access an object cross-compartment that doesn't have a definitive parent (via PreCreate), we create a new XPCWN in the current compartment. Unfortunately, this loses the results of QI.

There's a kinda-sorta fix for this in the bottom of WrapperFactory::PrepareForWrapping, but it's more of a heuristic than anything else (Blake says it comes from the dark nights of Brain Transplants). In particular, it assumes that the destination wrapper's native set is always a subset of the original, which isn't always true.

I've got a patch for this that fixes the problem. However, I want to gather some statistics to make sure that it won't regress performance. I'll upload the patch and then do that before flagging for review.
Comment 1 Bobby Holley (busy) 2012-03-09 12:41:04 PST
Created attachment 604494 [details] [diff] [review]
Take the full union of native sets when bringing non-PreCreate XPWNs across compartments. v1

Attaching patch.
Comment 2 Bobby Holley (busy) 2012-03-09 13:14:58 PST
Created attachment 604508 [details] [diff] [review]
Statistics instrumentation.

Attaching my statistics instrumentation patch (commentary to follow).
Comment 3 Bobby Holley (busy) 2012-03-09 13:19:53 PST
Overall, this looks really good. Here's what I get for just firing up the browser and goofing around in the pref pane:

============XPCNativeSet Union Statistics=============
Total calls to Union Creator: 1671
Maximum set size encountered: 5
Number of times we reused the first set: 1613
Number of times we reused the second set: 58
Number of times we made a new set: 0
Maximum number of incremental sets created while making a new set: 0
======================================================

This shows that we're not doing an unreasonable number of union calls. More importantly, it shows that they're not hugely expensive. If n is the set size, then the complexity of the function in the reuse case is O(n^2) (since we just have to count the number of unique elements). Given that the maximum set size is 5, this doesn't sound too bad.

Even more interesting is the fact that we had no instances where we needed to make a new set. This is the thing that this patch handles, so it's interesting to see that it's not very common.

Indeed, here are the statistics from the XPCShell test that failed without this patch (test_notifications_onDeleteURI.js):

============XPCNativeSet Union Statistics=============
Total calls to Union Creator: 134
Maximum set size encountered: 6
Number of times we reused the first set: 72
Number of times we reused the second set: 61
Number of times we made a new set: 1
Maximum number of incremental sets created while making a new set: 1
======================================================

So there was only 1 case, and that was precisely the case that was causing us to fail.

I'm going to run this through mochitest-browser-chrome just to make sure that we don't get any pessimally-large native sets. If all looks good, I'll flag for review.
Comment 4 Bobby Holley (busy) 2012-03-09 14:26:16 PST
I just ran mochitest-browser-chrome for about 25 minutes, and got the following:

============XPCNativeSet Union Statistics=============
Total calls to Union Creator: 200758
Maximum set size encountered: 6
Number of times we reused the first set: 187850
Number of times we reused the second set: 12908
Number of times we made a new set: 0
Maximum number of incremental sets created while making a new set: 0
======================================================

So basically, we're looking at a maximum of 36 pointer comparisons for each call to the union creator. This is probably cheaper than QI-ing a DOM node, so I think it should be fine.
Comment 5 Bobby Holley (busy) 2012-03-09 14:26:46 PST
Comment on attachment 604494 [details] [diff] [review]
Take the full union of native sets when bringing non-PreCreate XPWNs across compartments. v1

Flagging for review based on the above analysis.
Comment 6 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-03-16 03:52:32 PDT
Comment on attachment 604494 [details] [diff] [review]
Take the full union of native sets when bringing non-PreCreate XPWNs across compartments. v1

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

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp
@@ +675,5 @@
> +    // Figure out how many interfaces we'll need in the new set.
> +    PRUint32 uniqueCount = firstSet->mInterfaceCount;
> +    for (PRUint32 i = 0; i < secondSet->mInterfaceCount; ++i)
> +        if (!firstSet->HasInterface(secondSet->mInterfaces[i]))
> +            uniqueCount++;

Nit: Braces around the body of the for loop (the rule in the JS engine is that braces are required if there are multiple lines in the body, even if those lines count as a single statement).
Comment 7 Bobby Holley (busy) 2012-03-16 09:46:59 PDT
Pushed to try, along with some other patches: https://tbpl.mozilla.org/?tree=Try&rev=4d9ad69264c3
Comment 8 Bobby Holley (busy) 2012-03-16 12:49:00 PDT
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/4624437cd69e
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-17 17:09:07 PDT
https://hg.mozilla.org/mozilla-central/rev/4624437cd69e
Comment 10 :Ms2ger 2012-03-18 11:58:49 PDT
Comment on attachment 604494 [details] [diff] [review]
Take the full union of native sets when bringing non-PreCreate XPWNs across compartments. v1

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

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +235,5 @@
>          obj = JSVAL_TO_OBJECT(v);
>          NS_ASSERTION(IS_WN_WRAPPER(obj), "bad object");
>  
> +        // Because the underlying native didn't have a PreCreate hook, we had
> +        // to a new (or possibly pre-existing) XPCWN in our compartment.

We had to a XPCWN?
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-18 12:00:36 PDT
I sense a meme coming on.

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