Closed
Bug 913855
Opened 11 years ago
Closed 11 years ago
Be more principled about checking for closed windows when enumerating windows via nsIWindowMediator
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 26
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 1 obsolete file)
11.95 KB,
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Justin, do you mind reviewing this, since Gavin is out, or suggesting another reviewer?
Attachment #801154 -
Flags: review?(dolske)
Comment 2•11 years ago
|
||
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?
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 3•11 years ago
|
||
> 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.
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #801159 -
Flags: review?(dolske)
Assignee | ||
Updated•11 years ago
|
Attachment #801154 -
Attachment is obsolete: true
Attachment #801154 -
Flags: review?(dolske)
Comment 5•11 years ago
|
||
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. :)
Attachment #801159 -
Flags: review?(dolske) → review+
Assignee | ||
Comment 6•11 years ago
|
||
Flags: in-testsuite+
Target Milestone: --- → Firefox 26
Blocks: 916324
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 8•11 years ago
|
||
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
Comment 9•11 years ago
|
||
Synced the comment change to the add-on SDK repo and also filed bug 923715 on it.
You need to log in
before you can comment on or make changes to this bug.
Description
•