1742491 - DevTools Toolbox inappropriately creates new MessageHandlerFrameChild instances

Status: RESOLVED FIXED

(Remote Protocol :: Agent, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Using the session.subscribe() method to register for log.entryAdded events, causes a couple of WINDOW_GLOBAL message handlers to be created. We inappropriately handle these even we do not allow actors for chrome scope yet.

Here the steps:

1. Start Firefox with --remote-debugging-port
2. Load https://client-bidi-demo-sep21.glitch.me/ in a different instance of Firefox
3. Connect to localhost:9222, and hit both green buttons to create a session and register for log events.
4. Go back to the instance of Firefox as started in step 1
5. Open the DevTools toolbox
6. Observe the log output in the terminal

With step 6 you can see a couple of message handler registrations, whereby one gets unregistered again. Also logging the document URI shows:

*** url:

1637614307293	RemoteAgent	TRACE	Received command log._applySessionData for destination WINDOW_GLOBAL
1637614307293	RemoteAgent	TRACE	Module windowglobal/log created for WINDOW_GLOBAL
1637614307293	RemoteAgent	TRACE	Created MessageHandler WINDOW_GLOBAL for session 752d5ce0-c07b-48ca-b283-4ba63ed81cda


*** url: about:devtools-toolbox

1637614307363	RemoteAgent	TRACE	Received command log._applySessionData for destination WINDOW_GLOBAL
1637614307363	RemoteAgent	TRACE	Module windowglobal/log created for WINDOW_GLOBAL
1637614307363	RemoteAgent	TRACE	Created MessageHandler WINDOW_GLOBAL for session 752d5ce0-c07b-48ca-b283-4ba63ed81cda
1637614307380	RemoteAgent	TRACE	MessageHandler WINDOW_GLOBAL for session 752d5ce0-c07b-48ca-b283-4ba63ed81cda is being destroyed
1637614307381	RemoteAgent	TRACE	Unregistered MessageHandler [object WindowGlobalMessageHandler 752d5ce0-c07b-48ca-b283-4ba63ed81cda-WINDOW_GLOBAL-46] for session 752d5ce0-c07b-48ca-b283-4ba63ed81cda


*** url:

1637614307512	RemoteAgent	TRACE	Received command log._applySessionData for destination WINDOW_GLOBAL
1637614307512	RemoteAgent	TRACE	Module windowglobal/log created for WINDOW_GLOBAL
1637614307512	RemoteAgent	TRACE	Created MessageHandler WINDOW_GLOBAL for session 752d5ce0-c07b-48ca-b283-4ba63ed81cda

Adding a breakpoint to the actorCreated method the following stack can be observed for the about:devtools-toolbox URI:

actorCreated (chrome://remote/content/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameChild.jsm#40)
create (resource://devtools/client/framework/toolbox-hosts.js#82)
create (resource://devtools/client/framework/toolbox-host-manager.js#83)
_createToolbox (resource://devtools/client/framework/devtools.js#658)
showToolbox (resource://devtools/client/framework/devtools.js#532)
showToolboxForTab (resource://devtools/client/framework/devtools.js#587)
toggleToolboxCommand (resource://devtools/client/framework/devtools-browser.js#115)
onKeyShortcut (resource://devtools/client/framework/devtools-browser.js#304)
onKey (resource:///modules/DevToolsStartup.jsm#879)
xulKey (resource:///modules/DevToolsStartup.jsm#801)

As it looks like the DevTools toolbox creates a wrapper around our actor which causes an instance to be created. Checking this.manager.isProcessRoot I can temporarily ignore the registration of the message handlers and keep the actor inactive.

Julian, can you please have a look what's wrong here? Thanks.

Not sure if we need it for M2 given that during automation no-one will open the devtools toolbox, but there might be a fundamental issue in cooperation with DevTools. As such setting triage whiteboard entry for further / final discussion.

Comment 1

[Julian Descottes [:jdescottes]]

An important thing here is that we use different codepaths to create the actor if you open the toolbox before subscribing or after.

When I tried your scenario yesterday, I was opening the toolbox before, and didn't get any message handler created for DevTools. This is because we explicitly exclude chrome contexts & parent process contexts for broadcast via:

        // Skip window globals running in the parent process, unless we want to
        // support debugging Chrome context, see Bug 1713440.
        const isChrome = browsingContext.currentWindowGlobal.osPid === -1;

in FrameTransport.jsm

But when we open DevTools after subscribing to log.entryAdded, then we rely on the JSWindowActor to create the MessageHandler. Before we create the MessageHandler, there must have been a DOMWindowCreated event which fits the JSWindowActor definition, and there must be a session data item with a context which matches this new window global. Since all our session data items are for "ALL" at the moment, it means the only "filter" in effect here will be the JSWindowActor definition.

I assumed that our isChrome check in FrameTransport.jsm would be consistent with the includeChrome = false in the JSWindowActor definition (or rather it's lack of value). But since DevTools is using a <browser type="content"> it probably doesn't get excluded by the JSWindowActor logic.

So our issue here is that some chrome contexts are not filtered out as expected, and we'll need to make sure we have a consistent filtering between our broadcast codepath and the sessiondata (which relies on JSWindowActor events) codepath.

In my opinion the way forward is to make the JSWindowActor definition broader (includeChrome true, allFrames true, etc.) and filter with similar logic as the one used in FrameTransport.jsm. Right now the logic to compare the current context to the session data items context descriptor is in _isRelevantContext in WindowGlobalMessageHandler. But we might move this filtering earlier, before we create the MessageHandler, and filter on chrome/parent-process contexts here.

Or we could check with platform if they could exclude DevTools browsing contexts when includeChrome is false? (given that devtools uses type=content, I guess it might be difficult, but maybe worth asking).

As it looks like the DevTools toolbox creates a wrapper around our actor which causes an instance to be created.

I think it's rather that this._browserContainer.appendChild(this.frame); triggers the DOMWindowCreated event for the toolbox browser and therefore leads to create the actor.

Checking this.manager.isProcessRoot I can temporarily ignore the registration of the message handlers and keep the actor inactive.

We probably shouldn't do that. isProcessRoot can also match content frames, I think it describes that a given browsing context is the topmost for its process.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Julian Descottes [:jdescottes] from comment #1)

Thank you for all the details, especially that DevTools is creating a content browser. That clearly explains it. But note that there are still the two extra instances of the client actor created without any URL. I don't know yet where these are getting triggered from.

Or we could check with platform if they could exclude DevTools browsing contexts when includeChrome is false? (given that devtools uses type=content, I guess it might be difficult, but maybe worth asking).

Given the above and that it's type=content I don't think we should exclude it. But feel free to at least ask. Would be good to know what they might think of it.

Checking this.manager.isProcessRoot I can temporarily ignore the registration of the message handlers and keep the actor inactive.

We probably shouldn't do that. isProcessRoot can also match content frames, I think it describes that a given browsing context is the topmost for its process.

Sure. That was just for local testing yesterday and we should do the right checks as you mentioned above.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #2)

Thank you for all the details, especially that DevTools is creating a content browser. That clearly explains it. But note that there are still the two extra instances of the client actor created without any URL. I don't know yet where these are getting triggered from.

These are actually also from DevTools. Here the stacktraces for both of them:

actorCreated (chrome://remote/content/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameChild.jsm#40)
create (resource://devtools/client/framework/toolbox-hosts.js#82)
create (resource://devtools/client/framework/toolbox-host-manager.js#83)
_createToolbox (resource://devtools/client/framework/devtools.js#658)
showToolbox (resource://devtools/client/framework/devtools.js#532)
showToolboxForTab (resource://devtools/client/framework/devtools.js#587)
toggleToolboxCommand (resource://devtools/client/framework/devtools-browser.js#115)
onKeyShortcut (resource://devtools/client/framework/devtools-browser.js#304)
onKey (resource:///modules/DevToolsStartup.jsm#879)
xulKey (resource:///modules/DevToolsStartup.jsm#801)

actorCreated (chrome://remote/content/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameChild.jsm#40)
loadTool (resource://devtools/client/framework/toolbox.js#2619)
loadTool (resource://devtools/client/framework/toolbox.js#2596)
selectTool (resource://devtools/client/framework/toolbox.js#2844)
open (resource://devtools/client/framework/toolbox.js#1017)

The about:devtools-toolbox case comes in-between of these.

Comment 4

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #3)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #2)

Thank you for all the details, especially that DevTools is creating a content browser. That clearly explains it. But note that there are still the two extra instances of the client actor created without any URL. I don't know yet where these are getting triggered from.

These are actually also from DevTools. Here the stacktraces for both of them:

actorCreated (chrome://remote/content/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameChild.jsm#40)
create (resource://devtools/client/framework/toolbox-hosts.js#82)
create (resource://devtools/client/framework/toolbox-host-manager.js#83)
_createToolbox (resource://devtools/client/framework/devtools.js#658)
showToolbox (resource://devtools/client/framework/devtools.js#532)
showToolboxForTab (resource://devtools/client/framework/devtools.js#587)
toggleToolboxCommand (resource://devtools/client/framework/devtools-browser.js#115)
onKeyShortcut (resource://devtools/client/framework/devtools-browser.js#304)
onKey (resource:///modules/DevToolsStartup.jsm#879)
xulKey (resource:///modules/DevToolsStartup.jsm#801)

actorCreated (chrome://remote/content/shared/messagehandler/transports/js-window-actors/MessageHandlerFrameChild.jsm#40)
loadTool (resource://devtools/client/framework/toolbox.js#2619)
loadTool (resource://devtools/client/framework/toolbox.js#2596)
selectTool (resource://devtools/client/framework/toolbox.js#2844)
open (resource://devtools/client/framework/toolbox.js#1017)

The about:devtools-toolbox case comes in-between of these.

DevTools UI is using a lot of iframes. Depending on the panel selected, you get at least one frame in addition to the toolbox's frame, and potentially more (the inspector for sure uses several frames, might be the only panel to do so though).

Although the first stack you added is the same one as the one for about:toolbox. I imagine the first message handler is created for the blank iframe once it's inserted in the DOM and the second one when we set the src to about:devtools-toolbox.

Comment 5

[Julian Descottes [:jdescottes]]

Hi Nika, we realized that JSWindowActors with includeChrome set to false are still created for DevTools UI <browser> elements, most likely because the toolbox <browser> element uses type=content.

It's technically fine to keep it as is, but I imagine developers who defined JSWindowActor with includeChrome=false don't really expect their code to run for DevTools browser elements? Could this be an issue?

Do you think we should exclude them somehow, or should we keep the current behavior?

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1742491 - [remote] Filter out parent process frames in MessageHandlerFrameChild

Comment 7

[Nika Layzell [:nika] (ni? for response)]

TBH I'm not entirely sure what other frontend code is expecting to be happening there. There are a number of content frames which are not like normal content which could be being included by JSWindowActors intentionally, like the sidebar, browser embeds within pages like about:addons, and extension pop-ups. In general code which wants to only apply to web content areas has been using the messageManagerGroups: attribute in the configuration to filter to only the "browsers" group.

I suppose if devtools specifically is quite surprising for frontend engineers, we could opt scrips out of it by default and add an includeDevtools: true option to allow them to be targeted explicitly, but that adds some complexity and might muddy the meaning of includeChrome a bit.

ni? :mconley who probably has a better idea of what frontend developers are expecting than me :-)

Comment 8

[Julian Descottes [:jdescottes]]

Thanks for the feedback Nika! Sounds like I should have used messageManagerGroups here. I'll wait for the feedback from Mike, but I suppose with messageManagerGroups developers have enough filtering options and we probably don't need to make includeChrome more complex.

(For this particular bug, we could either filter on messageManagerGroups, or keep approach in my current patch, filtering manually. In the long run we'll need the actor to run almost everywhere, so filtering manually will become necessary.)

Comment 9

[Mike Conley (:mconley) (:⚙️)]

I agree that using messageManagerGroups is probably the right technique here. I believe it's as simple as adding a new attribute to your <browser type="content"> type browser with messagemanagergroup="devtools". I chose devtools as the group name, but it's really up to you.

Then register your actors with messageManagerGroups: ["devtools"] - I believe that should sequester all of those JSWindowActors to <browser> elements with your group, regardless of process.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69c303049330
[remote] Filter out parent process frames in MessageHandlerFrameChild r=webdriver-reviewers,whimboo

Comment 12

[Cristina Cozmuta (:CrissCozmuta)]

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