1133141 - [e10s] EventTargetParent events cause unnecessary CPOW traffic

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Dave Townsend [:mossop]]

When handing the EventProxy to an add-on we attempt to interpose it and do a bunch of unnecessary CPOW traffic checking what kind of object it is.

Comment 1

[Dave Townsend [:mossop]]

Attachment: patch

I'm trying to reduce the CPOW traffic associated with event handling. Lots of event listeners try to access the type or target properties of an event and we already have these in the parent so there should be no need to do CPOW traffic to get them. Here is where I am at but things are still not right.

I worked around the instanceof checks by implementing a QueryInterface through the proxy but there is still CPOW traffic. Same with type and target. In each case the proxy get trap is called and returns the property from prefilled but immediately after we do CPOW traffic to call getOwnPropertyDescriptor on the event anyway. I don't understand why. The proxy's getOwnPropertyDescriptor trap is never called. An example log:

*** overrode QueryInterface
CPOW from parent: <child Event:8:117212850>.getOwnPropertyDescriptor("QueryInterface")
#0            0x0   file:///Users/dave/mozilla/build/nightly/dist/Nightly.app/Contents/Resources/components/multiprocessShims.js:87 (0x11047a9c0 @ 47)
#1            0x0   file:///Users/dave/mozilla/build/nightly/dist/Nightly.app/Contents/Resources/components/multiprocessShims.js:121 (0x11047aa88 @ 56)
#2    0x10d9e0348   resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/windows/observer.js:45 (0x1228fa8f8 @ 20)
#3    0x10d9e02b0   resource://gre/modules/RemoteAddonsParent.jsm:547 (0x1104a1c18 @ 33)
#4    0x10d9e0210   resource://gre/modules/Prefetcher.jsm:457 (0x110490510 @ 68)
#5    0x10d9e00e8   resource://gre/modules/RemoteAddonsParent.jsm:545 (0x1104a18f8 @ 653)
#6    0x10d9e0020   resource://gre/modules/RemoteAddonsParent.jsm:489 (0x1104a1830 @ 100)

*** overrode type
CPOW from parent: <child Event:8:117212850>.getOwnPropertyDescriptor("type")
#0    0x10d9e0348   resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/commonjs/sdk/windows/observer.js:45 (0x1228fa8f8 @ 20)
#1    0x10d9e02b0   resource://gre/modules/RemoteAddonsParent.jsm:547 (0x1104a1c18 @ 33)
#2    0x10d9e0210   resource://gre/modules/Prefetcher.jsm:457 (0x110490510 @ 68)
#3    0x10d9e00e8   resource://gre/modules/RemoteAddonsParent.jsm:545 (0x1104a18f8 @ 653)
#4    0x10d9e0020   resource://gre/modules/RemoteAddonsParent.jsm:489 (0x1104a1830 @ 100)

console.log: test: Saw event activate

Comment 2

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

The behavior you're seeing is in the spec:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-proxy-object-internal-methods-and-internal-slots-get-p-receiver

Basically, when calling the "get" hook on a scripted proxy, we call GetOwnPropertyDescriptor on the target object. It seems to be purely for error checking purposes. Pretty annoying though.

Perhaps we shouldn't use a Proxy at all for the event. There aren't that many properties on events. We could just send them all up.

Comment 3

[Dave Townsend [:mossop]]

Yeah just found that. I just tried something different, create an object with the event as its prototype, seems to work in a simple test so I'm running it through try to see if anything breaks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de948ae1ee59

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch

Even if we try to send all properties across we will still need to do some CPOW traffic and special handling in some cases (preventDefault and defaultPrevented for example) so for now I think it still makes sense to use a proxy.

I've figured out how to make the proxy work with no CPOW traffic. Instead of proxying the event object instead proxy an object that contains the properties we want to override and then return event properties in other cases. This drops CPOW traffic for simple events listeners only accessing type and target to nothing. We have scope to add more properties to this in the future if we want.

Interestingly the proxy is never instanceof Ci.nsIDOMEventTarget (or nsISupports either) but apparently this is true of the current proxy too. The current proxy doesn't event have a working QueryInterface (I think because it returns the real event rather than the proxy), that doesn't appear to break anything but I added a basic working one anyway.

This gave a miraculously completely green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a05e7465b4e. I'm assuming our code and e10s tests already abuse the proxied event object enough that we can be pretty confident this is good.

Comment 5

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/95a87349301f

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95a87349301f