Closed
Bug 528440
Opened 15 years ago
Closed 15 years ago
Should skip windows that are closed but not yet destroyed when using nsIWindowMediator
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 3.7a1
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta4-fixed |
People
(Reporter: dao, Assigned: dao)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: intermittent-failure)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
zeniko
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•15 years ago
|
Whiteboard: [orange]
Assignee | ||
Comment 1•15 years ago
|
||
passed sessionstore browser chrome tests locally and everything on the tryserver
Comment 2•15 years ago
|
||
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)
Assignee | ||
Comment 3•15 years ago
|
||
true!
Attachment #412187 -
Attachment is obsolete: true
Attachment #412188 -
Flags: review?(zeniko)
Comment 4•15 years ago
|
||
hum, so if the most recent window is closing it will return null? should it not return _next_ most recent window?
Comment 5•15 years ago
|
||
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•15 years ago
|
||
well, my return condition is wrong, but is just the idea.
Comment 7•15 years ago
|
||
btw, actually it is not wrong
Comment 8•15 years ago
|
||
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.
Assignee | ||
Comment 9•15 years ago
|
||
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)
Comment 10•15 years ago
|
||
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+
Assignee | ||
Comment 11•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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?
Assignee | ||
Comment 14•15 years ago
|
||
(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...
Assignee | ||
Comment 15•15 years ago
|
||
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.
Assignee | ||
Comment 16•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e39e777f86bb
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Assignee | ||
Updated•15 years ago
|
Attachment #412205 -
Flags: approval1.9.2?
Assignee | ||
Comment 17•15 years ago
|
||
(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.
Updated•15 years ago
|
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Comment 18•15 years ago
|
||
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?
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [orange] → [orange][needs 1.9.2 landing]
Comment 19•15 years ago
|
||
Landed on 1.9.2: <http://hg.mozilla.org/releases/mozilla-1.9.2/rev/151cf89d430c>
status1.9.2:
--- → final-fixed
Keywords: checkin-needed
Whiteboard: [orange][needs 1.9.2 landing] → [orange]
Updated•12 years ago
|
Keywords: intermittent-failure
Updated•12 years ago
|
Whiteboard: [orange]
You need to log in
before you can comment on or make changes to this bug.
Description
•