Closed Bug 1164014 Opened 9 years ago Closed 9 years ago

[e10s] Shim optimization

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
e10s m7+ ---
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: gkrizsanits, Assigned: gkrizsanits)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

benchmark-addon.xpi
1.57 KB, application/x-xpinstall
Details
diff since the last round
10.88 KB, patch
Details | Diff | Splinter Review
InterposeProperty whitelist. v3
20.56 KB, patch
billm
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
fixing defaultShims
1.69 KB, patch
billm
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
patch for OOMs
1.48 KB, patch
gkrizsanits
: feedback+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
We should port some code from the multiprocessShims.js to AddonWrapper.cpp to gain some performance. I have my doubts about this but Bill has some convincing numbers from a small benchmark addon. We might want to uplift this to aurora, so this should be m7.
Assignee: nobody → gkrizsanits
tracking-e10s: --- → ?
Attached patch WIP (obsolete) — — Splinter Review 
Does this look like what you had in mind? I still want to polish the patch a bit (comments and maybe break up InterposeProperty a bit). I was about to add some methods to register iid's and dom interfaces we want to do interposition on. But since there are just a few ones we care about, I realized after a while that it isn't worth the effort (all the versions I had in mind just made things uglier for no good reason). Unless we want to port to c++ parts like this as well:
desc.get = function() { return interp.getters[prop](addon, target); };
But I would not go that far, I don't think it's worth it.

On the other hand I'm thinking about a flag that can turn off property interposition for the default shim (should be trivial).
Attachment #8604722 - Flags: feedback?(wmccloskey)
Yeah I think this is smart, though we should break it out into a heavily-commented helper called CanSkipInterposition I think.
Comment on attachment 8604722 [details] [diff] [review]
WIP

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

Unfortunately, I don't think this will help much by itself. Many of the objects that add-ons use from chrome will pass all these tests, so we'll still call into JS, which is what we want to avoid. It's very common for add-ons to touch chrome windows and XUL elements, for example.

I think we really need to have a hash table of property names that we interpose on. The C++ code would check if the property is in the hash table. If it's not, then we wouldn't call into JS. The JS code would maintain the hash table. Also, it would need to disable the hash table when using prefetching.

I think the API could look something like this:
  Cu.setInterpositionWhitelist(in nsIAddonInterposition interposition, in jsval whitelist);
The whitelist would be either a JSObject whose own properties tell us the names of properties to interpose on. Or it could be null or undefined, meaning that we should interpose on all properties.

setInterpositionWhitelist could enumerate all the own properties of the whitelist object and save them in a hashtable. One tricky thing is where to store the hashtable. Given how few interpositions we're likely to have (probably only two), I think it makes sense to have a global fixed-sized array of (interposition, hashtable) pairs that we would search every time we need the hashtable.

With the hashtable solution, I think the code in this patch shouldn't be needed. Eventually we could do more targeted optimization that uses a different hashtable of property names for each object tag. But I don't think we need that right now.

::: js/xpconnect/wrappers/AddonWrapper.cpp
@@ +44,5 @@
> +            iid != &NS_GET_IID(nsIXPCComponents_Utils))
> +            return true;
> +    } else {
> +        const js::Class* clasp = js::GetObjectClass(unwrapped);
> +        if (!mozilla::dom::IsDOMClass(clasp))

At least in the case of chrome windows, IsDOMClass is returning false. However, we want to interpose on chrome windows. I see a test failure in test_addonShims.js from this because of that. I think that's why this patch seems to speed up the benchmark I sent to you--it's causing us not to interpose on chrome windows at all.
Attachment #8604722 - Flags: feedback?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #3)
> 
> Unfortunately, I don't think this will help much by itself. Many of the
> objects that add-ons use from chrome will pass all these tests, so we'll
> still call into JS, which is what we want to avoid. It's very common for
> add-ons to touch chrome windows and XUL elements, for example.

Probably not enough in itself indeed.

> 
> I think we really need to have a hash table of property names that we
> interpose on. The C++ code would check if the property is in the hash table.
> If it's not, then we wouldn't call into JS. The JS code would maintain the
> hash table. Also, it would need to disable the hash table when using
> prefetching.
> 
> I think the API could look something like this:
>   Cu.setInterpositionWhitelist(in nsIAddonInterposition interposition, in
> jsval whitelist);
> The whitelist would be either a JSObject whose own properties tell us the
> names of properties to interpose on. Or it could be null or undefined,
> meaning that we should interpose on all properties.

Why not just an array of property names?

> 
> setInterpositionWhitelist could enumerate all the own properties of the
> whitelist object and save them in a hashtable. One tricky thing is where to
> store the hashtable. Given how few interpositions we're likely to have
> (probably only two), I think it makes sense to have a global fixed-sized
> array of (interposition, hashtable) pairs that we would search every time we
> need the hashtable.

Yeah I like the global array idea, or there could be two global hashtable and
when we set the XPCWrappedNativeScopes interposition we could chose a hashtable
as well to point to based on the interposition. Or something similar.

> 
> With the hashtable solution, I think the code in this patch shouldn't be
> needed. Eventually we could do more targeted optimization that uses a
> different hashtable of property names for each object tag. But I don't think
> we need that right now.
> 

I disagree there. Some of the properties are too generic imo: receiveMessage, dispatch, addEventListener, docShell, contentWindow, contentDocument, _content.
I can totally picture some of these in a loop, and it's not a lot of code to do some filtering on the target as well. Code like this is not uncommon:
 
for(...) {
  contentWindow.something[i]...
}

> ::: js/xpconnect/wrappers/AddonWrapper.cpp
> @@ +44,5 @@
> > +            iid != &NS_GET_IID(nsIXPCComponents_Utils))
> > +            return true;
> > +    } else {
> > +        const js::Class* clasp = js::GetObjectClass(unwrapped);
> > +        if (!mozilla::dom::IsDOMClass(clasp))
> 
> At least in the case of chrome windows, IsDOMClass is returning false.
> However, we want to interpose on chrome windows. I see a test failure in
> test_addonShims.js from this because of that. I think that's why this patch
> seems to speed up the benchmark I sent to you--it's causing us not to
> interpose on chrome windows at all.

Good catch! Actually I haven't run your benchmark yet, nor did I do any thorough testing,
just wanted get some feedback today, before I continue my work tomorrow, and it totally was
worth it :) So I think I will want to add the hashmap for prop names and might keep _some_ of
the filtering work of this patch as well. I'm not a big fun of the "different hashtable of property names for each object tag" idea either, since we should then add one for iid's as well maybe,
and we have other filters as well... But a customizable property filter like you suggested,
and some built in default target filter should do the job and keep things simple enough.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4)
> Why not just an array of property names?

