574127 - dead/broken code in HUD_SERVICE.shutdown

Status: RESOLVED DUPLICATE of bug 595350

(DevTools :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/annotate/e910fd948e5b/toolkit/components/console/hudservice/HUDService.jsm#l532

"aContentWindow" is undefined.

Comment 1

[David Dahl :ddahl]

sorry gavin, did not see this as my bz query was lazer focused on bugs blocking 529086. i better change that.

Comment 2

[David Dahl :ddahl]

Attachment: patch v1

Not sure if all of this is needed. For instance if there is no main xul window open, the HUDService object is probably GC'd already, no?

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 458454 [details] [diff] [review]
patch v1

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>@@ -496,32 +496,59 @@ HUD_SERVICE.prototype =
> 
>   /**
>    * Shutdown all HeadsUpDisplays on xpcom-shutdown
>    *
>    * @returns void
>    */
>   shutdown: function HS_shutdown()
>   {
>-    for (var displayId in this._headsUpDisplays) {
>-      this.unregisterDisplay(displayId);
>-    }
>-    // delete the storage as it holds onto channels
>-    delete this.storage;
>-
>-     var xulWindow = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+    var xulWindow;
>+    var contentWindow;

Just declare these at first use, JS vars are function scoped anyway.

>+    // We need to get a contentWindow, any contentWindow to get a XUL window reference
>+    for (var idx in this.hudWeakReferences) {
>+      // you are never sure if your weakRef will exist
>+      try {
>+        contentWindow = this.hudWeakReferences[idx].get().contentWindow;
>+      }
>+      catch (ex) {
>+        // noop: .get() will throw if the weakRef is gone
>+      }

get() does not throw if the reference is gone, it will merely return null. Test that and get rid of the try catch block.

>+
>+      if (contentWindow) {
>+        break;
>+      }
>+    }
>+
>+    if (contentWindow) {
>+      // only run this xul window routine if there are any actual tabs open
>+      // this will alos fail if the contentWindow goes away inside of this
>+      // method.
>+      try {
>+        xulWindow = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>        .getInterface(Ci.nsIWebNavigation)
>        .QueryInterface(Ci.nsIDocShellTreeItem)
>        .rootTreeItem
>        .QueryInterface(Ci.nsIInterfaceRequestor)
>        .getInterface(Ci.nsIDOMWindow);

The indentation is messed up here, did you diff with -w?

This is all happening at xpcom-shutdown right? No windows should exist at that point so this all seems to be over the top. If they do then you have to account for the fact that there may be multiple windows existing, this code seems to only handle the first one it comes across.

Comment 5

[David Dahl :ddahl]

Attachment: v 2 shutdown-patch

seems to fix any hang-around leaks...

Comment 6

[David Dahl :ddahl]

Attachment: v 2 shutdown-patch

Comment 7

[Patrick Walton (:pcwalton)]

Comment on attachment 474227 [details] [diff] [review]
v 2 shutdown-patch

LGTM.

Comment 8

[Patrick Walton (:pcwalton)]

Should this have a test?

Comment 9

[David Dahl :ddahl]

Attachment: v 3 shutdown patch

added unregister.bind(this) to provide the correct scope

Comment 10

[David Dahl :ddahl]

(In reply to comment #8)
> Should this have a test?

You cannot test stuff on shutdown:(

Comment 11

[David Dahl :ddahl]

The weird thing is that the leak disappears with this patch on Linux but is still there on windows, but seems less severe.

Comment 12

[Patrick Walton (:pcwalton)]

Comment on attachment 474238 [details] [diff] [review]
v 3 shutdown patch

This doesn't fix the leak. The problem is that the console isn't unregistered when new windows are closed with the console still open. The quit event isn't what you want: instead, you want the window close event (which is fired on application quit as well as on manual window close).

Comment 13

[David Dahl :ddahl]

(In reply to comment #12)
> you want the window close event (which is fired on
> application quit as well as on manual window close).

Ok, HUDService.onTabClose handles tab closure, which is called on each tab as Firefox shuts down. I guess we do not handle the situation where a console is opened and closed but the tab remains open. That must be the (one of) leak(s).

Comment 14

[Dietrich Ayala (:dietrich)]

Why do we need to hold the release for this? Please explain why blocking is needed when making the request.

Comment 15

[David Dahl :ddahl]

(In reply to comment #14)
> Why do we need to hold the release for this? Please explain why blocking is
> needed when making the request.

I should just ask for approval once I have r+. Just want to land it asap.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> Ok, HUDService.onTabClose handles tab closure, which is called on each tab as
> Firefox shuts down. I guess we do not handle the situation where a console is
> opened and closed but the tab remains open. That must be the (one of) leak(s).
Does that mean that this isn't ready?

Comment 17

[David Dahl :ddahl]

(In reply to comment #16)
> Does that mean that this isn't ready?

no. It means I need to file another bug for handling the reference eradication on closed consoles. I thought we handled everything, but of course, this code has changed a lot since I wrote the console close routine.

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 474238 [details] [diff] [review]
v 3 shutdown patch

>   /**
>-   * Shutdown all HeadsUpDisplays on xpcom-shutdown
>+   * Shutdown all HeadsUpDisplays on quit-application-granted
nit: punctuation please :)

>+    // remove all event listeners that we created
ditto

>+    let windowEnum = Services.wm.getEnumerator("navigator:browser");
>+    while (windowEnum.hasMoreElements()) {
>+      let window = windowEnum.getNext();
>+      let gBrowser = window.gBrowser;
>+      gBrowser.tabContainer.removeEventListener("TabClose",
>+						this.onTabClose, false);
TABS!

>+    Services.obs.addObserver(this, "quit-application-granted", false);
You use/reference the quit-application-granted topic in several places.  constifiy it globally, and use the constant please.

r=sdwilsh with those changes

Comment 19

[David Dahl :ddahl]

(In reply to comment #18)
> >+    let windowEnum = Services.wm.getEnumerator("navigator:browser");
> >+    while (windowEnum.hasMoreElements()) {
> >+      let window = windowEnum.getNext();
> >+      let gBrowser = window.gBrowser;
> >+      gBrowser.tabContainer.removeEventListener("TabClose",
> >+						this.onTabClose, false);
> TABS!

How embarassing. I think that is due to the emacs on my windoze box, which needs some .emacs tweaks.

Comment 20

[Patrick Walton (:pcwalton)]

(In reply to comment #17)
> (In reply to comment #16)
> > Does that mean that this isn't ready?
> 
> no. It means I need to file another bug for handling the reference eradication
> on closed consoles. I thought we handled everything, but of course, this code
> has changed a lot since I wrote the console close routine.

The patches in bug 595350 should handle this.

Comment 21

[Patrick Walton (:pcwalton)]

(In reply to comment #13)
> (In reply to comment #12)
> > you want the window close event (which is fired on
> > application quit as well as on manual window close).
> 
> Ok, HUDService.onTabClose handles tab closure, which is called on each tab as
> Firefox shuts down.

Just tested and unfortunately this doesn't seem to be the case. You have to listen for the window close event explicitly.

Comment 22

[Rob Campbell [:rc] (:robcee)]

this has an r+'d patch sitting here and blocks 606055. Is that patch still valid?

Comment 23

[David Dahl :ddahl]

Pretty sure this bug should now be a dupe of bug 595350 http://hg.mozilla.org/mozilla-central/rev/b2cb6f8e4840