1474843 - ContentFrameMessageManager roots `data` object passed to message listeners to ContentFrameMessageManager or BackstagePass instead of listener's compartment

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

Bug 888600 changed the compartment of message manager `data` object passed to Javascript listener functions. Instead of being the compartment of the JS function, it is always the ContentFrameMessageManager or BackstagePass one, depdending if you are listening from the content process or the parent process.

It regressed significantly devtools as we use the message manager to pipe all DevTools data and we now hit cross compartment overhead as DevTools modules run in sandbox compartments. I imagine it also impacts other code using content content frame message manager outside of frame scripts (jsm/xpcom).

I tracked it down to the ReceiveMessage:
https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/dom/base/nsFrameMessageManager.cpp#710
  object = webIDLListener->CallbackOrNull();

Where `object` here, for some reason I can't explain, has ContentFrameMessageManager or BackstagePass as global. I would expect it to be the global of the JS function we pass to AddMessageListener. In Devtools it would be a Sandbox global.

It is important for `object` to be coming from the right global as it will later impact `data` object final compartment via:

https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/dom/base/nsFrameMessageManager.cpp#724-725
  AutoEntryScript aes(object, "message manager handler");
  JSContext* cx = aes.cx();

https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/dom/base/nsFrameMessageManager.cpp#746
  aCloneData->Read(cx, &json, aError);

https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/dom/ipc/StructuredCloneData.cpp#104
  nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));

Note that AddWeakMessageListener is still correct and root the data object to the passed function's compartment.
I'll use that in DevTools as a workaround, but still, I think this may have regressed performance of any JSM/xpcom using the ContentFrameMessageManager.

Comment 1

[Peter Van der Beken [:peterv]]

As part of the WebIDL conversion we switched to WebIDL behaviour for converting and running callbacks, which is more in line with how the rest of the Web platform works. The important bit is to know where the addMessageListener call is happening, that should determine the callback context we use for the receiveMessage call.

We need to go through XPConnect to implement the weak listeners, so those still use the legacy behaviour. I wish we could remove them entirely.

Comment 2

[Alexandre Poirot [:ochameau]]

The call to `addMessageListener` is done over here:
https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/devtools/shared/transport/transport.js#708
    _addListener() {
      this._mm.addMessageListener(this._messageName, this);
    },

This transport.js module is loaded via the DevTools loader.
Because of that it runs in a Sandbox and so I'm expecting to see receiveMessage from this same class here:
https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/devtools/shared/transport/transport.js#734-736
    receiveMessage: function({data}) {
      this.hooks.onPacket(data);
    },
To be called with `data` being hosted on the Sandbox compartment and not the ContentFrameMessageManager one.
As `this` passed as addMessageListener second argument comes from the Sandbox compartment.
Is that wrong assumption to have?

This code is very easy to trigger, you only need to open DevTools against a tab loaded in the content process.

I don't know if the precise definition of `_mm` is important here?
It comes from here:
  https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/devtools/server/startup/frame.js#37
frame.js is loaded as a frame script.
`target` should be this following IDL property:
  https://searchfox.org/mozilla-central/source/dom/chrome-webidl/MessageManager.webidl#172

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Test message manager data's global.

Here is a test script to highlight what is wrong.
Before 888600, or with addWeakMessageListener, `data`'s global is the Sandbox object.
Now it always is ContentFrameMessageManager object, or frame script global.

Which makes all Sandboxes (so all Devtools modules) and all JSMs to hit an overhead
because all message manager data will be in another global and force to have cross compartment wrappers.

If that helps, here is a script that you can run in scratchpad/browser console:
  gBrowser.selectedBrowser.messageManager.loadFrameScript("data:,global = this; new " + function () {
    const sandbox = Cu.Sandbox(Services.scriptSecurityManager.getSystemPrincipal());
    sandbox.console = console;
    sandbox.Cu = Cu;
    sandbox.mm = global;
    Cu.evalInSandbox("global = this; new " + function () {
      mm.addMessageListener("foo", function onFoo(message) {
        mm.removeMessageListener("foo", onFoo);
        console.log("in-content", Cu.getGlobalForObject(message.data) == global, Cu.getGlobalForObject({}) == global);
      });
    }, sandbox);
  }, false);
  gBrowser.selectedBrowser.messageManager.sendAsyncMessage("foo", { bar: 1 })

Comment 4

[Alexandre Poirot [:ochameau]]

Also see bug 1472491 comment 60 about why it may impact current work to reduce memory usage of frame scripts.

Comment 5

[Peter Van der Beken [:peterv]]

Thanks for the testcase, I'll take a look.

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: Use listener's compartment to create argument

Comment 7

[Peter Van der Beken [:peterv]]

Using the listener's own compartment to create the argument for the listener does seem to improve some things:

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=dd72686554664580d19f4fccf6edf898e82e9e77&newProject=try&newRevision=dbf875a05bdb8ee84719aeb19ca3a58b919e9826&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=12