Yes, that would be fine too.

> Yeah I like the global array idea, or there could be two global hashtable and
> when we set the XPCWrappedNativeScopes interposition we could chose a
> hashtable
> as well to point to based on the interposition. Or something similar.

We want to be able to change the hashtable after we've created an XPCWrappedNativeScope. So I think it would be a little harder to do that way.

> I disagree there. Some of the properties are too generic imo:
> receiveMessage, dispatch, addEventListener, docShell, contentWindow,
> contentDocument, _content.
> I can totally picture some of these in a loop, and it's not a lot of code to
> do some filtering on the target as well. Code like this is not uncommon:

receiveMessage and dispatch are not properties that we interpose on. The others don't seem that generic to me. I really would rather not move the tagging code into C++. I'm worried it will introduce errors and make it harder to write new shims. And I don't think it will have much effect on performance--it just doesn't seem like these names are very likely to be used for multiple purposes. If we find that they are, we can revisit the decision and do more work. Until then, we should do the minimum possible.
I'm totally lost in trying to create a jsid hashset. One thing is to get the tracing bits right (probably I need to implement a CustomAutoRooter for that), but I don't even know how to use a meaningful hash function since JsidHasher is buried deep inside the js engine internals and I have no idea why, and what would be an acceptable way to make it available for xpconnect http://mxr.mozilla.org/mozilla-central/source/js/src/jsatom.h#42.

