1709267 - Support target switching for about:debugging tab debugging

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P3)

Description

[Julian Descottes [:jdescottes]]

When debugging tabs (remotely or not) from about:debugging, target switching is not supported.

If the target navigates and changes process, the about:devtools-toolbox that was debugging it will "close" (the tab will still be around but it will show an error message).

Prerequisite:

• enable fission
• check that local tab debugging is enabled (devtools.aboutdebugging.local-tab-debugging to true)

STRs:

• open a tab on wikipedia.org
• open about:debugging in another tab
• select This Nightly (or This Firefox)
• click on Inspect for the wikipedia.org tab
• (an about:devtools-toolbox tab will open and show a toolbox debugging the wikipedia tab)
• navigate the wikipedia.org tab to mozilla.org (either from the about:debugging UI or from the real URL bar, doesn't matter)

ER: The toolbox should still be displayed
AR: The about:devtools-toolbox tab will now show an error message

Once we support target switching for about:debugging, we should be able to remove the isLocalTab check at https://searchfox.org/mozilla-central/rev/185ab5e4f4e01341e009cd4633d1275ffe4d4c8b/devtools/client/fronts/descriptors/tab.js#165

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: toolbox_tab_after_navigation.png

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1709267 - [devtools] Add support for target switching in about:debugging.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1709267 - [devtools] Remove unused "target" query parameter from toolbox location.

This wasn't used except for a test and wasn't working with server side targets.
Making this compatible with SST wasn't trivial, so I went for removing this.

Comment 4

[Alexandre Poirot [:ochameau]]

It looks like I finally got a green try:
https://treeherder.mozilla.org/jobs?repo=try&revision=3c149e94b14b55f2dd455c5d446c5b6712b794d9

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88f46043876e
[devtools] Remove unused "target" query parameter from toolbox location. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/e4dc58404c6c
[devtools] Add support for target switching in about:debugging. r=jdescottes

Comment 7

[Natalia Csoregi [:nataliaCs]]

Backed out for causing devtools failures

• backout: https://hg.mozilla.org/integration/autoland/rev/120476bc666b7ab2eceeaf6f9983a3048fa5728c

• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=e4dc58404c6ca58d3de526dcd1628d430cefe114&selectedTaskRun=Zw1LN07sT26WLo7ZdENFVQ.0

• failure logs:

• TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging/test/browser/browser_aboutdebugging_devtoolstoolbox_navigate_back_forward.js | A promise chain failed to handle a rejection: Connection closed, pending request to server0.conn1.windowGlobal19327352833/inspectorActor4, type getWalker failed

• TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_target_parents.js | Uncaught exception - at resource://devtools/shared/protocol/Front.js:365 - Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWorkerDebugger.postMessage]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://devtools/server/connectors/worker-connector.js :: connectToWorker/< :: line 102" data: no] fro

• TEST-UNEXPECTED-FAIL | devtools/client/framework/test/browser_about-devtools-toolbox_reload.js | Test timed out -

• [TEST-UNEXPECTED-FAIL | devtools/shared/commands/target/tests/browser_target_command_various_descriptors.js | Uncaught exception - at resource://devtools/shared/protocol/Front.js:365 - Error: Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWorkerDebugger.postMessage]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://devtools/server/connectors/worker-connector.js :: connectToWorker/<

Comment 8

[Alexandre Poirot [:ochameau]]

I think I got rid of all the failures now:
https://treeherder.mozilla.org/jobs?repo=try&revision=a5c75c0de083510622ba85f446e7aee8df78b986
Hopefully no other intermittent will appear.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec5eb72262cc
[devtools] Remove unused "target" query parameter from toolbox location. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/da9d3297348d
[devtools] Add support for target switching in about:debugging. r=jdescottes

Comment 10

[Sandor Molnar[:smolnar]]

Backed out for causing dt failures in browser_about-devtools-toolbox_reload.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/1f6ee488fb6bdc3d7b8bb18d0ad7c1b5ce141939

Push with failures

Failure log

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1709267 - [devtools] Fix duplicated transports when detaching the top target.

We ended up having duplicated JSWindowActorTransport's when detaching the target.
This only reproduces in case of remote debugging, where we detach/destroy the top target
when closing the toolbox. And we later reuse the same DevToolsClient to spawn a new
WindowGlobalTargetActor, with a new Transport.
But as the old transport was still around, we ended up duplicating the packets.

In this patch, I'm also tuning WindowGlobalTargetActor.destroy to avoid crashing
if the target wasn't attached when we destroy it, and avoid trying to destroy
twice if destroy is reentrant.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1709267 - [devtools] Fix duplicated transports when detaching the top target.

We ended up having duplicated JSWindowActorTransport's when detaching the target.
This only reproduces in case of remote debugging, where we detach/destroy the top target
when closing the toolbox. And we later reuse the same DevToolsClient to spawn a new
WindowGlobalTargetActor, with a new Transport.
But as the old transport was still around, we ended up duplicating the packets.

In this patch, I'm also tuning WindowGlobalTargetActor.destroy to avoid crashing
if the target wasn't attached when we destroy it, and avoid trying to destroy
twice if destroy is reentrant.

Comment 13

[Alexandre Poirot [:ochameau]]

Should be good now with the new changeset:
https://treeherder.mozilla.org/jobs?repo=try&revision=416e60c10fd97b27383c7022b3d8326f392f9a4c

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd7440c8b49b
[devtools] Remove unused "target" query parameter from toolbox location. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/eb3b0be05e16
[devtools] Fix duplicated transports when detaching the top target. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/8f2bd81028da
[devtools] Add support for target switching in about:debugging. r=jdescottes

Comment 15

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/cd7440c8b49b
https://hg.mozilla.org/mozilla-central/rev/eb3b0be05e16
https://hg.mozilla.org/mozilla-central/rev/8f2bd81028da

Comment 16

[Chris Peterson [:cpeterson]]

Setting status-firefox95=wontfix because I assume we don't need to uplift target switching to Beta 95.