1631451 - Remove "tabDetached" event from DevToolsFrameParent and instead destroy the target from the TargetList

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

[Alexandre Poirot [:ochameau]]

To followup on this comment, we should try to remove the unecessary tabDetached RDP packet.
This is made redundant by the target-destroyed-form RDP event.
The TargetList listen to this event, but do not try to destroy the target.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Control DevToolsClient closure from Descriptor instead of Target.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Stop emitting now unused tabDetached event

Now that we are destroying the target via target-destroyed-form, sent via the watcher actor
we no longer need this legacy event.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Improve error logging when a request fails with a custom exception.

Like BrowsingContextTargetActor.detach which was throwing { error: wrongState }
and we were logging "(void 0)" because error.message was undefined,
while error was the thrown JS object.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Make BrowsingContextTargetActor.detach request oneway.

This request may easily fail when called from TargetMixin.destroy.
Leading to pending requests that never finishes.
Note that this method on the front side will destroy the front,
so that even if the request is sent, we can't correctly process the response as the front is already destroyed.

It would be easier to make this oneway, especially given
that we don't expect any returned value.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Bail out if DevToolsClient.close becomes reentrant.

Close method may be called by the transport on close.
This was highlighted by browser_target_from_url.js
Surprisingly devtools/shared/tests/xpcshell/test_debugger_client.js
tries quite hard to cover such issue, but we seem to get yet another type of reentrancy.

Comment 6

[Phabricator Automation]

Comment on attachment 9208345 [details]
Bug 1631451 - [devtools] Improve error logging when a request fails with a custom exception.

Revision D107986 was moved to bug 1698554. Setting attachment 9208345 [details] to obsolete.

Comment 7

[Phabricator Automation]

Comment on attachment 9206975 [details]
Bug 1631451 - [devtools] Stop emitting now unused tabDetached event

Revision D107249 was moved to bug 1698541. Setting attachment 9206975 [details] to obsolete.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Always listen to target-available and destroyed events from TargetCommand.

This is to allow destroying correctly the targets even if we aren't watching for additional targets.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Destroy TabDescriptor instead of Target from toolbox and tests.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Destroy target front via Watcher's target-destroyed-form.

That, instead of tabDetached event, fired on the target actor themself.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Ignore connection closed when closing browser toolbox in tests.

This is to fix intermittent failure on all browser toolbox tests.
It looks like these patches make toolbox.destroy shuts down connection faster
and lead to evaluateJSAsync request still be pending while the connection is closed
and actors and fronts are destroyed.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/810bdfa0d899
[devtools] Bail out if DevToolsClient.close becomes reentrant. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/d7a1c44ea37c
[devtools] Always listen to target-available and destroyed events from TargetCommand. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/da765754e3a8
[devtools] Destroy TabDescriptor instead of Target from toolbox and tests. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/b5cb78fd1f61
[devtools] Destroy target front via Watcher's target-destroyed-form. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/bc9072114706
[devtools] Control DevToolsClient closure from Descriptor instead of Target. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/8cc10a259de1
[devtools] Ignore connection closed when closing browser toolbox in tests. r=nchevobbe

Comment 13

[Cosmin Sabou [:CosminS]]

Backed out for causing devtools and xpcshell failures.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=0d9035ed5c50b569d09fab60d4e64c16c362de10&selectedTaskRun=B6cCh9skSbiADrrmcp1q7A.0

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=333573797&repo=autoland
https://treeherder.mozilla.org/logviewer?job_id=333573618&repo=autoland

Backlout link: https://hg.mozilla.org/integration/autoland/rev/0f098000e0e79fbe1b1f4542de062b7429454d49

Comment 15

[Phabricator Automation]

Comment on attachment 9208347 [details]
Bug 1631451 - [devtools] Bail out if DevToolsClient.close becomes reentrant.

Revision D107988 was moved to bug 1699432. Setting attachment 9208347 [details] to obsolete.

Comment 16

[Phabricator Automation]

Comment on attachment 9209373 [details]
Bug 1631451 - [devtools] Always listen to target-available and destroyed events from TargetCommand.

Revision D108576 was moved to bug 1699432. Setting attachment 9209373 [details] to obsolete.

Comment 17

[Phabricator Automation]

Comment on attachment 9209374 [details]
Bug 1631451 - [devtools] Destroy TabDescriptor instead of Target from toolbox and tests.

Revision D108577 was moved to bug 1699432. Setting attachment 9209374 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9209457 [details]
Bug 1631451 - [devtools] Ignore connection closed when closing browser toolbox in tests.

Revision D108630 was moved to bug 1699432. Setting attachment 9209457 [details] to obsolete.

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1631451 - [devtools] Prevent re-entrant destroy loop in Pool.destroy.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae55f558b993
[devtools] Destroy target front via Watcher's target-destroyed-form. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/08465dc7451d
[devtools] Control DevToolsClient closure from Descriptor instead of Target. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/bbcf6c8aefdc
[devtools] Prevent re-entrant destroy loop in Pool.destroy. r=jdescottes

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ae55f558b993
https://hg.mozilla.org/mozilla-central/rev/08465dc7451d
https://hg.mozilla.org/mozilla-central/rev/bbcf6c8aefdc