Closed Bug 1631451 Opened 2 years ago Closed 10 months ago

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

Categories

(DevTools :: Framework, enhancement)

enhancement

Tracking

(Fission Milestone:M7a, firefox89 fixed)

RESOLVED FIXED
89 Branch
Fission Milestone M7a
Tracking Status
firefox89 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-fission-m3-mvp)

Attachments

(3 files, 7 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

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.

See Also: → 1631728
Depends on: 1696471

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

Depends on: 1697490

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.

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.

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.

Depends on: 1697869
Blocks: 1698541
Blocks: 1698554

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.

Attachment #9208345 - Attachment is obsolete: true
Attachment #9208346 - Attachment is obsolete: true

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.

Attachment #9206975 - Attachment is obsolete: true
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED

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

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

Attachment #9206974 - Attachment description: Bug 1631451 - [devtools] Destroy target from TargetList.onTargetDestroyed and get rid of tabDetached → Bug 1631451 - [devtools] Control DevToolsClient closure from Descriptor instead of Target.

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.

Blocks: 1698842
Blocks: 1698876
Blocks: 1698890
Blocks: 1699111
Pushed by apoirot@mozilla.com:
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
Duplicate of this bug: 1687965
Depends on: 1699432

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.

Attachment #9208347 - Attachment is obsolete: true

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.

Attachment #9209373 - Attachment is obsolete: true

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.

Attachment #9209374 - Attachment is obsolete: true

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.

Attachment #9209457 - Attachment is obsolete: true
Depends on: 1699493
Flags: needinfo?(poirot.alex)
No longer blocks: 1698842
Depends on: 1698842
Blocks: 1644397
Whiteboard: dt-fission-m3-triage
Whiteboard: dt-fission-m3-triage → dt-fission-m3-mvp
Fission Milestone: --- → M7a
Fission Milestone: M7a → M8
Fission Milestone: M8 → M7a
Pushed by apoirot@mozilla.com:
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
Regressions: 1703545
You need to log in before you can comment on or make changes to this bug.