343871 - session restore after nightly update opens a bunch of blank windows

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Mike Beltzner [:beltzner, not reading bugmail]]

After my nightly update (from 20060706 -> 20060707) when Bon Echo restarted, in addition to the windows I expected to have restored, about 20 about:blank windows were opened. Not exactly sure why, but in that previous session I had probably opened (and then closed) about 20 new windows (not tabs) so I'm wondering if it was remembering those operations and trying to reopen them?

Anyone seen this before?

Comment 1

[Phil Ringnalda (:philor)]

Once, after a crash restore, but they weren't blank: I got two extra windows that had the content they had when I had closed them days earlier. Still haven't found any way to reproduce (probably because I rarely open windows).

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

(clearing blocking nomination)

Mano, let's wait until we get more confirmations and a STR before asking for blocking here.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

I've had this happen to me twice, now, and Shaver just had this happen to him, too, which makes me think that I'm a little less nuts. We're both using Intel Macs, and in all cases where this has been seen it's after we do nightly updates.

Marking qawanted.

Comment 4

[Simon Bünzli]

If this happens after a crash, could you please grab a copy of sessionstore.bak for inspection? And in case this doesn't happen only after crashes, it'd be nice if you could start Firefox through a script which automatically makes a copy of sessionstore.js. Having a peak into a file which triggers your case might help to identify the issue at hand....

Comment 5

[Nickolay_Ponomarev]

Happened to me on Windows branch nightly, after an update. Didn't manage to copy the sessionstore file. I had gmail open.

Is there a reason session store doesn't backup the file itself? (Perf?) Keeping a backup would be useful for debugging things like this and also for other reasons (such as the reason mentioned in bug 345135).

Comment 6

[Simon Bünzli]

(In reply to comment #5)
> Is there a reason session store doesn't backup the file itself?

Privacy. - If you want to change that, please file a new bug.

Comment 7

[Adam Guthrie]

Attachment: sessionstore.js from the session that opened up all the blank windows

This just happened to me. Attaching sessionstore.js... For what's it's worth, I had five blank windows open.

Comment 8

[Simon Bünzli]

Attachment: possible fix

The file attached by Adam (thanks!) contains entries for several apparently tab-less windows. I'm not sure where they come from, but hope to catch the issue by making sure that (1) we're not tracking any windows after quit-application-granted (after which we don't further update window states) and (2) we're not opening any such tab-less windows at startup (which would be pretty pointless anyway). At least the fix for (1) must go in anyway - and the fix for (2) won't hurt...

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 233319 [details] [diff] [review]
possible fix

(In reply to comment #8)
> Created an attachment (id=233319) [edit]
> possible fix
> 
> The file attached by Adam (thanks!) contains entries for several apparently
> tab-less windows. I'm not sure where they come from, but hope to catch the
> issue by making sure that (1) we're not tracking any windows after
> quit-application-granted (after which we don't further update window states)
> and (2) we're not opening any such tab-less windows at startup (which would be
> pretty pointless anyway). At least the fix for (1) must go in anyway - and the
> fix for (2) won't hurt...
> 

Thanks Simon. Good catch on (1), that has the potential for causing problems. And the fix for (2) should work as a preventative remedy for the blank windows.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

This was checked in on the trunk, so FIXED.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Should this be blocking-firefox2+?  I keep hitting it, so _I_ certainly think so. (Sessionstore.js available upon request; there was some force-quitting involved, sadly.)

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

--> blocking Firefox2, not beta since the frequency (shaver's an anomaly here) is still relatively low.

Comment 13

[Simon Bünzli]

Comment on attachment 233319 [details] [diff] [review]
possible fix

Drivers: This is a low-risk patch which prevents SessionStore from registering windows it won't ever track (which could otherwise lead to additional blank windows). Furthermore this patch also prevents data-less blank windows to be restored (for the improbable case that shaver sees a different issue).

Comment 14

[Mike Schroepfer]

Comment on attachment 233319 [details] [diff] [review]
possible fix

a=schrep for drivers - approving all [181approval pending] bugs now that tree is open.

Comment 15

[Gennady Feldman]

OK, with todays nightly ( Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b2) Gecko/20060824 BonEcho/2.0b2 ) after updating (from Aug 22 nightly) and having 2 windows open during update after Firefox restarted only the first window restored the tabs. The second window had the right number of tabs, but they came up blank.

Re-open? Regression?

Comment 16

[Simon Bünzli]

If the number of tabs was correct, you observe a different bug. Please try to reproduce the faulty behavior, observe the Error Console (having set javascript.options.showInConsole to true) and - if you can reproduce the issue reliably - file a new bug and attach your sessionstore.js for further analysis.

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #15)
> OK, with todays nightly ( Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
> rv:1.8.1b2) Gecko/20060824 BonEcho/2.0b2 ) after updating (from Aug 22 nightly)
> and having 2 windows open during update after Firefox restarted only the first
> window restored the tabs. The second window had the right number of tabs, but
> they came up blank.
> 
> Re-open? Regression?
> 

I'm able to reproduce this on branch, but not on trunk, which is odd as this patch is already on trunk. Reverting this patch on the branch fixes the problem. I'm reopening the bug, and am reverting the patch for now, because it's a major regression.

Comment 18

[Simon Bünzli]

I don't see how this patch could trigger the observed bug. AFAICT it's rather the patch to bug 347336 which triggers the bug (which makes it - as I said - a different bug).

The issue is that for the all but the first restored windows, restoreWindow is called before onLoad - because the "load" listener attached in _openWindowWithState seems to be called before the "domwindowopened" - but that one is required for assigning the window an id: __SSi.

The solution would thus be to ensure that onLoad is called before restoreWindow (or to call it from restoreWindow if aWindow.__SSi is undefined when it's required). But please do that in a different bug.

Comment 19

[Dietrich Ayala (:dietrich)]

(In reply to comment #18)
> I don't see how this patch could trigger the observed bug. AFAICT it's rather
> the patch to bug 347336 which triggers the bug (which makes it - as I said - a
> different bug).

ok, i applied and removed the 2 patches separately, and you are correct. it is bug 347336 which exposed the problem. when i tested reverting this patch, i must have accidentally reverted both patches, causing it appear as if this one caused the problem :(

i'll check this back in, and then fix the regression in the other bug.

Comment 20

[Dietrich Ayala (:dietrich)]

Checked back in on branch. Addressing comment #15 in bug 347336.