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

RESOLVED FIXED in Firefox 3.7a1

Status

()

defect
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on 1 bug, Blocks 1 bug, {intermittent-failure})

Trunk
Firefox 3.7a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.6 +

Firefox Tracking Flags

(status1.9.2 beta4-fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Whiteboard: [orange]
Posted patch patch (obsolete) — Splinter Review
passed sessionstore browser chrome tests locally and everything on the tryserver
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #412187 - Flags: review?(zeniko)
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.
Attachment #412187 - Flags: review?(zeniko)
Posted patch patch v2 (obsolete) — Splinter Review
true!
Attachment #412187 - Attachment is obsolete: true
Attachment #412188 - Flags: review?(zeniko)
hum, so if the most recent window is closing it will return null?
should it not return _next_ most recent window?
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)
well, my return condition is wrong, but is just the idea.
btw, actually it is not wrong
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.
Posted patch patch v3Splinter Review
This tries to find the most recent open window. I've pushed this to the tryserver, tests passed for both ifdef branches locally.
Attachment #412188 - Attachment is obsolete: true
Attachment #412205 - Flags: review?(zeniko)
Attachment #412188 - Flags: review?(zeniko)
Depends on: 462222
No longer blocks: 527074
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.
Attachment #412205 - Flags: review?(zeniko) → review+
I'm not sure if returning closed-but-not-yet-destroyed windows is useful in other contexts. I guess I'll file a bug.
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.
Blocks: 526194
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.
Flags: blocking-firefox3.6?
(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 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.
http://hg.mozilla.org/mozilla-central/rev/e39e777f86bb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Attachment #412205 - Flags: approval1.9.2?
No longer blocks: 521802
No longer blocks: 486683
Depends on: 528706
(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.
No longer blocks: 528219
No longer blocks: 521889
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Comment on attachment 412205 [details] [diff] [review]
patch v3

This doesn't require approval now that it's a blocker.
Attachment #412205 - Flags: approval1.9.2?
Keywords: checkin-needed
Whiteboard: [orange] → [orange][needs 1.9.2 landing]
Landed on 1.9.2: <http://hg.mozilla.org/releases/mozilla-1.9.2/rev/151cf89d430c>
Keywords: checkin-needed
Whiteboard: [orange][needs 1.9.2 landing] → [orange]
Blocks: 529875
No longer blocks: 509966
Blocks: 558636
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.