1134006 - [e10s] Observer service shim causes unexpected CPOW traffic

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Dave Townsend [:mossop]]

When the observer service shim forwards a message to a registered observer it triggers two CPOW messages. I think because the observer is a wrapped nsIObserver just calling its observe method is causing xpconnect to call QueryInterface on the subject to confirm that it is in fact an nsISupports of some kind.

Gabor, can you confirm that and is there any way around it?

CPOW from parent: get <child HTMLDocument:8:10e6d90e0>."QueryInterface" = <child Function:9:116e84980>
#0    0x10dfe00d0   resource://gre/modules/RemoteAddonsParent.jsm:334 (0x110364830 @ 129)
#1    0x10dfe0020   resource://gre/modules/RemoteAddonsParent.jsm:323 (0x110364768 @ 71)
CPOW from parent: <child Function:9:116e84980>.call(<child Function:9:116e84980>, <child HTMLDocument:8:10e6d90e0>, <JSIID>) = <child HTMLDocument:8:10e6d90e0>
#0    0x10dfe00d0   resource://gre/modules/RemoteAddonsParent.jsm:334 (0x110364830 @ 129)
#1    0x10dfe0020   resource://gre/modules/RemoteAddonsParent.jsm:323 (0x110364768 @ 71)

Comment 1

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend [:mossop] from comment #0)
> Gabor, can you confirm that and is there any way around it?

Yes, I think that makes sense. I don't know any workaround off the top of my head, I will try to think it over some more. I mean, I could imagine that if we know for sure that the subject is an nsISupport and that operation has no value for us, that we can wrap the subject in an object on the parent side that implements QI, which returns the subject CPOW... but that seems like a very dirty trick, and even if it works I wouldn't feel too comfortable suggesting it. Might worth a try just for the fun of it. Is there an easy way I can reproduce this for debugging?  (I guess it matters what observer are you using...)

Comment 2

[Dave Townsend [:mossop]]

Attachment: test.xpi

Anything that triggers the shims will do. This add-on registers an empty observer for "document-element-inserted". Loading any remote page causes CPOW traffic to be logged. I experimented with substituting a proxy as we've done for events and found that it's attempting to QI to nsIClassInfo which makes sense from my limited xpconnect understanding. I could probably use a proxy to get around that but proxies fail to respond correctly to instanceof and I know that will break observers.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Right. Thanks for the test, so it's a problem with a simple js observer too... There should be a workaround I'm trying to find it, just I haven't looked into this code since ages. My guess is that xpconnect creates a wrapper around the observer when addObserver is called if it's a vanilla js function object. So it can handle it as an nsIObserver internally (nsXPCWrappedJS), and then when we get it back via enumerateObservers then I think it gets double wrapped (XPCWrappedNative). If that is the case then if we could unwrap it and get back the original vanilla js function, we could avoid the whole xpconnect machinery when we do the that tries to convert the arguments (with the QI and all the black magic). I have a vague memory that it was possible to do that with .wrappedJSObject (not the same thing as the one on xray wrappers just has the same name). Anyway, I'm working on this a bit more and will get back with the results.

Comment 4

[Dave Townsend [:mossop]]

Attachment: experiment

