Closed Bug 1281731 Opened 4 years ago Closed 4 years ago

Destroy the toolbox when toolbox.xul unloads

Categories

(DevTools :: Framework, defect, P2)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
Attached patch patch v2 (obsolete) — Splinter Review
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!
Attachment #8764602 - Flags: review?(jryans)
Attachment #8764507 - Attachment is obsolete: true
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 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.
Attachment #8764602 - Flags: review?(jryans)
:ochameau, how do you want to proceed here?
Flags: needinfo?(poirot.alex)
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?
Flags: needinfo?(poirot.alex)
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.
Attachment #8764602 - Flags: review+
: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.
Attached patch patch v3Splinter Review
With error handling to the console.
Attachment #8765820 - Flags: review+
Attachment #8764602 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/51b82683c933
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.