Closed
Bug 1302148
Opened 8 years ago
Closed 8 years ago
Toolbox document leaks in many possible ways
Categories
(DevTools :: Framework, defect)
DevTools
Framework
Tracking
(firefox52 fixed)
RESOLVED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(4 files, 4 obsolete files)
I don't get why the toolbox is always reported as leaking during tests, but there is many places where we leak the toolbox documents from CommonJS Modules. Modules outlive all documents. Module sandboxes are allocated once and stay alive until Firefox closes. So any reference from such module to any devtools document like toolbox or webconsole.xul, inspector.xul, ... should report a leak. But it only happens when working on some random patches modifying slightly toolbox codepath. I was finally able to produce a little helper to highlight all these references and fix those for the leaks reported in bug 1266134. Hopefully, fixing those will help us landing patches without having these misterious leaks. But I suspect I introduce another, higher level leak in bug 1266134 which introduce a toolbox leak...
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•8 years ago
|
||
I was finally able to get rid of these leaks locally, hopefully try is going to confirm that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2bd4a48073b The second patch is the one that actually fix the leaks I've hit when working on bug 1266134. Every single modification in this patch removes a leak and I needed all these to stop leaking the toolbox document, which itself leak the tools one. The first is just helping not leaking everything when we leaks something. The last fixes a race where we would call toolbox.destroy from some nested code initiated by toolbox.destroy itself.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
Tweaked the gcli fix to ignore unwanted unload events.
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8790359 [details] Bug 1302148 - Fix potential race in toolbox.destroy. https://reviewboard.mozilla.org/r/78236/#review76770 ::: devtools/client/framework/toolbox.js:2053 (Diff revision 2) > // destruction promise so we're sure to destroy only once > if (this._destroyer) { > return this._destroyer; > } > + let deferred = defer(); > + this._destroyer = deferred.promise; I don't see how this is a race - there is no asyncness between here and the current assignment to `this._destroyer`. Can you explain what is causing the issue here?
Attachment #8790359 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #8) > Comment on attachment 8790359 [details] > Bug 1302148 - Fix potential race in toolbox.destroy. > > https://reviewboard.mozilla.org/r/78236/#review76770 > > ::: devtools/client/framework/toolbox.js:2053 > (Diff revision 2) > > // destruction promise so we're sure to destroy only once > > if (this._destroyer) { > > return this._destroyer; > > } > > + let deferred = defer(); > > + this._destroyer = deferred.promise; > > I don't see how this is a race - there is no asyncness between here and the > current assignment to `this._destroyer`. > > Can you explain what is causing the issue here? There is no need of asyncness to have a race. May be race is not the best word. If any code called between the test: `if(this._destroyer)` and the set: `this._destroyer = ` i.e., this chunk: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#2056-2140 ends up calling toolbox.destroy(), this _destroyer isn't a guard anymore and we will start destroying stuff multiple times and start throwing. I got into this situation within a test calling toolbox.destroy multiple times like this. Not calling toolbox.destroy(), but having toolbox.destroy() calling itself within the dependencies.
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8790358 [details] Bug 1302148 - Fix various code leaking devtools toolbox. https://reviewboard.mozilla.org/r/78234/#review76774 ::: devtools/client/shared/developer-toolbar.js:160 (Diff revision 2) > > command.state.onChange(target, onChange); > onChange("", { target: target }); > - document.defaultView.addEventListener("unload", () => { > - if (command.state.offChange) { > - command.state.offChange(target, onChange); > + }; > + let listener = function (event) { > + if (event.target != document) { Why would event.target not be document? ::: devtools/client/shared/theme-switching.js:179 (Diff revision 2) > } else { > switchTheme(Services.prefs.getCharPref("devtools.theme")); > > gDevTools.on("pref-changed", handlePrefChange); > - window.addEventListener("unload", function () { > + window.addEventListener("unload", function (event) { > + if (event.target.location.href != "about:blank") { When can you observe this as a problem? Can you check just `window.location.href` instead? Can it be fixed by making the listener capturing instead? Also: not sure if this will help at all to leaks, but there's been a long-standing thing where we should get rid of gDevTools listener here and instead set up a pref observer with Services.prefs.addObserver("devtools.theme", handlePrefChange) and Services.prefs.removeObserver
Attachment #8790358 -
Flags: review?(bgrinstead)
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8790359 [details] Bug 1302148 - Fix potential race in toolbox.destroy. https://reviewboard.mozilla.org/r/78236/#review76778
Attachment #8790359 -
Flags: review+
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #10) > Comment on attachment 8790358 [details] > Bug 1302148 - Fix various code leaking devtools toolbox. > > https://reviewboard.mozilla.org/r/78234/#review76774 > > ::: devtools/client/shared/developer-toolbar.js:160 > (Diff revision 2) > > > > command.state.onChange(target, onChange); > > onChange("", { target: target }); > > - document.defaultView.addEventListener("unload", () => { > > - if (command.state.offChange) { > > - command.state.offChange(target, onChange); > > + }; > > + let listener = function (event) { > > + if (event.target != document) { > > Why would event.target not be document? I don't know much about gcli but we receive a unload event from an iframe, this should be the typical code to filter events coming from iframes. Should I put a comment about iframes? > > ::: devtools/client/shared/theme-switching.js:179 > (Diff revision 2) > > } else { > > switchTheme(Services.prefs.getCharPref("devtools.theme")); > > > > gDevTools.on("pref-changed", handlePrefChange); > > - window.addEventListener("unload", function () { > > + window.addEventListener("unload", function (event) { > > + if (event.target.location.href != "about:blank") { > > When can you observe this as a problem? When I apply bug 1266134 and run devtools/client/webconsole/test/browser_webconsole_bug_595350_multiple_windows_and_tabs.js for example. But I keep hitting this toolbox document leak over the years... I still can't say why it only happens for some patches and not always. I'm using some local scripts to parse the whole gc and detect all cross compartments references (xrays). [this is in fact a revive of my demo from sunnyvale workweek if you remember] We clearly keep xrays between some module sandboxes (which are alive for the whole firefox instance) and the toolbox and tool documents. In this patch, I'm getting rid of all xrays between modules and toolbox.xul. That allows me to apply my host refactoring with leaks and hopefully will save us from these leak reports, or at least some of them... fyi, I applied this script to bug 1260552 in order to see that the webconsole was still around and leaking the webconsole document. > Can you check just > `window.location.href` instead? Yes, but may be I should also check for iframes here, like gcli patch. It is generally safer to check for event.target when listening for load/unload events. > Can it be fixed by making the listener capturing instead? I already switch to capture, otherwise we miss some unload events. (I haven't try to figure out why) But then, I also have to ignore the famous about:blank unload... > Also: not sure if this will help at all to leaks, but there's been a > long-standing thing where we should get rid of gDevTools listener here and > instead set up a pref observer with > Services.prefs.addObserver("devtools.theme", handlePrefChange) and > Services.prefs.removeObserver Yes it won't change. For the leak it is all about properly calling remove/off. I can change to pref observer if that help anything...
Comment 13•8 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #12) > > Can you check just > > `window.location.href` instead? > > Yes, but may be I should also check for iframes here, like gcli patch. It is > generally safer to check for event.target when listening for load/unload > events. > > > Can it be fixed by making the listener capturing instead? > > I already switch to capture, otherwise we miss some unload events. (I > haven't try to figure out why) > But then, I also have to ignore the famous about:blank unload... What's the deal with the about:blank unload? > > Also: not sure if this will help at all to leaks, but there's been a > > long-standing thing where we should get rid of gDevTools listener here and > > instead set up a pref observer with > > Services.prefs.addObserver("devtools.theme", handlePrefChange) and > > Services.prefs.removeObserver > > Yes it won't change. For the leak it is all about properly calling > remove/off. > I can change to pref observer if that help anything... Yes, that would help if you don't mind making the change. It will then actually catch changes to pref when they happen outside of toolbox options panel (like in about:config).
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #13) > > I already switch to capture, otherwise we miss some unload events. (I > > haven't try to figure out why) > > But then, I also have to ignore the famous about:blank unload... > > What's the deal with the about:blank unload? Oh I mixed up load and unload. about:blank is famous for the load event, not really on unload. We do receive unload events from iframes having about:blank as document... no idea what are those, may be some gcli internals? At the end that's the same than gcli, we receive unload event from tool iframes. A better fix is to do: if (event.target == document) to only process toolbox.xul unload and ignore the rest. > > > > Also: not sure if this will help at all to leaks, but there's been a > > > long-standing thing where we should get rid of gDevTools listener here and > > > instead set up a pref observer with > > > Services.prefs.addObserver("devtools.theme", handlePrefChange) and > > > Services.prefs.removeObserver > > > > Yes it won't change. For the leak it is all about properly calling > > remove/off. > > I can change to pref observer if that help anything... > > Yes, that would help if you don't mind making the change. It will then > actually catch changes to pref when they happen outside of toolbox options > panel (like in about:config). Ok, I'll in the next patch.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
I reverted back to use bubbling phase. My patch in bug 1266134 was messing up with unload event of the toolbox document... That's why some xrays were kept alive. When we listen for unload on the bubbling phase, we don't receive unload events for iframes, so it is simplier and it works. That was just my other patch which was prevent it from working in bubble but only in capture. And I tweaked theme-switching to listen directly to the pref as suggested.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•8 years ago
|
||
I pulled out theme-switching.js change apart as I had to tweak the related test and also modify the toolbox option panel to listen for pref change.
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8791195 [details] Bug 1302148 - Update devtools theme on devtools.theme pref change. https://reviewboard.mozilla.org/r/78696/#review77658 ::: devtools/client/framework/toolbox-options.js:259 (Diff revision 1) > this.panelWin.focus(); > }, > > setupThemeList: function () { > let themeBox = this.panelDoc.getElementById("devtools-theme-box"); > + themeBox.innerHTML = ""; Why is this needed / why was it *not* needed before? ::: devtools/client/shared/theme-switching.js:92 (Diff revision 1) > - function switchTheme(newTheme, oldTheme) { > - if (newTheme === oldTheme) { > + function switchTheme(newTheme) { > + if (newTheme === gOldTheme) { > return; > } > + let oldTheme = gOldTheme; > + gOldTheme = newTheme; Hm, I guess gDevTools pref-changed did do some convenient things. Still not worth keeping it around - if nothing else we should later make a new helper module that exposes a nice interface but doesn't rely on gDevTools or the options panel. We actually have the start of one, oddly in the styleeditor folder (utils.js) ::: devtools/client/shared/theme-switching.js:163 (Diff revision 1) > notifyWindow(); > }, console.error.bind(console)); > } > > - function handlePrefChange(event, data) { > - if (data.pref == "devtools.theme") { > + function handlePrefChange(subject, topic, prefName) { > + if (prefName == "devtools.theme") { We are only observing a single pref for now and we could be more generic later if needed. This should work just the same: function handleThemeChange() { switchTheme(Services.prefs.getCharPref("devtools.theme")); } Services.prefs.addObserver("devtools.theme", handleThemeChange, false) ::: devtools/client/shared/theme-switching.js:182 (Diff revision 1) > switchTheme(documentElement.getAttribute("force-theme")); > } else { > switchTheme(Services.prefs.getCharPref("devtools.theme")); > > - gDevTools.on("pref-changed", handlePrefChange); > - window.addEventListener("unload", function () { > + Services.prefs.addObserver("devtools.theme", handlePrefChange, false); > + window.addEventListener("unload", function onUnload(event) { You can use the 'once' option with addEventListener to avoid needing to name and remove it
Attachment #8791195 -
Flags: review?(bgrinstead)
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8790357 [details] Bug 1302148 - Nullify attributes on destroy to prevent leaking the toolbox. https://reviewboard.mozilla.org/r/78232/#review77666 ::: devtools/client/webconsole/panel.js:112 (Diff revision 1) > } > > this._destroyer = this.hud.destroy(); > this._destroyer.then(() => this.emit("destroyed")); > + this._destroyer.then(() => { > + this._frameWindow = null; Can't these be tacked onto the previous .then call (before "destroyed" is emitted)?
Attachment #8790357 -
Flags: review?(bgrinstead) → review+
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8790358 [details] Bug 1302148 - Fix various code leaking devtools toolbox. https://reviewboard.mozilla.org/r/78234/#review77668 ::: devtools/client/shared/developer-toolbar.js:167 (Diff revision 4) > - command.state.offChange(target, onChange); > + command.state.offChange(target, onChange); > - } > + } > - }, false); > - } > + button.remove(); > + button = null; > + }; > + document.defaultView.addEventListener("unload", listener); This can use the 'once' option with addEventListener instead of storing the function as a named variable and using removeEventListener
Attachment #8790358 -
Flags: review?(bgrinstead) → review+
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #22) > Comment on attachment 8791195 [details] > Bug 1302148 - Update devtools theme on devtools.theme pref change. > > https://reviewboard.mozilla.org/r/78696/#review77658 > > ::: devtools/client/framework/toolbox-options.js:259 > (Diff revision 1) > > this.panelWin.focus(); > > }, > > > > setupThemeList: function () { > > let themeBox = this.panelDoc.getElementById("devtools-theme-box"); > > + themeBox.innerHTML = ""; > > Why is this needed / why was it *not* needed before? That was broken, it isn't related to my patch. See devtools/client/framework/test/browser_toolbox_theme_registration.js. You will see duplicated themes while running this theme. > ::: devtools/client/webconsole/panel.js:112 > (Diff revision 1) > > } > > > > this._destroyer = this.hud.destroy(); > > this._destroyer.then(() => this.emit("destroyed")); > > + this._destroyer.then(() => { > > + this._frameWindow = null; > > Can't these be tacked onto the previous .then call (before "destroyed" is > emitted)? I'll give that a shot, I'm wondering if nullifying early can break some tests... I've addressed the rest.
Assignee | ||
Updated•8 years ago
|
Attachment #8790357 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8790358 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8791195 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8790359 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•8 years ago
|
||
Oops, I rebased so I discarded the previous mozreview to prevent weird interdiff, but it looks like it re-request review... May be it is better to just have weird interdiff.
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8792510 [details] Bug 1302148 - Nullify attributes on destroy to prevent leaking the toolbox. https://reviewboard.mozilla.org/r/79522/#review78156
Attachment #8792510 -
Flags: review?(bgrinstead) → review+
Comment 32•8 years ago
|
||
mozreview-review |
Comment on attachment 8792511 [details] Bug 1302148 - Fix various code leaking devtools toolbox. https://reviewboard.mozilla.org/r/79524/#review78158
Attachment #8792511 -
Flags: review?(bgrinstead) → review+
Comment 33•8 years ago
|
||
mozreview-review |
Comment on attachment 8792512 [details] Bug 1302148 - Update devtools theme on devtools.theme pref change. https://reviewboard.mozilla.org/r/79526/#review78162 ::: devtools/client/shared/theme-switching.js:162 (Diff revision 1) > gDevTools.emit("theme-switched", window, newTheme, oldTheme); > notifyWindow(); > }, console.error.bind(console)); > } > > - function handlePrefChange(event, data) { > + function handlePrefChange(subject, topic, prefName) { Nit: unnecessary args ::: devtools/client/shared/theme-switching.js:179 (Diff revision 1) > switchTheme(documentElement.getAttribute("force-theme")); > } else { > switchTheme(Services.prefs.getCharPref("devtools.theme")); > > - gDevTools.on("pref-changed", handlePrefChange); > - window.addEventListener("unload", function () { > + Services.prefs.addObserver("devtools.theme", handlePrefChange, false); > + window.addEventListener("unload", function (event) { Nit: unnecessary 'event' arg
Attachment #8792512 -
Flags: review?(bgrinstead) → review+
Comment 34•8 years ago
|
||
mozreview-review |
Comment on attachment 8792513 [details] Bug 1302148 - Fix potential race in toolbox.destroy. https://reviewboard.mozilla.org/r/79528/#review78164
Attachment #8792513 -
Flags: review?(bgrinstead) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•8 years ago
|
||
mozreview-review |
Comment on attachment 8792512 [details] Bug 1302148 - Update devtools theme on devtools.theme pref change. https://reviewboard.mozilla.org/r/79526/#review78174
Comment 38•8 years ago
|
||
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cbe6f8892338 Nullify attributes on destroy to prevent leaking the toolbox. r=bgrins https://hg.mozilla.org/integration/autoland/rev/d772aeb63c38 Fix various code leaking devtools toolbox. r=bgrins https://hg.mozilla.org/integration/autoland/rev/d0b6ed087223 Update devtools theme on devtools.theme pref change. r=bgrins https://hg.mozilla.org/integration/autoland/rev/1fdbce2821b5 Fix potential race in toolbox.destroy. r=bgrins
Comment 39•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cbe6f8892338 https://hg.mozilla.org/mozilla-central/rev/d772aeb63c38 https://hg.mozilla.org/mozilla-central/rev/d0b6ed087223 https://hg.mozilla.org/mozilla-central/rev/1fdbce2821b5
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•