1531704 - WorkerTargetFront destroy/detach sequence is confusing when opened from openWorkerToolbox

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

  async openWorkerToolbox(workerTarget, toolId) {
    const toolbox = await gDevTools.showToolbox(workerTarget, toolId, Toolbox.HostType.WINDOW);
    toolbox.once("destroy", () => workerTarget.detach());
  },

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/devtools/client/framework/devtools-browser.js#386

I think this is the only toolbox where the client is explicitly calling detach() on the target at the moment.

This leads to calling detach() twice when we close a toolbox inspecting a worker

- target.detach() <- called by devtools-browser via the event listener
- target.destroy() <- called by destroyToolbox
- target.detach() <- called by the destroy(), will log a warning because connection is closed

If we stop calling detach first, then the cleanup is not done correctly because WorkerTargetFront destroy will set a flag "isClosed":

  destroy() {
    this._isClosed = true;

    super.destroy();
  }

https://searchfox.org/mozilla-central/source/devtools/shared/fronts/targets/worker.js#44

and this flag will make the detach bail out early:

  async detach() {
    if (this.isClosed) {
      return {};
    }

I think the reason behind this flag is that WorkerTargetFront::detach() calls WorkerTargetFront::destroy() which calls TargetMixin::destroy() which calls WorkerTargetFront::detach() (etc... etc...).

I propose to make the detach/destroy sequence closer to the other target fronts and remove the call to detach() from openWorkerToolbox.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1531704 - Stop calling WorkerTargetFront::destroy() from detach()

Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=02f79fac79a313124cfe6368812e84d823fab685

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1531704 - Remove isClosed flag from WorkerTargetFront

Depends on D21673

The test is already relying on events triggered by the destroy method of the front to progress
I don't think having a flag brings anything here.

Comment 3

[Julian Descottes [:jdescottes]]

Try is green except for browser_dbg-windowless-workers-early-breakpoint.js in debug builds, but it seems unrelated. I also have the failure locally without my changes, I guess it's an issue linked to artifact builds, as I get failures such as

JavaScript error: resource://devtools/server/actors/worker/worker-target-actor-list.js, line 57: TypeError: dbg.setDebuggerReady is not a function

Comment 4

[Julian Descottes [:jdescottes]]

rebased try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cecba9cf36be39896d5f3d2db8a69cc6f9b2d8b0

Comment 5

[Julian Descottes [:jdescottes]]

new try after addressing review comments:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a96c619056b628403aa3171fae6c542621172524

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3c262330737
Stop calling WorkerTargetFront::destroy() from detach() r=ochameau
https://hg.mozilla.org/integration/autoland/rev/95c5a6730ef6
Remove isClosed flag from WorkerTargetFront r=ochameau

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c3c262330737
https://hg.mozilla.org/mozilla-central/rev/95c5a6730ef6