1648499 - Use JSProcessActor to debug content processes

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Today, DevTools uses Process Message Managers to connect and debug content processes.
But they are meant to be replaced by JSProcessActors, introduced in bug 1580448.
We should remove the legacy code using Message managers:
https://searchfox.org/mozilla-central/source/devtools/server/connectors/content-process-connector.js
https://searchfox.org/mozilla-central/rev/a87a1c3b543475276e6d57a7a80cb02f3e42b6ed/devtools/server/actors/descriptors/process.js#92-107
https://searchfox.org/mozilla-central/rev/a87a1c3b543475276e6d57a7a80cb02f3e42b6ed/devtools/server/actors/process.js#23-34
In favor of something using JSProcessActors.

This work may depend on bug 1620248 or block it. It isn't clear yet.
Bug 1620248 would get rid of the content-process-connector, which is hard to convert.
But this work would help bug 1620248 by making content process debugging use an Actor and be closer to the existing code listening to frame, which uses JSWindowActor.

Comment 1

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Attachment: Bug 1648499 - WIP to try to use JSProcessActor.

We still need to convert content-process-connector, but that isn't trivial.
It would be great to reuse or share some code with DevToolsFrame JSWindowActor,
but it only work if the target is created by the WatcherActor, from the content process.
Whereas process target are still created on-demand, from the frontend, via ProcessDescriptor.getTarget.

Comment 2

[Chris Peterson [:cpeterson]]

Adding dt-fission whiteboard tag to DevTools bugs that mention Fission or block Fission meta bugs but don't already have a dt-fission whiteboard tag.

Comment 3

[Chris Peterson [:cpeterson]]

Moving dt-fission bugs to Fission MVP.

Comment 4

[Alexandre Poirot [:ochameau] PTO, back August 11th]

In this bug we could probably try to addres this review comment:
https://phabricator.services.mozilla.com/D65529#inline-531419
which higlights that we aren't calling watcher.notifyTargetDestroyed when the content process target actor is manually destroyed.

While refactoring this piece of code to JSProcessActors, we should try to better update the actors map and remove items from the Arrays when a target actor is destroyed.

Comment 5

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Another limitation of bug 1620248:
https://phabricator.services.mozilla.com/D65529#inline-532990
If we open many Browser toolboxes against the same firefox,
when closing one, we will unconditionally remove the "process script", leading to various issues.

Comment 6

[Neha Kochar [:neha]]

Bulk change of all bugs with whiteboard tag of dt-fission to Fission MVP milestone.

Comment 7

[Chris Peterson [:cpeterson]]

Moving old "dt-fission" bugs to "dt-fission-future" because they don't block Fission MVP.

Comment 8

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Unfortunately this is slightly more complex than expected because the JS Process Actor is loaded in the shared global
Because of that it is considered a debuggee and this mess up with the debugger.
Typically, we get JSProcessActor's receiveMessage function triggered while the debugger is paused and this breaks it.
The Debugger.Frame on which we paused is no longer the last one on the stack... And it is as if the debugger wasn't fully pausing the content process.

We didn't have this issue with Message Manager because we could register message listeners from the devtools global directly to the C++ code:
mm.addMessageListener(function)
This is no longer possible with JS Process actors, you have to have a JS Actor file, with a receiveMessage function, which can only be loaded in the shared global.

We should probably find a way to somehow load the JS Actors in the devtools global.

Comment 9

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Attachment: Bug 1648499 - [devtools] Avoid calling JSWindowActor's method from JsWindowActorTransport

This avoid calling shared global's code from browser toolbox server's custom global.
This prevent many "xxxx would run yyyy" exceptions.

Comment 10

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Attachment: Bug 1648499 - [devtools] Avoid attaching BrowsingContext thread actors in the context of the browser toolbox.

Comment 11

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Attachment: Bug 1648499 - [devtools] Use JS Process actors to debug content process via DevTools.

We were using Process script to and process message managers to interact with content processes.
This patch migrates the whole logic to JS Process Actor.

Note that we may still use old API in process descriptor, but that's a different story.

This helps align frame process and worker all around JS Actors,
making it easier to align all three target types to the same logic.

We may be able to start sharing more code?

Comment 12

[Alexandre Poirot [:ochameau] PTO, back August 11th]

Attachment: Bug 1648499 - [devtools] Pass isModeSwitching for content process targets

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebcd4a352206
[devtools] Avoid attaching BrowsingContext thread actors in the context of the browser toolbox. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/31b9ef95a482
[devtools] Use JS Process actors to debug content process via DevTools. r=devtools-reviewers,nchevobbe

Comment 14

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/ebcd4a352206
https://hg.mozilla.org/mozilla-central/rev/31b9ef95a482