Closed
Bug 1281731
Opened 8 years ago
Closed 8 years ago
Destroy the toolbox when toolbox.xul unloads
Categories
(DevTools :: Framework, defect, P2)
DevTools
Framework
Tracking
(firefox50 fixed)
RESOLVED
FIXED
Firefox 50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(1 file, 2 obsolete files)
4.23 KB,
patch
|
ochameau
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d8426c69c24
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8764507 -
Attachment is obsolete: true
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b71f610850a3
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)
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
With error handling to the console.
Attachment #8765820 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8764602 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a574d4a2b89
Assignee | ||
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/51b82683c933e5b0c6e61581bd5015487a005dde Bug 1281731 - Destroy the toolbox on toolbox.xul unload. r=jryans
Priority: -- → P2
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/51b82683c933
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Comment 15•8 years ago
|
||
(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.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•