Bill, Bobby, can you guys help me with this, or should we needinfo someone from the js team? I could also just use a JSObject and call HasOwnProperty on it instead of using a jsid hashset for the whitelist... But I would prefer a hashset.
Let's just convert the jsid to a string before putting it in a hashtable. I think you can use nsAutoJSString and initialize it with a jsid to get a nsString-like thing, although maybe Bobby knows a better way.
(In reply to Bill McCloskey (:billm) from comment #7)
> Let's just convert the jsid to a string before putting it in a hashtable. I
> think you can use nsAutoJSString and initialize it with a jsid to get a
> nsString-like thing, although maybe Bobby knows a better way.

But that would mean to do the jsid->jsstring conversion for each and every property access, but if you think that is fast enough I can do that... (my concern is that it would mean to convert every number to string which could be a problem even in your benchmark addon, no?).
Hmm... if I could skip the non-string jsid's... alright I will do that.
Why not just intern all the jsid strings we have interpositions for? Then we don't have to worry about tracing or conversion at all.
Yeah, that's a good idea. Then you could just use JSID_BITS as the hash key.
(In reply to Bill McCloskey (:billm) from comment #11)
> Yeah, that's a good idea. Then you could just use JSID_BITS as the hash key.

That would be great but for some reason it does not seem to work. I do JS_InternJSString and then INTERNED_STRING_TO_JSID on the incoming strings in setInterpositionWhitelist and then add the JSID_BITS of these jsid's to the hashset. But then in the InterposeProperty call the id I get differs for the same string for some reason. (for example setInterpositionWhitelist were called with ["utils"] and then when we get to the InterposeProperty from a Components.utils the JSID_BITS(id) is totally different)

I'm not super familiar with interned strings, should I do something differently?
That's unexpected. Could you post a patch?
Attached patch WIP2 (obsolete) — — Splinter Review 
Sure, but the patch is heavily under construction... maybe I'm just doing something stupid, I will look into it as well. But if you could take a look at the interning strings part, that would be great...
Attachment #8604722 - Attachment is obsolete: true
Attachment #8605965 - Flags: feedback?(wmccloskey)
Comment on attachment 8605965 [details] [diff] [review]
WIP2

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

The problem was not related to the interned strings. Xpconnect screwed me over a bit, by returning different nsIAddonInterposition pointer for the same thing, based on the caller :( I guess it is because of the double wrapping, but I'm not quite sure. Passing in a js implemented nsIAddonInterposition into a Cu method from the interpostions own scope (from it's constructor let's say) has different result than doing the same from some other scopes (XPIProvider let's say).

It's weird. I wouldn't want to hold up this bug for trying to fix this problem in xpconnect... For a workaround I will do the setInterpositionWhitelist automatically from the setAddonInterposition call (the interposition itself will know its whitelist at this point).
Attachment #8605965 - Flags: feedback?(wmccloskey)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> Comment on attachment 8605965 [details] [diff] [review]
> WIP2
> 
> Review of attachment 8605965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The problem was not related to the interned strings. Xpconnect screwed me
> over a bit, by returning different nsIAddonInterposition pointer for the
> same thing, based on the caller :( I guess it is because of the double
> wrapping, but I'm not quite sure. Passing in a js implemented
> nsIAddonInterposition into a Cu method from the interpostions own scope
> (from it's constructor let's say) has different result than doing the same
> from some other scopes (XPIProvider let's say).

Huh, I didn't know that could happen.

What happens if, when you call setInterpositionWhitelist, you get a pointer to the interposition using Cc["name-of-shim"].getService(Ci.nsIAddonInterposition) rather than using a reference to the shim directly? You might have to do it lazily so that the service gets initialized before you ask for it.

> It's weird. I wouldn't want to hold up this bug for trying to fix this
> problem in xpconnect... For a workaround I will do the
> setInterpositionWhitelist automatically from the setAddonInterposition call
> (the interposition itself will know its whitelist at this point).

I'm not sure that's going to work. In order to deal with prefetching we need to be able to change the whitelist later on.
(In reply to Bill McCloskey (:billm) from comment #16)
> What happens if, when you call setInterpositionWhitelist, you get a pointer
> to the interposition using
> Cc["name-of-shim"].getService(Ci.nsIAddonInterposition) rather than using a
> reference to the shim directly? You might have to do it lazily so that the
> service gets initialized before you ask for it.

Well, I tried to call setInterpositionWhitelist from the constructor of the shim,
so that would be kind of an infinite loop or some error... But we could do it the
other way around and change setAddonInterpostion to take the name of the addon,
and do the getService bits in C++ maybe...

> I'm not sure that's going to work. In order to deal with prefetching we need
> to be able to change the whitelist later on.

I had the prefetching part in mind as well, and I didn't see any problem with it.
We would only add the default white list to the interposition once (for the first time when 
is registered with some id). Then it should not be a problem to add or remove elements
from this list for prefetching (from c++ or js). The only thing I had to avoid is to call Cu.setInterpositionWhitelist from the interpositions own scope. 
But actually we might want to use a per addonID or per scope set for that part anyway...
I'm not even sure we need to deal with them here. I mean, what would we lose
if we just let every CPOWs through? If they benefit from the prefetching we
want to call into the interposition, and if not, then CPOW are slow enough that
we won't win a lot by not calling into the interposition...
By the way, what is the future of prefetching? Do we plan to add more add-ons to the list?
I hope that the two addons we have prefetching for will be ported to e10s and we can
ditch that part... But you might have totally different plans for them.
> I'm not even sure we need to deal with them here. I mean, what would we lose
> if we just let every CPOWs through? If they benefit from the prefetching we
> want to call into the interposition, and if not, then CPOW are slow enough that
> we won't win a lot by not calling into the interposition...

That's a good idea. So we'll just ignore the whitelist if it's a CPOW.
Attached patch InterposeProperty whitelist. v1 (obsolete) — — Splinter Review 
Something like this I guess. With the benchmark test addon the numbers are (opt release build on linux):

with patch:

TOPIC profile-after-change
TOPIC domwindowopened
Benchmark took 0.0021ms per iter
TOPIC domwindowopened
Benchmark took 0.0012ms per iter
TOPIC domwindowclosed

without:

TOPIC profile-after-change
TOPIC domwindowopened
Benchmark took 0.0123ms per iter

It's looking a lot better :)
Attachment #8605965 - Attachment is obsolete: true
Attachment #8607476 - Flags: review?(wmccloskey)
Attached file benchmark-addon.xpi — 
For the curious, here is the benchmark add-on from Bill I was referring to.
Comment on attachment 8607476 [details] [diff] [review]
InterposeProperty whitelist. v1

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

Let's do one more round.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +779,4 @@
>          nsContentUtils::RegisterShutdownObserver(new ClearInterpositionsObserver());
>      }
>      if (interp) {
> +        MOZ_ASSERT(gInterpositionMap);

This is not a useful assertion. Please remove.

@@ +784,5 @@
> +        NS_ENSURE_TRUE(ok, false);
> +
> +        // If this is the first time that the interpostion is being used,
> +        // we need to set up the property whitelist for it.
> +        bool wlInited = !!GetInterpositionWhitelist(interp, false);

I'm not really sure why this call needs to be here.

@@ +792,5 @@
> +            if (NS_FAILED(rv)) {
> +                JS_ReportError(cx, "Could not get the whitelist from the interposition.");
> +                return false;
> +            }
> +            if (!JS_WrapValue(cx, &whitelist))

Rather than wrapping whitelist, it seems easier to have SetInterpositionWhitelist enter the compartment of the whitelist.

@@ +820,5 @@
> +    InterpositionWhitelistArray& wls = *gInterpositionWhitelists;
> +    InterpositionWhitelist* wl = nullptr;
> +    for (uint32_t i = 0; i < wls.Length(); i++) {
> +        if (wls[i]->interposition == interposition) {
> +            wl = &wls[i]->whitelist;

Let's just return it here. It will remove some branching.

@@ +854,5 @@
> +    RootedObject whitelistObj(cx, &whitelist.toObject());
> +    uint32_t length;
> +    if (!JS_IsArrayObject(cx, whitelistObj) ||
> +        !JS_GetArrayLength(cx, whitelistObj, &length) ||
> +        !length) {

Why the !length check? Won't the length be 0 for the default shim?

@@ +859,5 @@
> +        JS_ReportError(cx, "Argument must be an array.");
> +        return false;
> +    }
> +
> +    nsTArray<uint64_t> entries;

Why use an array here? Why not just put the entries into the hash table directly?

@@ +873,5 @@
> +
> +        RootedString str(cx, idval.toString());
> +        str = JS_InternJSString(cx, str);
> +        if (!str) {
> +            JS_ReportError(cx, "Argument mustString internization failed.");

Copy and paste error or something.

@@ +881,5 @@
> +        jsid id = INTERNED_STRING_TO_JSID(cx, str);
> +        entries.AppendElement(JSID_BITS(id));
> +    }
> +
> +    InterpositionWhitelist* wl = GetInterpositionWhitelist(interposition);

You should clear the hashtable before adding entries to it.

::: js/xpconnect/src/xpcprivate.h
@@ +1024,5 @@
> +    nsIAddonInterposition* interposition;
> +    InterpositionWhitelist whitelist;
> +};
> +
> +typedef nsTArray<InterpositionWhitelistPair*> InterpositionWhitelistArray;

I think you should be able to make this an nsTArray<InterpositionWhitelistPair>. That would simplify some of the memory allocation.

@@ +1251,5 @@
>  
>      // This is a service that will be use to interpose on some property accesses on
>      // objects from other scope, for add-on compatibility reasons.
>      nsCOMPtr<nsIAddonInterposition>  mInterposition;
> +    

Extra whitespace.

::: toolkit/components/addoncompat/multiprocessShims.js
@@ +73,5 @@
> +
> +  let wl = [];
> +  for (let v in this._interfaceInterpositions) {
> +    let interp = this._interfaceInterpositions[v];
> +    Array.prototype.push.apply(wl, Object.getOwnPropertyNames(interp.methods));

An easier way to do this would be wl.push(...Object.getOwnPropertyNames(interp.methods)).
Attachment #8607476 - Flags: review?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #21)
> > +        // If this is the first time that the interpostion is being used,
> > +        // we need to set up the property whitelist for it.
> > +        bool wlInited = !!GetInterpositionWhitelist(interp, false);
> 
> I'm not really sure why this call needs to be here.

I just want to avoid re-parsing the map each time an add-on related scope is created and the interposition is set on it. Reseting the hashset each time the interposition is associated with a new scope seems like a bad idea to me.

> 
> @@ +792,5 @@
> > +            if (NS_FAILED(rv)) {
> > +                JS_ReportError(cx, "Could not get the whitelist from the interposition.");
> > +                return false;
> > +            }
> > +            if (!JS_WrapValue(cx, &whitelist))
> 
> Rather than wrapping whitelist, it seems easier to have
> SetInterpositionWhitelist enter the compartment of the whitelist.

Good idea, I'll do that.

> > +    RootedObject whitelistObj(cx, &whitelist.toObject());
> > +    uint32_t length;
> > +    if (!JS_IsArrayObject(cx, whitelistObj) ||
> > +        !JS_GetArrayLength(cx, whitelistObj, &length) ||
> > +        !length) {
> 
> Why the !length check? Won't the length be 0 for the default shim?

Whoops, I was going to remove that check... Currently we don't call this
for the default shim as interposition is set directly for those scopes not
via SetAddonInterposition, which I think is fine, but length check should
be removed for sure.

> 
> @@ +859,5 @@
> > +        JS_ReportError(cx, "Argument must be an array.");
> > +        return false;
> > +    }
> > +
> > +    nsTArray<uint64_t> entries;
> 
> Why use an array here? Why not just put the entries into the hash table
> directly?

Somehow in my mind we wanted to add the elements of the input array to the set instead of resetting the set, so this nsTArray was there for error handling (adding elements only after the array parsing succeeded). Since we don't need to do prefetching part, or change the set, this is stupid, I will remove it. 

> I think you should be able to make this an
> nsTArray<InterpositionWhitelistPair>. That would simplify some of the memory
> allocation.

I'd wish. I hate this part but I could not make that version to compile. Then I saw gInterpositionMap and I thought it's a pointer for the same allocation problem. I can give in another try but I'm not too optimistic.

> > +    Array.prototype.push.apply(wl, Object.getOwnPropertyNames(interp.methods));
> 
> An easier way to do this would be
> wl.push(...Object.getOwnPropertyNames(interp.methods)).

Thanks!
Probably it helps if I attach an interdiff too.
Attached patch InterposeProperty whitelist. v2 (obsolete) — — Splinter Review 

        Attachment #8607476 -
        Attachment is obsolete: true

        Attachment #8608196 -
        Flags: review?(wmccloskey)

    
    

    
  
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22) 
> > I think you should be able to make this an
> > nsTArray<InterpositionWhitelistPair>. That would simplify some of the memory
> > allocation.
> 
> I'd wish. I hate this part but I could not make that version to compile.
> Then I saw gInterpositionMap and I thought it's a pointer for the same
> allocation problem. I can give in another try but I'm not too optimistic.

I'm not too enthusiastic about implement a copy ctor for hashset, which
we would need to turn InterpositionWhitelistPair* to InterpositionWhitelistPair
(http://mxr.mozilla.org/mozilla-central/source/js/public/HashTable.h#501) but
tried to turn nsTArray* into an nsTArray at least.

But I'm not sure how to stop a static nsTArray causing a leak like this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2f6e02149a3

Let's talk about this later today and make a decision.

    
    

    
  
Comment on attachment 8608196 [details] [diff] [review]
InterposeProperty whitelist. v2

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

I forgot about the default shim. We're going to need some way of enabling the whitelist for that. The current patch prevents the default shim from doing property interposition.

Also, let's refactor the code a bit. Let's have two functions:

* GetInterpositionWhitelist(interp):
- finds the interposition in the whitelist array and returns it
- doesn't add anything

* UpdateInterpositionWhitelist(cx, interp)
- call FindInterpositionWhitelist and return if found
- AppendElemend to the whitelist array
- enter the PrivilegedJunkScope. otherwise we might get security errors if cx is not in a system compartment.
- call GetWhitelist on the interposition and parse the result into the newly appended element

Then we could call UpdateInterpositionWhitelist from the default-shims code in the XPCWrappedNativeScope constructor as well as from SetAddonInterposition.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +26,5 @@
>  
>  XPCWrappedNativeScope* XPCWrappedNativeScope::gScopes = nullptr;
>  XPCWrappedNativeScope* XPCWrappedNativeScope::gDyingScopes = nullptr;
>  XPCWrappedNativeScope::InterpositionMap* XPCWrappedNativeScope::gInterpositionMap = nullptr;
> +InterpositionWhitelistArray XPCWrappedNativeScope::gInterpositionWhitelists;

This needs to be a pointer to avoid the leak.

@@ +810,5 @@
>  
> +/* static */ InterpositionWhitelist*
> +XPCWrappedNativeScope::GetInterpositionWhitelist(nsIAddonInterposition* interposition, bool createNew)
> +{
> +    for (uint32_t i = 0; i < gInterpositionWhitelists.Length(); i++) {

size_t

@@ +822,5 @@
> +
> +    InterpositionWhitelistPair* newPair = new InterpositionWhitelistPair();
> +    newPair->interposition = interposition;
> +    newPair->whitelist.init();
> +    gInterpositionWhitelists.AppendElement(newPair);

You can avoid the copy constructor by writing the code this way:

InterpositionWhitelistPair* newPair = gInterpositionWhitelists->AppendElement();
newPair->interposition = interposition;
newPair->whitelist.init();

Then you can make the hashtable a value in the pair.

@@ +827,5 @@
> +    return &newPair->whitelist;
> +}
> +
> +class MOZ_STACK_CLASS AutoClearWhitelistIfError {
> +public:

public: and private: should be indented two spaces (JS style).

@@ +840,5 @@
> +    }
> +
> +    void Succeeded() { mWl = nullptr; }
> +private:
> +    InterpositionWhitelist* mWl;

Let's call this mWhitelist.

@@ +860,5 @@
> +
> +    InterpositionWhitelist* wl = GetInterpositionWhitelist(interposition);
> +    wl->clear();
> +
> +    AutoClearWhitelistIfError acwl(wl);

It doesn't seem worth the complexity to clear the map if this fails. An empty whitelist just means the interposition can't be used at all.

        Attachment #8608196 -
        Flags: review?(wmccloskey)

    
    

    
  


  
(In reply to Bill McCloskey (:billm) from comment #26)
> - enter the PrivilegedJunkScope. otherwise we might get security errors if

I didn't get this part. What kind of security errors do you have in mind? I think the cleanest if we enter the compartment of the array we got from the interposition. So we won't have to deal with any kind of wrappers.
By ensuring that it is from a system scope I think it'll be as safe as it can get.

        Attachment #8608196 -
        Attachment is obsolete: true

        Attachment #8610579 -
        Flags: review?(wmccloskey)

    
    

    
  


  
It's really embarrassing but during testing it turned out that deafultShims are not really turned in because I forgot to assert that we really get the service, and the test for callInterposition did not use the defaultShims. So this is a pre-req for the whitelist patch.

        Attachment #8610581 -
        Flags: review?(wmccloskey)

    
    

    
  
Comment on attachment 8610579 [details] [diff] [review]
InterposeProperty whitelist. v3

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

Looks good. I guess I forgot about the nsTArray resizing. This should be fine though.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +800,5 @@
> +/* static */ InterpositionWhitelist*
> +XPCWrappedNativeScope::GetInterpositionWhitelist(nsIAddonInterposition* interposition)
> +{
> +    if (!gInterpositionWhitelists)
> +        gInterpositionWhitelists = new InterpositionWhitelistArray(MAX_INTERPOSITION);

Let's just return null here and allocate the array in UpdateInterpositionWhitelist instead.

@@ +804,5 @@
> +        gInterpositionWhitelists = new InterpositionWhitelistArray(MAX_INTERPOSITION);
> +
> +    InterpositionWhitelistArray& wls = *gInterpositionWhitelists;
> +    for (size_t i = 0; i < wls.Length(); i++) {
> +        if (wls[i].interposition == interposition) {

No braces here.

@@ +835,5 @@
> +        return false;
> +    }
> +
> +    if (!whitelistVal.isObject()) {
> +        JS_ReportError(cx, "Argument must be an array.");

How about "Whitelist must be an array"?

@@ +866,5 @@
> +            if (!JS_GetElement(cx, whitelistObj, i, &idval))
> +                return false;
> +
> +            if (!idval.isString()) {
> +                JS_ReportError(cx, "Argument must contain strings only.");

"Whitelist must contain strings only."

        Attachment #8610579 -
        Flags: review?(wmccloskey) → review+

        Attachment #8610581 -
        Flags: review?(wmccloskey) → review+

    
    

    
  
I was wondering if we should uplift these patches to aurora...

    
    

    
  
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #31)
> Looks like this landed with the wrong bugnumber and got attributed to bug
> 1164011:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/da3d272ad1c2
> https://hg.mozilla.org/integration/mozilla-inbound/rev/63671ebfa2dd

Uhh... sorry about that and thanks for helping me out!

    
    

    
  
No problem, I'm looking forward to seeing this change in action :)

    
    

    
  
Backed out for crashes in bug 1164011 :( At least you can fix up the commit message now!

    
    

    
  
See bug 1164011 comment 13. (Backout)
Flags: needinfo?(gkrizsanits)

    
    

    
  
I forgot to null-check the whitelist in AddonWrapper after the changes on GetInterpositionWhitelist. Funny how I've got a green try build like that... anyway, fixed the bug and got a green try run, I hope this time it's valid:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c72de78a7ec1

Good thing is that I could change the commit message this way that I messed up earlier.
Flags: needinfo?(gkrizsanits)

    
    

    
  
Assertion failure: (mAudioContextState == AudioContextState::Suspended && aNewState == AudioContextState::Running) || (mAudioContextState == AudioContextState::Running && aNewState == AudioContextState::Suspended) || (mAudioContextState == AudioContextState::Running && aNewState == AudioContextState::Closed) || (mAudioContextState == AudioContextState::Suspended && aNewState == AudioContextState::Closed) || (mAudioContextState == aNewState) (Invalid AudioContextState transit
TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/reftest/tests/dom/media/test/crashtests/868504.html | application terminated with exit code 1
PROCESS-CRASH | file:///C:/slave/test/build/tests/reftest/tests/dom/media/test/crashtests/868504.html | application crashed [@ mozilla::dom::AudioContext::OnStateChanged(void *,mozilla::dom::AudioContextState)]
TEST-UNEXPECTED-FAIL | leakcheck | default process: missing output line for total leaks! 


This is scary... This test only fails on windows xp debug build and seems quite unrelated, but somehow my patch still manages to break it :(

    
    

    
  
So this test is failing on windows XP while I don't do anything audio related or platform specific stuff. I'm pretty sure it has to be some timing related, but anything is possible.

http://hg.mozilla.org/mozilla-central/annotate/f8d21278244b/dom/media/test/crashtests/868504.html
http://mxr.mozilla.org/mozilla-central/source/dom/media/webaudio/AudioContext.cpp#785
https://treeherder.mozilla.org/logviewer.html#?job_id=8052918&repo=try

What I know for sure that I have zero idea what this test should do or what the assertion asserts here and the related bugs do not contain any information. Could you please give me a hint what is going on here? This issue is blocking a very important patch, is there any chance to remove this test until someone can figure out what is wrong with it? (I seriously doubt I can fix this test by changing something on my patch and have no clue about AudioContextState to have a chance to fix the test itself)
Flags: needinfo?(ehsan)

    
    

    
  
These assertions were added recently by Paul in bug 1094764.  If you file a new bug and ni? Paul on it, I think it's OK to disable this test and land your patch, as it's very unlikely that your patch is at fault, beyond perhaps changing the timing of things...
Flags: needinfo?(ehsan)

    
    

    
  
Absolutely, I'm happy with disabling this test on XP, if a bug is filed, and I'm assigned to it. It's is probably a very easy fix.

    
    

    
  
Thanks for the quick feedback guys! After turning off the test treeherder revealed another failing test with the same assertion. Several iteration later now I'm trying to #ifdef out the assertion for windows...

    
    

    
  
Filed Bug 1170547, try looks green (even on windows xp debug), crossing fingers.

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46)
> Backed out for e10s OOMs (which it hit previously too IIRC).

I think you're right. It's just hard to spot an intermittent test to start failing in a 3rd way intermittently among all the other intermittent failing tests... sigh

This drives me nuts. So far the only explanation I found for this is just what the error message says: we're really running out of memory on this platform sometimes. It feels like my patch just gives that last drop into the ocean that this test needed to run out of memory. It does not happen on other platforms or in 64 bit linux. And I could not detect any detectable raise in memory footprint. But it's hard to believe because the only thing this patch really does is interning an array of strings (couple of dozens top most). That seems like a very tiny drop to cause this much trouble... I'm totally stuck here.

Since this is a TenuringTracer crash I needinfo Andrew, he might have a better explanation than I do. So this test seems to parse all the js script we have, and here it seems like we get the same assertion in chrome and content process as well: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gkrizsanits@mozilla.com-3c07c149b8bc/try-linux/try_ubuntu32_vm_test-mochitest-e10s-browser-chrome-1-bm04-tests1-linux32-build912.txt.gz

but you can find more crashes here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c07c149b8bc
or here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfeffadd90b5
a typical stack in human readable format is: https://bug1171109.bugzilla.mozilla.org/attachment.cgi?id=8614842

Bug 1171109 is not helping either... What do you think? Is this really an OOM or can it be something else? String interning can cause issues like this? If it's an OOM what should we do, bumping up the available memory on the 32bit linux box? Breaking up the test?
Flags: needinfo?(continuation)
Flags: needinfo?(wmccloskey)

    
    

    
  
You can look at the MEMORY_STAT stuff to compare total memory usage. If you notice a spike during a certain test, then you could probably add stuff before and after that test to dump a memory report, then use the about:memory diffing functionality to see where the additional memory is going.

I don't know much about our generational GC implemementation, so I'm not sure what this particular crash indicates. Terrence should be able to give you more information about that, but I would guess it is just where we decide to allocate a new 1MB chunk.
Flags: needinfo?(continuation)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch for OOMs
        — 
          — Splinter Review
      

    


  
Gabor, can you please test this patch and see if it fixes the OOM problem?
Flags: needinfo?(wmccloskey)

        Attachment #8616291 -
        Flags: feedback?(gkrizsanits)

    
    

    
  
I filed bug to cover the GC issue uncovered here.

    
    

    
  
Comment on attachment 8616291 [details] [diff] [review]
patch for OOMs

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1ea374cc927

I don't see any more OOMs, so it seems like it fixed the problem. Million thanks for it it would have take my life time to figure this one out. I'm going to push it to mc on Monday, don't want to do a risky push on the weekend.

        Attachment #8616291 -
        Flags: feedback?(gkrizsanits) → feedback+

    
    

    
  
(In reply to Bill McCloskey (:billm) from comment #51)
> I filed bug to cover the GC issue uncovered here.

For the curious: Bug 1172193

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/05aa8f92365f
https://hg.mozilla.org/mozilla-central/rev/973c9ea52413
https://hg.mozilla.org/mozilla-central/rev/e03144903268
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox41:
          --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

    
    

    
  
Comment on attachment 8610579 [details] [diff] [review]
InterposeProperty whitelist. v3

Approval Request Comment
[Feature/regressing bug #]: With this we use the shim layer a lot more conservatively (only when we really need it) which speeds up things significantly.

[User impact if declined]: Since the shim layer is now on aurora, and even if e10s is turned off they are active, there can be performance issues in the developer edition (see: Bug 1167667). 

[Describe test coverage new/current, TreeHerder]: Shim layer has quite good test coverage, this patch does not add any new ones.

[Risks and why]: It was backed out a few times, but those issues were unrelated to this patch. It's a bit more complex patch than what I like to uplift in general, but it should be safe enough. If we don't uplift the developer addition will have performance issues. Even if we uplift it will likely not ride the train any further for a while, which mitigates the risk we're taking with the uplift.

[String/UUID change made/needed]: None

        Attachment #8610579 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8610581 [details] [diff] [review]
fixing defaultShims

Approval Request Comment
[Feature/regressing bug #]: We want to have a default shim layer even for e10s compatible add-ons. It was broken and this patch is to fix them. Pre-req for the shim optimization.

[User impact if declined]: An important API is broken without it, causing delays in e10s work, also the optimization patch relying on this.

[Describe test coverage new/current, TreeHerder]: With the optimization patch it's asserted that the default shim is loaded (unlike before).

[Risks and why]: I don't see any risk.

[String/UUID change made/needed]: None

        Attachment #8610581 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8616291 [details] [diff] [review]
patch for OOMs

Approval Request Comment
[Feature/regressing bug #]: A test started to fail with the optimization patch applied. It's a GC bug, and this is a workaround for it in the test.
[User impact if declined]: Intermittent failures on try with the optimization patch applied.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: An already intermittent test would start failing in a new way on try.
[String/UUID change made/needed]: None

        Attachment #8616291 -
        Flags: approval-mozilla-aurora?

    
    

    
  
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #56)
> Comment on attachment 8610579 [details] [diff] [review]
> InterposeProperty whitelist. v3
> 
> Approval Request Comment
> [Feature/regressing bug #]: With this we use the shim layer a lot more
> conservatively (only when we really need it) which speeds up things
> significantly.
> 
> [User impact if declined]: Since the shim layer is now on aurora, and even
> if e10s is turned off they are active, there can be performance issues in
> the developer edition (see: Bug 1167667). 
> 
> [Describe test coverage new/current, TreeHerder]: Shim layer has quite good
> test coverage, this patch does not add any new ones.
> 
> [Risks and why]: It was backed out a few times, but those issues were
> unrelated to this patch. It's a bit more complex patch than what I like to
> uplift in general, but it should be safe enough. If we don't uplift the
> developer addition will have performance issues. Even if we uplift it will
> likely not ride the train any further for a while, which mitigates the risk
> we're taking with the uplift.
> 
> [String/UUID change made/needed]: None

Eh... sorry I forgot that this patch is changing the UUID of nsIAddonInterposition

    
    

    
  
What is the impact of the UUID change on nsIAddonInterposition?

Why this cannot ride the train with 41? Thanks

    
    

    
  
(In reply to Sylvestre Ledru [:sylvestre] from comment #61)
> What is the impact of the UUID change on nsIAddonInterposition?

nsIAddonInterposition is an internal interface that is only used by us. Previously when add-ons accessed any property on any object that is not from the add-ons own scope we did an interposition. Post patch we do this only for a selected few property names. nsIAddonInterposition has got a new method that returns this whitelist of property names. We need this to avoid unnecessary expensive interposition calls

> 
> Why this cannot ride the train with 41? Thanks

Interpositions just have landed on aurora with 40, and they are causing painful performance issues in some cases, which is an issue for the developer edition (Bug 1167667). This patch could ease the pain. I think the benefit of this patch out-weights the risk by far. Also, without this patch the shims cannot ride the train any further.

    
    

    
  
Comment on attachment 8610579 [details] [diff] [review]
InterposeProperty whitelist. v3

OK, let's do it. 
Is it testable by QA?

        Attachment #8610579 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

        Attachment #8610581 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

        Attachment #8616291 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
(In reply to Sylvestre Ledru [:sylvestre] from comment #63)
> OK, let's do it. 
> Is it testable by QA?

Either asking Justin Wood in Bug 1167667 to test his use case or alternatively using the attached add-on and checking the numbers printed on the console should show significant difference between the pre and post patch versions (for that an optimized release build is highly suggested).

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ffacf2c84367
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/87faf4cd8056
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c828b7442818

    
    

    
  
Please add "PERF" key word.
Depends on: 1262342

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: