528440 - Should skip windows that are closed but not yet destroyed when using nsIWindowMediator

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Dão Gottwald [:dao]]

nsIWindowMediator tracks windows until they are destroyed, but there's a brief period when windows are closed without being destroyed yet. Session restore should only care for windows that are open and should thus skip windows that aren't.

I don't think this is relevant for real users, but it likely is relevant for tests and could in fact be the cause for some intermittent failures we're seeing.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

passed sessionstore browser chrome tests locally and everything on the tryserver

Comment 2

[Simon Bünzli]

Comment on attachment 412187 [details] [diff] [review]
patch

>+    var window = windowMediator.getMostRecentWindow("navigator:browser");
>+    return window.closed ? null : window;

This will fail when getMostRecentWindow returns |null| - which it does when there are no open "navigator:browser" windows at all.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch v2

true!

Comment 4

[Marco Bonardo [:mak]]

hum, so if the most recent window is closing it will return null?
should it not return _next_ most recent window?

Comment 5

[Marco Bonardo [:mak]]

i was thinking something like this:

+    var win = windowMediator.getMostRecentWindow("navigator:browser");
+    if (win && win.closed) {
+      // Use our own _openBrowserWindows cache.
+      for (var i = 0; i < this._openBrowserWindows.length; i++) {
+        win = this._openBrowserWindows[i];
+        if (!win.closed)
+          break;
+      }
+
+    return win && win.closed ? null : win;

where _openBrowserWindows is a local cache filled up using domwindowopened and domwindowclosed (since there is no way to read the window timestamp)

Comment 6

[Marco Bonardo [:mak]]

well, my return condition is wrong, but is just the idea.

Comment 7

[Marco Bonardo [:mak]]

btw, actually it is not wrong

Comment 8

[Marco Bonardo [:mak]]

could even make sense add an optional aOnlyOpenWindows param to getEnumerator and getMostRecentWindow... the interface is not frozen and being optional should not create big headache.

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch v3

This tries to find the most recent open window. I've pushed this to the tryserver, tests passed for both ifdef branches locally.

Comment 10

[Simon Bünzli]

Comment on attachment 412205 [details] [diff] [review]
patch v3

Wouldn't it make more sense to fix nsIWindowMediator::getMostRecentWindow instead? What you're doing here is how I'd have expected it to behave in the first place.

Comment 11

[Dão Gottwald [:dao]]

I'm not sure if returning closed-but-not-yet-destroyed windows is useful in other contexts. I guess I'll file a bug.

Comment 12

[Marco Bonardo [:mak]]

it is useful in the enumerator, exactly to wait for windows to be closed, for example in the browser-test.js case.
that's why i suggest an optional param.

Comment 13

[(no longer active)]

This bug can affect the fix of bug 526194, which is a 3.6 blocker, so I think this should also block the release of Firefox 3.6.

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #12)
> it is useful in the enumerator, exactly to wait for windows to be closed, for
> example in the browser-test.js case.
> that's why i suggest an optional param.

You're not using getMostRecentWindow in that other bug.

(In reply to comment #13)
> This bug can affect the fix of bug 526194, which is a 3.6 blocker, so I think
> this should also block the release of Firefox 3.6.

I still think this is only relevant for tests...

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 412205 [details] [diff] [review]
patch v3

>   _getMostRecentBrowserWindow: function sss_getMostRecentBrowserWindow() {
>-    var windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"].
>-                         getService(Ci.nsIWindowMediator);
>-    return windowMediator.getMostRecentWindow("navigator:browser");
>+    var wm = Cc["@mozilla.org/appshell/window-mediator;1"].
>+             getService(Ci.nsIWindowMediator);
>+
>+    var win = wm.getMostRecentWindow("navigator:browser");
>+    if (win && !win.closed)
>+      return win;

I've changed the last two lines to this:

>+    if (!win)
>+      return null;
>+    if (!win.closed)
>+      return win;

No need to call getEnumerator / getZOrderDOMWindowEnumerator if getMostRecentWindow returns null already.

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e39e777f86bb

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #10)
> (From update of attachment 412205 [details] [diff] [review])
> Wouldn't it make more sense to fix nsIWindowMediator::getMostRecentWindow
> instead? What you're doing here is how I'd have expected it to behave in the
> first place.

Filed bug 528706.

Comment 18

[(no longer active)]

Comment on attachment 412205 [details] [diff] [review]
patch v3

This doesn't require approval now that it's a blocker.

Comment 19

[(no longer active)]

Landed on 1.9.2: <http://hg.mozilla.org/releases/mozilla-1.9.2/rev/151cf89d430c>