1604211 - Support target switching for DevTools webextensions

Status: RESOLVED FIXED

(DevTools :: General, task, P1)

Description

[Julian Descottes [:jdescottes]]

Follow up to a conversation in the review of Bug 1566850:
https://phabricator.services.mozilla.com/D57103#inline-347930

DevTools webextensions rely on the console actor id for the "inspect" binding, which evaluates code in the context of the debugged page. With Fission and target switching, retrieving the console actor once will no longer be enough as the target and actors will change during the lifecycle of the toolbox.

See the phabricator discussion for more context about this webextensions API. The goal of this bug is to at least support target switching (meta bug 1592575).

In a second step we can look into supporting remote frames and contexts.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking DevTools bugs for Fission Nightly (M6)

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1604211: Introduce target-switching mechanism for devtools inspectedWindow.

Comment 3

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 9133120 [details]
Bug 1604211: Introduce target-switching mechanism for devtools inspectedWindow.

Hello!
I wrote a patch to support target-switching for DevTools webextension.
However, as I am not sure whether this approach is good or not since the architecture is quite different from other tools, couldn't you give your opinions?
(All tests under browser/components/extensions/test/browser/ were passed.)

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Daisuke Akatsuka (:daisuke) from comment #3)

Comment on attachment 9133120 [details]
Bug 1604211: Introduce target-switching mechanism for DevTools webextension.

Hello!
I wrote a patch to support target-switching for DevTools webextension.
However, as I am not sure whether this approach is good or not since the architecture is quite different from other tools, couldn't you give your opinions?
(All tests under browser/components/extensions/test/browser/ were passed.)

hi Daisuke,
I just took an initial look to D66736, there is one detail that I'd like to double-check with you:

If I didn't miss any part of D66736 that was dealing with this, with the current version of D66736 getInspectedWindowFront is going to create a new cloned targetFront for every call to browser.devtools.inspectedWindow.eval and/or browser.devtools.inspectedWindow.reload, whereas currently we create a targetFront clone only once.
Would you mind to double-check if that is the case? (and looking into tweaking the patch accordingly if it is the case).

The reason for the cloned targetFront is that we decided (agreed together with ochameau when we initially worked on the devtools API) that the extensions should use a separate "Remote Debugging Protocol" connection to prevent them from impacting the RDP messages exchanged by the integrated devtools panels (e.g. if a misbehaving extension is flooding its own RDP connection with RDP messages).

It would also nice to make sure that the new implementation actually works on a fission tab that is navigating to an url that requires a process switch (and so also a target switch from a devtools perspective) by having a test case to explicitly test that scenario.

I'd also like to give to this scenario a try locally, is the following enough as an STR?

• setting "devtools.target-switching.enabled" to true
• setting "fission.autostart" to true
• install a minimal test extension that includes a devtools page and/or devtools panel and calls browser.devtools.inspectedWindow.eval
• navigate between two urls that should load in two separate child processes and check that calling browser.devtools.inspectedWindow.eval is working as expected before and after navigating between these two urls

Comment 5

[Daisuke Akatsuka (:daisuke)]

Hi Luca, thank you very much for your feedback!

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #4)

If I didn't miss any part of D66736 that was dealing with this, with the current version of D66736 getInspectedWindowFront is going to create a new cloned targetFront for every call to browser.devtools.inspectedWindow.eval and/or browser.devtools.inspectedWindow.reload, whereas currently we create a targetFront clone only once.
Would you mind to double-check if that is the case? (and looking into tweaking the patch accordingly if it is the case).

The reason for the cloned targetFront is that we decided (agreed together with ochameau when we initially worked on the devtools API) that the extensions should use a separate "Remote Debugging Protocol" connection to prevent them from impacting the RDP messages exchanged by the integrated devtools panels (e.g. if a misbehaving extension is flooding its own RDP connection with RDP messages).