This is a quick experiment I put together. By wrapping the proxy in the nsISupportsInterfacePointer suddenly "instanceof Ci.nsISupports" works! But also the trapped QueryInterface is never called for anything other than nsISupports or nsIClassInfo (for an object that doesn't implement nsIClassInfo). Nor is there any CPOW traffic. I don't understand that. Feels like this might be close, any ideas?

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend [:mossop] from comment #4)
> This is a quick experiment I put together. By wrapping the proxy in the
> nsISupportsInterfacePointer suddenly "instanceof Ci.nsISupports" works! But

I think that's because it gets wrapped. But it still surprises me that this works...

> also the trapped QueryInterface is never called for anything other than
> nsISupports or nsIClassInfo (for an object that doesn't implement

These calls are coming from the native function call machinery, that is triggered from
the wrapped observer to convert the argument into an nsISupport. The nsIClassInfo part
is kind of an optimization. I would implement only the nsISupports case.
What I don't know is if the observer were some native observer and not just a simple js function
(let's say with DOM binding) what would be the call path here... I hope that's not a valid use case for us?

> nsIClassInfo). Nor is there any CPOW traffic. I don't understand that. Feels
> like this might be close, any ideas?

So the only CPOW traffic at this point should be that QI call, that you avoided with this hack, so that's fine. The question is what side effects might we have on this new, super tricky subject argument inside the registered observer call. It's now some sort of nsXPCWrappedJS around a proxy that forwards the operations to a CPOW (which has it's own layers as well)... I could not even guess about the possible side effects at this point tbh, but I'm officially scared :)

Comment 6

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8566646 [details] [diff] [review]
experiment

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

I pass the ball to Blake, he knows these ancient wrappers far better than I probably... I still think the unwrapping approach would be the proper solution, but that might take a while to implement... this approach is quite scary, but it does seem like a tempting temporary workaround for avoiding all the unnecessary CPOW traffic here, so I think it is worth to be considered.

Comment 7

[Dave Townsend [:mossop]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> (In reply to Dave Townsend [:mossop] from comment #4)
> > This is a quick experiment I put together. By wrapping the proxy in the
> > nsISupportsInterfacePointer suddenly "instanceof Ci.nsISupports" works! But
> 
> I think that's because it gets wrapped. But it still surprises me that this
> works...

Kind of odd yes, I copied it from here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#32

> > also the trapped QueryInterface is never called for anything other than
> > nsISupports or nsIClassInfo (for an object that doesn't implement
> 
> These calls are coming from the native function call machinery, that is
> triggered from
> the wrapped observer to convert the argument into an nsISupport. The
> nsIClassInfo part
> is kind of an optimization. I would implement only the nsISupports case.

But xpconnect seems to always check for nsIClassInfo too, if I don't implement that case its going to cause CPOW traffic.

> What I don't know is if the observer were some native observer and not just
> a simple js function
> (let's say with DOM binding) what would be the call path here... I hope
> that's not a valid use case for us?
> 
> > nsIClassInfo). Nor is there any CPOW traffic. I don't understand that. Feels
> > like this might be close, any ideas?
> 
> So the only CPOW traffic at this point should be that QI call, that you
> avoided with this hack, so that's fine.

The bit missing from this patch is that my observer attempts to QI to Ci.nsIDOMDocument, that doesn't cause any CPOW traffic and throws so there is something going wrong.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8566646 [details] [diff] [review]
experiment

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

I have to admit, I'm not sure how I feel about this extra wrapper (and I can't say for certain that I fully understand how the CPOW, proxy, and additional nsISupportsInterfacePointer all work together). This feels really hacky to me, and I would rather do something at the CPOW level (at least that way, we'll get similar benefits for other cases, though I guess there might not be many of them?).

I have a patch that I'll attach in a second that fixes this bug and isn't *too* ugly (I think). I'll attach it here so we can see the two patches side by side.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Alternative experiment

Here's my alternative patch.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8574204 [details] [diff] [review]
Alternative experiment

Yeah I'd much prefer to do this than mess with JS proxies. Looks like this makes QI for nsISupports or nsIClassInfo a no-op for DOM objects, sounds good.

I assume that xpconnect has a list of interfaces it already knows an object to implement, either from nsIClassInfo calls or other inference, presumably it would be possible to forward that with the CPOW to avoid the need to QI in those cases too. No idea how much work that is or how much of a benefit it would have though.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Dave Townsend [:mossop] from comment #10)
> I assume that xpconnect has a list of interfaces it already knows an object
> to implement, either from nsIClassInfo calls or other inference, presumably
> it would be possible to forward that with the CPOW to avoid the need to QI
> in those cases too. No idea how much work that is or how much of a benefit
> it would have though.

I think that without nsIClassInfo this has gotten harder. If we find that this is a significant amount of CPOW traffic, then it wouldn't be too hard to fix on the CPOW side; but let's cross that bridge when we get to it.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8574204 [details] [diff] [review]
Alternative experiment

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

::: js/ipc/WrapperOwner.cpp
@@ +424,5 @@
> +                return true;
> +            }
> +
> +            // Webidl-implemented DOM objects never have nsIClassInfo.
> +            if (idptr->Equals(NS_GET_IID(nsIClassInfo))) {

No braces here.

@@ +465,5 @@
>  
> +    AuxCPOWData *data = AuxCPOWDataOf(proxy);
> +    if (data->isDOMObject &&
> +        idVar.type() == JSIDVariant::TnsString &&
> +        idVar.get_nsString().EqualsLiteral("QueryInterface")) {

The brace should go on its own line since this is a multi-line conditional.

@@ +468,5 @@
> +        idVar.type() == JSIDVariant::TnsString &&
> +        idVar.get_nsString().EqualsLiteral("QueryInterface")) {
> +        // Handle QueryInterface on DOM Objects specially since we can assume
> +        // certain things about their implementation.
> +        RootedFunction QueryInterface(cx,

Maybe just call this qi.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: For checkin

This patch (which is what I pushed to try) addresses all of Bill's nits.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Sheriffs: please note that attachment 8574885 [details] [diff] [review] depends on the test patch (also checkin-needed) for bug 1140636.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c7ebb2f5a92

Comment 17

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=7405397&repo=mozilla-inbound

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Merged patch

This lost a race to remove the parent argument to JS_NewFunction. Let's try again.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69c682891670

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69c682891670