1281731 - Destroy the toolbox when toolbox.xul unloads

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P2)

Description

[Alexandre Poirot [:ochameau]]

When opening about:devtools-toolbox in a tab, if you close the Tab, the toolbox will still be around and continue debugging the target. It is especially painful when using about:devtools-toolbox?type=process where we have a browser toolbox slowing down everything.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Comment 2

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d8426c69c24

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

STR:
- open about:devtools-toolbox?type=process
- open another tab
- close about:devtools-toolbox tab

Without the patch:
- the toolbox actor are still on and you see many "Would run" messages

With the patch:
- toolbox actor are shut down and there shouldn't be any more messages coming from them,
  also, firefox should be running full speed once you close the toolbox tab!

Comment 4

[Alexandre Poirot [:ochameau]]

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

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8764602 [details] [diff] [review]
patch v2

Review of attachment 8764602 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, I need to try harder to merge my Positron commits more quickly...  I did something very similar over there:

https://github.com/mozilla/positron/commit/7fb9923dade8cced3a6e02f0c1e30cc6c3fda056

Should I land the Positron version to avoid conflicts, or do you prefer yours?

::: devtools/client/framework/toolbox-init.js
@@ -65,5 @@
> -    targetFromURL(url).then(target => {
> -      let options = { customIframe: host };
> -      return gDevTools.showToolbox(target, tool, Toolbox.HostType.CUSTOM, options);
> -    }).then(null, e => {
> -      window.alert("Unable to start the toolbox:" + e.message);

Any interest in keeping this logging?  It could be a catch handler for the entire Task.spawn perhaps.  Maybe console.log / error is better than alert?

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8764602 [details] [diff] [review]
patch v2

I guess I'll clear the review for the moment while we discuss which version to land.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ochameau, how do you want to proceed here?

Comment 8

[Alexandre Poirot [:ochameau]]

Our patch are mosly similar, except that this one get rid of the error logger (I should reintroduce it)
and used more Task, which looks like a good change regarding readiness.

Is that any more complex if I proceed with this patch and you just drop the positron patch on the next merge with central?

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8764602 [details] [diff] [review]
patch v2

Okay, let's proceed with this one then.  Please do consider some kind of error logging.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:myk, just so you're aware, this bug will conflict with a similar change that was landed in Positron.  Next time you merge upstream, I can help untangle the conflict.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

With error handling to the console.

Comment 12

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a574d4a2b89

Comment 13

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/51b82683c933e5b0c6e61581bd5015487a005dde
Bug 1281731 - Destroy the toolbox on toolbox.xul unload. r=jryans

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51b82683c933

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> :myk, just so you're aware, this bug will conflict with a similar change
> that was landed in Positron.  Next time you merge upstream, I can help
> untangle the conflict.

Thanks for the tip!  I merged from upstream in https://github.com/mozilla/positron/pull/96 and describe a problem I'm having with the toolbox there.