Yes, I also had thought we need to cache manually.
As you said, we call DevToolsShim.createWebExtensionInspectedWindowFront() for every call browser.devtools.inspectedWindow.*, actually. Then, inside the function, we call tabTarget.getFront("webExtensionInspectedWindow").
https://searchfox.org/mozilla-central/source/devtools/client/framework/devtools.js#688-690
And it seems that the getFront has a cache mechanism, returns cached front if the front was created already.
https://searchfox.org/mozilla-central/source/devtools/shared/fronts/targets/target-mixin.js#187-202
I confirmed the instance that is returned after 2nd times is the same one as the instance returned at first time. And of course, a new instance will be created at target-switching.
Thus, I think we don't have to keep the instance manually, but we'd better cache explicitly?
(Or, we'd better rename createWebExtensionInspectedWindowFront to getWebExtensionInspectedWindowFront??)
What do you think??

It would also nice to make sure that the new implementation actually works on a fission tab that is navigating to an url that requires a process switch (and so also a target switch from a devtools perspective) by having a test case to explicitly test that scenario.

Yep, I'll add a test!

I'd also like to give to this scenario a try locally, is the following enough as an STR?

• setting "devtools.target-switching.enabled" to true
• setting "fission.autostart" to true
• install a minimal test extension that includes a devtools page and/or devtools panel and calls browser.devtools.inspectedWindow.eval
• navigate between two urls that should load in two separate child processes and check that calling browser.devtools.inspectedWindow.eval is working as expected before and after navigating between these two urls

I am usually testing with navigating between a page running on content process and running parent process.
like follows:

• setting "devtools.target-switching.enabled" to true
• install a test extension that includes a devtools page and calls browser.devtools.inspectedWindow.reload()
• test whether reloading works well or not while navigating between normal page (e.g. http://exmaple.com) and a page running on parent process such as about:robots.

Thanks!

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Daisuke Akatsuka (:daisuke) from comment #5)

Hi Luca, thank you very much for your feedback!

:daisuke thanks for these details, based on the code you linked it seems that we may not need to cache the target front also in the devtools API internals if that is already being done internally by the devtools internals now.

I took another look, and if I'm not missing something the current version of D66736 the WebExtensions devtools API isn't creating a separate RDP connection for the extension context (and so the extension context will be using the same one used by the integrated devtools).

In the current version a new RDP connection is created for the extension context (if one hasn't been created yet) from getDevToolsTargetForContext, by calling DevToolsShim.createTargetForTab(tab); (which internally creates a new connection here).

If that is right (and I'm not missing something that is making me reading it wrong), then I think that we should likely be doing something like:

• create a new RDP connection for the extension context and retrieve the initial top level target (the tabTarget)
• create a new TargetList to watch the target switching for the RDP connection related to the specific extension context

does this sounds right to you too?

Comment 7

[Daisuke Akatsuka (:daisuke)]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #6)

In the current version a new RDP connection is created for the extension context (if one hasn't been created yet) from getDevToolsTargetForContext, by calling DevToolsShim.createTargetForTab(tab); (which internally creates a new connection here).

If that is right (and I'm not missing something that is making me reading it wrong), then I think that we should likely be doing something like:

• create a new RDP connection for the extension context and retrieve the initial top level target (the tabTarget)
• create a new TargetList to watch the target switching for the RDP connection related to the specific extension context

does this sounds right to you too?

Oh, I see! I had missed the logic and had not known about we should create a new RDP connection for web extension.
I am not sure if we need another TargetList, but let me try to fix!

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1604211: Address target-switching for devtools network.

Depends on D66736

Comment 9

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1604211: Add test of target-switching for inspectedWindow.

Depends on D67438

Comment 10

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1604211: Add test of target-switching for devtools network.

Depends on D67439

Comment 11

[Chris Peterson [:cpeterson]]

Tentatively moving P1 Fission M6 bugs to M6a.

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9133120 [details]
Bug 1604211: Introduce target-switching mechanism for devtools inspectedWindow.

I'm marking this as f+ because the approach sounds reasonable to me, and I'm going back to actually review the patch asap (my apologies for the time I needed to actually come back to this)

:daisuke would you mind to move D67438 and D67440 to Bug 1626494?

Comment 13

[Daisuke Akatsuka (:daisuke)]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #12)

Comment on attachment 9133120 [details]
Bug 1604211: Introduce target-switching mechanism for devtools inspectedWindow.

I'm marking this as f+ because the approach sounds reasonable to me, and I'm going back to actually review the patch asap (my apologies for the time I needed to actually come back to this)

:daisuke would you mind to move D67438 and D67440 to Bug 1626494?

Thanks, yes sure!

Comment 14

[Phabricator Automation]

Comment on attachment 9134344 [details]
Bug 1604211: Address target-switching for devtools network.

Revision D67438 was moved to bug 1626494. Setting attachment 9134344 [details] to obsolete.

Comment 15

[Phabricator Automation]

Comment on attachment 9134346 [details]
Bug 1604211: Add test of target-switching for devtools network.

Revision D67440 was moved to bug 1626494. Setting attachment 9134346 [details] to obsolete.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bed5bc16127c
Introduce target-switching mechanism for devtools inspectedWindow. r=rpl,ochameau,jdescottes
https://hg.mozilla.org/integration/autoland/rev/8f794de69bea
Add test of target-switching for inspectedWindow. r=rpl

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/bed5bc16127c
https://hg.mozilla.org/mozilla-central/rev/8f794de69bea