If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Only add last closed window to open windows at shutdown

RESOLVED FIXED in Firefox 4.0b7

Status

()

Firefox
Session Restore
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: zpao, Assigned: zpao)

Tracking

Trunk
Firefox 4.0b7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
How can we detect whether _getCurrentState is called during shutdown?
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.

Updated

7 years ago
Blocks: 595236
Created attachment 476464 [details] [diff] [review]
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.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #476464 - Flags: review?(dolske)
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.
Attachment #476464 - Flags: review?(dietrich)
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? :)
Attachment #476464 - Flags: review?(dolske)
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 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.
Attachment #476464 - Flags: review?(dietrich) → review+

Comment 8

7 years ago
Sent to try with 595236

Comment 9

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

Comment 10

7 years ago
Rather, here's the proper build log: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1285017295.1285018434.11888.gz&fulltext=1
Created attachment 477282 [details] [diff] [review]
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)
Attachment #476464 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/03a1f45d2099
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
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!)
You need to log in before you can comment on or make changes to this bug.