734475 - Take the full union of native sets when bringing non-PreCreate XPWNs across compartments

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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 (:bholley)]

Attachment: Take the full union of native sets when bringing non-PreCreate XPWNs across compartments. v1

Attaching patch.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Statistics instrumentation.

Attaching my statistics instrumentation patch (commentary to follow).

Comment 3

[Bobby Holley (:bholley)]

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 (:bholley)]

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 (:bholley)]

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) (inactive)]

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 (:bholley)]

Pushed to try, along with some other patches: https://tbpl.mozilla.org/?tree=Try&rev=4d9ad69264c3

Comment 8

[Bobby Holley (:bholley)]

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/4624437cd69e

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/4624437cd69e

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 (Exited; not receiving bugmail, old account, do not use)]

I sense a meme coming on.