However, it doesn't resolve all the regressions from bug 888600. There's a lot of regressions/improvements that don't overlap (comparing with: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=18a940f854bfa1df873b9b1701ecb6b9e5e3ce59&newProject=try&newRevision=0376052261bfaeba6b827acb12d74eecbf9397f9&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1). And it regresses at least one test that didn't regress with the original landing (custom.inspector.collapseall.balanced), and it regresses one test that improved with the original landing (panelsInBackground.reload.DAMP).

Alexandre, any idea what's going on? I don't think I'd mind landing this, but I'd like to know why there are so many differences in the tests as to what regressed and what resolving this issue improves.

Comment 9

[Alexandre Poirot [:ochameau]]

Thanks a lot for looping back on this Peter!

custom.inspector.collapseall.balanced regression could easily be ignored.
While the percentage is high (41%), the absolute difference is low (6ms).
It could easily be one reflow or more likely a GC that is shifted and happens
during that test.

Regarding panelsInBackground.reload.DAMP, I don't remember having looked into this.
So it is not clear why this has been improved by bug 888600 and regressed here.
We may have some code here evaluated in a frame script...
By looking at a diff between two profiles without/with your fix:
  https://perfht.ml/2EaYgRN
Nothing really obvious stands out.
I'll try to gather some more profiles as in this one subtest last for 600ms both with/without the fix while using the profiler...

> Alexandre, any idea what's going on? I don't think I'd mind landing this,
> but I'd like to know why there are so many differences in the tests as to
> what regressed and what resolving this issue improves.

This patch clearly fixes bug 1473828. DevTools packets no longer go through CrossCompartmentWrapper when being accessed,
which makes tests involving a lot of packets faster (inspector opening/reload, debugger pause/steps,...).
This improvement is big and most likely impact many DevTools features!
I can followup on the regression for panels in background if you want to proceed.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 9015451 [details] [diff] [review]
Use listener's compartment to create argument

I tried to compare profiles without/with the patch but I can't find any obvious culprit.

Here is two distinct profile comparisons between two DAMP runs:
https://perfht.ml/2AQGY7O
https://perfht.ml/2UeKykL

I tried to look at any additional CrossCompartment frames, but can't see any obvious one.

But again the win on all others makes this patch worth the regression on background panel performance. So do not hesitate to move forward with it.

Comment 11

[Alexandre Poirot [:ochameau]]

Actually, I found something wrong about the test. There is a lot of frames related to network-monitor that only appear with the patch. It looks totally unrelated to the patch as the test itself do not appear to wait correctly for these network-monitor related code.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Peter, looks you've been working on this, assigning to you. Feel free to reset if I misunderstand it.

Comment 13

[Alexandre Poirot [:ochameau]]

Is there anything around this code that changed recently?
It looks like this patch no longer work as expected:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=1422ea3ae2a4a8a76dc7a0acdbf53f166e3b5af3&newProject=try&newRevision=42b28be1f9b3251d1b4eadab389906b9575435a3&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=12

I applied your patch on today's mozilla-central.
Earlier in December the patch was still improving DevTools perf.

Comment 14

[Jan de Mooij [:jandem]]

Alexandre, for what it's worth, my WIP patches for bug 1512029 do improve damp tests. I didn't retrigger these jobs so the data isn't super reliable, but some sub tests improved a lot:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=a394b4ee0dec&framework=12&selectedTimeRange=172800

This puts sandboxes created with the system-principal in the same compartment as the shared JSM global. Are there other compartments we care about for devtools?

Anyway it might be worth waiting for bug 1512029 - I'm actively working on it and I'll do my best to land it this year.

Comment 15

[Alexandre Poirot [:ochameau]]

(In reply to Jan de Mooij [:jandem] from comment #14)
> Alexandre, for what it's worth, my WIP patches for bug 1512029 do improve
> damp tests. I didn't retrigger these jobs so the data isn't super reliable,
> but some sub tests improved a lot:
> 
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> central&newProject=try&newRevision=a394b4ee0dec&framework=12&selectedTimeRang
> e=172800
> 
> This puts sandboxes created with the system-principal in the same
> compartment as the shared JSM global. Are there other compartments we care
> about for devtools?

See bug 1512029 comment 19 for more detail about your patch impact.
This patch is about yet another border and yet another kind of cross compartment wrappers.
This is about cross compartment wrappers happening between a devtools module (no matter which loader we use) and the message manager API, which sets the objects into the frame script's compartment instead of the listener's one.

But nevermind, I was pushing to talos with --artifact, I'm so used to modify only the Javascript.
Sorry about that unnecessary noise!

Comment 16

[Alexandre Poirot [:ochameau]]

So I was able to confirm that the regression to panelsInBackground is related to DAMP test itself.
I opened bug 1513265 to address that.

If I push your patch on top of a fix for that, we only get improvements being reported.
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=b9ae59f98af79bf1aac3cb2ebf6f4326e88db147&newProject=try&newRevision=2b1ec14b23f95fe1189ee8f1a3dec3478b1340eb&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=12

Peter, could you proceed with this patch? Thanks!

Comment 17

[Peter Van der Beken [:peterv]]

Is this still relevant? We're not too keen on taking this :-/.

Comment 18

[Alexandre Poirot [:ochameau]]

No. This all disappeared with the merge of all privileged compartments.