1473828 - RDP packets are wrapped in a compartment different from the module loader one

Status: NEW

(DevTools :: Framework, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

While looking at performance regression of bug 1471853 (regression due to browser loader), it occurs to me that RDP packets and especially the "attributes" one from the inspector actors are coming form another compartment (it is hard to say which one) and so it causes to involve cross compartment wrapper anytime we query any packet.

Here is a profile of DAMP on mozilla-central, when running custom.inspector.open/reload:
  https://perfht.ml/2MVe3DJ

Let's focus on getAttribute
https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/devtools/client/shared/node-attribute-parser.js#240-247
function getAttribute(attributes, attributeName) {
  for (const {name, value} of attributes) {
    if (name === attributeName) {
      return value;
    }
  }
  return null;
}

You see that this simple function has to cross compartments when accessing `attributes`. This attributes is some data sent by the inspector actor, it ultimately comes from this function within the transport layer:
  https://searchfox.org/mozilla-central/rev/1193ef6a61cb6e350460eb2e8468184d3cb0321d/devtools/shared/transport/transport.js#734-736
When node-attribute-parser is loaded in the main devtools loader, both transport.js and node-attribute-parser.js should be loaded in the same compartment and do not have any overhead related to wrappers.
So it looks like the `data` object passe to `receiveMessage` is bound to some other compartment, may be the tab child global? I'm not sure how to assert that.
But I think that this relates to 1449231.

Based on that, I tried using StructuredCloneHolder to ensure wrapping the packets in the right compartment, the main loader compartment. It has a significant and very positive impact on the inspector, but it regresses the protocoljs test and debugger stepIn:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=a554da86c8c6ea693d6bc3ad110a88a34c1eb138&newProject=try&newRevision=c4355a351e904bdd334f218dc700c13836ebc113&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1473828 - Use weak message manager listener to have message data be in sandbox compartment.

Review commit: https://reviewboard.mozilla.org/r/255268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255268/

Comment 2

[Alexandre Poirot [:ochameau]]

For some reason I can't really explain, using StructuredCloneHolder manually like this was introducing an overhead at GC level as if we had an overhead while destroying the previous packets.

But I found out that messageManager.addWeakMessageListener did not regressed from bug 1449231
and that we can recover the right compartment for RDP packets:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=413b493db32c282c87c143fbb5a5430c5aba0b3c&newProject=try&newRevision=37a26141201a44806fc58ab5b51f8a8c7e461aa6&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1
This time only panelsInBackground regressed, but I'll assume that just because of tests overlap.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8990233 [details]
Bug 1473828 - Use weak message manager listener to have message data be in sandbox compartment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255268/diff/1-2/

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8990233 [details]
Bug 1473828 - Use weak message manager listener to have message data be in sandbox compartment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255268/diff/2-3/

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8990233 [details]
Bug 1473828 - Use weak message manager listener to have message data be in sandbox compartment.

Wait, I'm still looking forward a green try before going for review.
I may also clean things up around worker and transport.js as that's full of historical cruft.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8990233 [details]
Bug 1473828 - Use weak message manager listener to have message data be in sandbox compartment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255268/diff/3-4/

Comment 7

[Alexandre Poirot [:ochameau]]

For some reason, it breaks the responsive design.
I tried hacking into tunnel.js without much success yet.

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

Note that this patch is now based on top of bug 1000814 and bug 1474980.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The tunnel currently skips weak listeners and just passes them through, so that sounds related, but I guess that's the part you tried adjusting...  Anyway, maybe it's better to fix the core issue with compartments directly?