We have a bunch of places that enumerate windows via nsIWindowMediator. The list of windows it returns can include closed windows: that is windows for which .closed is true and which will be disconnected from window mediator when the event loop spins. Some consumers check for this, some do not. This is a bug about making some of the ones that don't check, but should, do so.

Justin, do you mind reviewing this, since Gavin is out, or suggesting another reviewer?

Comment on attachment 801154 [details] [diff] [review] Fix consumers of window mediator to be more consistent in their checking for closed windows. that we can't just stop returning closed windows from the window >--- a/browser/base/content/utilityOverlay.js >+++ b/browser/base/content/utilityOverlay.js >@@ -447,16 +447,19 @@ function isBidiEnabled() { > } catch (e) {} > > return rv; > } > > function openAboutDialog() { > var enumerator = Services.wm.getEnumerator("Browser:About"); > while (enumerator.hasMoreElements()) { >+ if (window.closed) { >+ continue; >+ } > // Only open one about window (Bug 599573) > let win = enumerator.getNext(); window != win >--- a/toolkit/components/social/MozSocialAPI.jsm >+++ b/toolkit/components/social/MozSocialAPI.jsm >@@ -297,17 +297,17 @@ function findChromeWindowForChats(prefer > enumerator = Services.wm.getEnumerator("navigator:browser"); > } else { > // here we explicitly ask for bottom-to-top so we can use the same logic > // where BROKEN_WM_Z_ORDER is true. > enumerator = Services.wm.getZOrderDOMWindowEnumerator("navigator:browser", false); > } > while (enumerator.hasMoreElements()) { > let win = enumerator.getNext(); >- if (win && isWindowGoodForChats(win)) >+ if (win && !win.closed && isWindowGoodForChats(win)) > topMost = win; > } I don't see the point of null-checking win... >--- a/xpfe/appshell/public/nsIWindowMediator.idl >+++ b/xpfe/appshell/public/nsIWindowMediator.idl >@@ -24,17 +24,19 @@ interface nsIWindowMediatorListener; > interface nsIWindowMediator: nsISupports > { > /** Return an enumerator which iterates over all windows of type aWindowType > * from the oldest window to the youngest. > * @param aWindowType the returned enumerator will enumerate only > * windows of this type. ("type" is the > * |windowtype| attribute of the XML <window> element.) > * If null, all windows will be enumerated. >- * @return an enumerator of nsIDOMWindows >+ * @return an enumerator of nsIDOMWindows. Note that windows close >+ * asynchronously in many cases, so windows returned from this >+ * enumerator can have .closed set to true. Caveat enumerator! > */ > nsISimpleEnumerator getEnumerator(in wstring aWindowType); Maybe we should just change the API to skip closed windows by default, if that's what consumers usually want, and add some other way to get closed windows if necessary?

> window != win Er, yes, good catch. > I don't see the point of null-checking win... I don't either; I can drop that part. > Maybe we should just change the API to skip closed windows by default, if that's what > consumers usually want What consumers want is .. unclear. I considered doing that, but went for the minimally invasive change.

Comment on attachment 801159 [details] [diff] [review] With Dão's comments addressed Review of attachment 801159 [details] [diff] [review]: ----------------------------------------------------------------- Was mulling over if nsIWindowMediator should either ignore closing windows or have an API that allows the caller to explicitly request them (perhaps a new API, since removing this one would probably have a big addon compat hit). But... meh. It can already had out closing windows today, and so having it be more async probably won't be a big change. Most of the call sites being fixed up here would only fail in a mildly annoying way, so it's hard to get worked up about it. ::: xpfe/appshell/public/nsIWindowMediator.idl @@ +30,5 @@ > * |windowtype| attribute of the XML <window> element.) > * If null, all windows will be enumerated. > + * @return an enumerator of nsIDOMWindows. Note that windows close > + * asynchronously in many cases, so windows returned from this > + * enumerator can have .closed set to true. Caveat enumerator! "Caveat enumerator". Nice. :)

Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/03e790568329d2952c2da539cb9858295a87797c Bug 913855. Fix consumers of window mediator to be more consistent in their checking for closed windows. r=dolske

Synced the comment change to the add-on SDK repo and also filed bug 923715 on it.