Closed Bug 1302148 Opened 8 years ago Closed 8 years ago

Toolbox document leaks in many possible ways

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

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: nobody → poirot.alex
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.
Tweaked the gcli fix to ignore unwanted unload events.
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)
(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 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 on attachment 8790359 [details]
Bug 1302148 - Fix potential race in toolbox.destroy.

https://reviewboard.mozilla.org/r/78236/#review76778
Attachment #8790359 - Flags: review+
(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...
(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).
(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.
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.
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 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 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 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+
(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.
Attachment #8790357 - Attachment is obsolete: true
Attachment #8790358 - Attachment is obsolete: true
Attachment #8791195 - Attachment is obsolete: true
Attachment #8790359 - Attachment is obsolete: true
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 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 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 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 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 on attachment 8792512 [details]
Bug 1302148 - Update devtools theme on devtools.theme pref change.

https://reviewboard.mozilla.org/r/79526/#review78174
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
Depends on: 1311789
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: