Closed Bug 1133141 Opened 5 years ago Closed 5 years ago

[e10s] EventTargetParent events cause unnecessary CPOW traffic

Categories

(Toolkit :: General, defect)

x86
macOS
defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.3 - 23 Feb
Tracking Status
e10s m5+ ---
firefox38 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
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
Attachment #8564424 - Flags: feedback?(wmccloskey)
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.
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
Blocks: 1129663
Attached patch patchSplinter Review
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.
Attachment #8564424 - Attachment is obsolete: true
Attachment #8564424 - Flags: feedback?(wmccloskey)
Attachment #8564588 - Flags: review?(wmccloskey)
Blocks: 1133834
No longer blocks: 1129663
Status: NEW → ASSIGNED
Flags: qe-verify-
Flags: firefox-backlog+
Attachment #8564588 - Flags: review?(wmccloskey) → review+
Iteration: --- → 38.1 - 26 Jan
Points: --- → 2
Iteration: 38.1 - 26 Jan → 38.3 - 23 Feb
https://hg.mozilla.org/mozilla-central/rev/95a87349301f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 1129567
You need to log in before you can comment on or make changes to this bug.