597071 - Only add last closed window to open windows at shutdown

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

This came out of bug 596592, where entering PB mode with just a popup and then leaving causes the last opened window to reopen.

_getCurrentState is actually lying about the current state when there is a single window open and it's a popup (and there was a recently closed non-popup). It shifts a closed window into the open window list
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1953

This is fine for when quitting and we've been doing such for a long time. But I don't think it's the right thing to do when entering/exiting private browsing mode.

Comment 1

[(no longer active)]

How can we detect whether _getCurrentState is called during shutdown?

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

if (_this._loadState == STATE_QUITTING)
  // move last closed non-popup

I think we also want to check something about _restoreLastWindow (for the all browser windows are closed, but view-source was left open), since we'll still have STATE_RUNNING.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Fix it all up and it even includes a test (win/linux only).

I didn't have a windows machine handy, but Sean has run this and says it passes with the fix, and fails without.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 476464 [details] [diff] [review]
Patch v0.1

Dietrich / Dolske: whoever gets to it first wins! I know the timing is pretty crappy, but Panorama needs this to fix a b7 blocker (it really just makes test behavior for some private browsing tests accurate).

It is a bit changy - API consumers will get an accurate state as opposed to the fake state we've been providing.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 476464 [details] [diff] [review]
Patch v0.1

Alas, I think dietrich should do the review here, I don't know session restore well enough to judge the impact of this. Even though it's a 1-line change.


>+++ b/browser/components/sessionstore/test/browser/browser_597071.js
...
>+  // Test is Win/Linux only
>+  // if ("nsILocalFileMac" in Ci)
>+  //   return;

Really? :)

Comment 6

[Mats Palmgren (inactive)]

I pushed a workaround for this in bug 596592:
http://hg.mozilla.org/mozilla-central/rev/f1eec85dcfdc
It should be backed out when this bug is fixed.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 476464 [details] [diff] [review]
Patch v0.1

This makes sense, makes the API more consistent. To make the test only run for Win/Linux, move the check to the makefile so that it doesn't run at all. r=me w/ that change.

Comment 8

[Sean Dunn]

Sent to try with 595236

Comment 9

[Sean Dunn]

Try server failed this bug's test (timeout) on linux, see try build http://hg.mozilla.org/try/rev/617debdc2f5e

Comment 10

[Sean Dunn]

Rather, here's the proper build log: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1285017295.1285018434.11888.gz&fulltext=1

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v1.0 (for checkin)

Used #ifndef XP_MACOSX in the Makefile to target win/linux. Also moved the listener off the tabContainer to gBrowser, since that's what was most likely causing the timeout (tabContainer doesn't capture page loads, just favicons)

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/03a1f45d2099

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/7b85cdce3ea6 to followup on the bustage. I can't do what I had done in the Makefile (don't know why it worked locally!)