Closed Bug 1695929 Opened 1 year ago Closed 1 year ago

Use a descriptor event to show the error page for remote debugging.

Categories

(DevTools :: General, task, P3)

task

Tracking

(Fission Milestone:M7, firefox88 fixed)

RESOLVED FIXED
88 Branch
Fission Milestone M7
Tracking Status
firefox88 --- fixed

People

(Reporter: jdescottes, Assigned: ochameau)

References

Details

(Whiteboard: dt-fission-m3-mvp)

Attachments

(4 files, 2 obsolete files)

See https://phabricator.services.mozilla.com/D106426#inline-596735

Using a descriptor event will allow to attach to the event before the toolbox started. Otherwise we might miss targets destroyed before the toolbox was fully opened.

I might address such edge case in https://phabricator.services.mozilla.com/D106835 by introducing descriptor-destroyed event.
At least I had to tweak the code showing the error page.

I might use this bug to proceed just with the new descriptor-destroyed event.

I'll try to use this bug to introduce a server side descriptor-destroyed event.
This will help bug 1694651 which is struggling around descriptor/target destructions.

Assignee: nobody → poirot.alex

Here is a little summary on what is going on around destroy codepaths.

Important takeaways from my recent experience around destroy is that:

  • surprisingly there isn't many callsites to toolbox.destroy
  • the server code order is super important. tabDetach and cancelForwarding/purgeRequests are both racing. The purgeRequests disabling front events can be confusing, while being super helpful regarding memory...
  • we listen to TabClose from all the places :)

Various typical destroy codepath, which may race each others:

  • Toolbox document removal from the DOM:
    • fires unload event and calls Toolbox.destroy()
    • itself ultimately calling Target.destroy(), with shouldCloseClient=true, will close the client
    • and destroy all fronts. which may disable other codepath as no event will fire on fronts anymore
  • TabClose event is also watched by TabDescriptor
    • and calls toolbox.destroy()
  • "tabDetached" emitted by the server:
    • occurs in many cases:
      • when client calls detach(), which only occurs when Target.destroy is called.
        This is only meant to be useful for remote debugger, when we keep using the same client.
      • BrowsingContextTargetActor.detach() is called by exit() itself called by destroy()
        so anytime the target actor is destroy, we will emit tabDetached
      • when the tab's message manager closes (or in near future when JSWindowActor is destroyed = WindowGlobal is destroyed)
        we also emit tabDetached
    • tl;dr; server emits tabDetached if the tab closes (message manager is destroyed, TabClose will destroy the target actor)
      ,or, when the client called target.detach()
    • on tabDetached we destroy the target, with shouldCloseClient=true, we will close the client and everything

When and who destroy the toolbox:

  • TabDescriptor on TabClose
  • Toolbox on unload

When and who destroy the target:

  • DevToolsClient on closed
  • Target on tabDetached

When and who destroy the DevToolsClient:

  • Target on destroy if shouldCloseClient=true
  • itself on connection drops

The case of cancelForwarding and purgeRequests:

  • It destroy all fronts based on a given prefix
  • it happens when the message manager closes (tab closing, target switching may be?) and when the connection drops
  • It is about the connection.cancelForwarding(prefix) calls from the backend and purgeRequests in the client
  • it is important as it is close to where we emit tabDetached and easily races with it.
  • As soon as the fronts are destroyed, no more events are fired on them.
Blocks: 1694651

This is mostly used by tests and we do not really benefit from having two distinct events.
We are not using the fact that close is emitted immediately.

This will help close the toolbox and client only when the tab closes.
We no longer have to workaround target switching.
It also helps making "worker-close" event less special.

This should help catching immediately destroyed targets/descriptors
and no longer be impacted by top level target being destroyed on target switching.

Attachment #9206940 - Attachment description: Bug 1695929 - [devtools] Use descriptor-destroyed for showing disconnexion message in toolboxes. → Bug 1695929 - [devtools] Use descriptor-destroyed for showing disconnection message in toolboxes.
Blocks: 1696471
Attachment #9206940 - Attachment description: Bug 1695929 - [devtools] Use descriptor-destroyed for showing disconnection message in toolboxes. → Bug 1695929 - [devtools] Use descriptor-destroyed for showing disconnexion message in toolboxes.
Attachment #9206956 - Attachment is obsolete: true
Attachment #9206636 - Attachment description: Bug 1695929 - Remove redundant TargetFrontMixin's close event, in favor of target-destroyed. → Bug 1695929 - [devtools] Remove redundant TargetFrontMixin's close event, in favor of target-destroyed.
Attachment #9206637 - Attachment description: Bug 1695929 - Emit descriptor-destroyed on all descriptor actor when their related context is closed/destroyed. → Bug 1695929 - [devtools] Emit descriptor-destroyed on all descriptor actor when their related context is closed/destroyed.
Attachment #9206940 - Attachment description: Bug 1695929 - [devtools] Use descriptor-destroyed for showing disconnexion message in toolboxes. → Bug 1695929 - [devtools] Use descriptor-destroyed for showing disconnection message in toolboxes.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afd5ecd702da
[devtools] Remove redundant TargetFrontMixin's close event, in favor of target-destroyed. r=jdescottes,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/c716d7eeb478
[devtools] Emit descriptor-destroyed on all descriptor actor when their related context is closed/destroyed. r=jdescottes,devtools-backward-compat-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/0fb780c2319a
[devtools] Use descriptor-destroyed for showing disconnection message in toolboxes. r=jdescottes,nchevobbe

The previous changeset slightly changes when the TabDescriptor is destroyed.
It is now destroyed when the target is destroyed in case of descriptors created via listTabs.
This better translates whats happens with remote debugging.

Attachment #9206940 - Attachment description: Bug 1695929 - [devtools] Use descriptor-destroyed for showing disconnection message in toolboxes. → Bug 1695929 - [devtools] Use descriptor-destroyed for showing disconnexion message in toolboxes.
Blocks: 1697109
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eea9263de09d
[devtools] Remove redundant TargetFrontMixin's close event, in favor of target-destroyed. r=jdescottes,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/b1f65ab7edec
[devtools] Emit descriptor-destroyed on all descriptor actor when their related context is closed/destroyed. r=jdescottes,devtools-backward-compat-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/dd618e7f4314
[devtools] Use descriptor-destroyed for showing disconnexion message in toolboxes. r=jdescottes,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/c54dc8cc17c9
[devtools] Assert the new behavior of TabDescriptors between getTab and listTabs. r=jdescottes

Comment on attachment 9207449 [details]
Bug 1695929 - [devtools] Always destroy worker and webextension descriptors when descriptor-destroyed is sent.

Revision D107466 was moved to bug 1697109. Setting attachment 9207449 [details] to obsolete.

Attachment #9207449 - Attachment is obsolete: true
Fission Milestone: --- → M7
Whiteboard: dt-fission-m3-mvp
Blocks: 1698671
No longer blocks: 1698671
You need to log in before you can comment on or make changes to this bug.