Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla14

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Created attachment 604494 [details] [diff] [review]
Take the full union of native sets when bringing non-PreCreate XPWNs across compartments. v1

Attaching patch.
Created attachment 604508 [details] [diff] [review]
Statistics instrumentation.

Attaching my statistics instrumentation patch (commentary to follow).
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.
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 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.
Attachment #604494 - Flags: review?(mrbkap)
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).
Attachment #604494 - Flags: review?(mrbkap) → review+
Pushed to try, along with some other patches: https://tbpl.mozilla.org/?tree=Try&rev=4d9ad69264c3
Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/4624437cd69e
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/4624437cd69e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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?
I sense a meme coming on.
You need to log in before you can comment on or make changes to this bug.