642624 - If shutdown Firefox when all closed windows are popups, exception occurs and session isn't saved.

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Michael Kraft [:morac]]

For a while now, I've noticed that sometimes when shutting down Firefox 4.0 (betas and RC), especially after having used it for a long period of time, that the sessionstore.js file still indicates the session is "running", meaning the final write never occurred.   I've seen this on both my work and home PC's (running XP SP3).   I've also had experiences when starting Firefox (I have it set to load windows and tabs from last time), that long closed windows open.  Normally I quit Firefox with a single blank tab open, which may be what's causing this problem.

I have logging enabled in my Session Manager add-on and I'm seeing the following exception occur on shutdown when this occurs.

Thu, 17 Mar 2011 22:51:36 GMT: {'[JavaScript Error: "total[0] is undefined" {file: "resource://gre/components/nsSessionStore.js" line: 2267}]' when calling method: [nsISessionStore::getBrowserState]}


I looked at the code and the only explanation I can come up with is if all the closed windows were popups and sure enough my sessionstore.js file indicated as such.  There was one closed window and it was a popup.

What happens then is that the following code will shift all the closed popup windows and then finally shift a NULL into total[0] which will throw an exception since total[0] will then be 0.

      do {
        total.unshift(lastClosedWindowsCopy.shift())
      } while (total[0].isPopup)


My guess is that this should probably be changed to:

      do {
        total.unshift(lastClosedWindowsCopy.shift())
      } while (total[0].isPopup && lastClosedWindowsCopy.length > 0)


Though since the code appears to be trying to find the last non-popup closed window, I'm not sure this is exactly correct since there are no non-popup closed windows.


This problem is fairly easy to reproduce, simply clear your session data, find a page that has a popup window and open it.  Then close the window.  Finally shut down Firefox and look at the sessionstore.js file and it will show as "running".

Comment 1

[Michael Kraft [:morac]]

Attachment: Prevent exception when there there are no open windows and all closed windows are popups

I put a patch to fix the exception, but I don't think the code is currently coded correctly in the first place.

The way the code is written for non-MAC OS X versions of Firefox, if there are no open non-pop up windows when the user shuts down the browser and the user has the browser set to restore windows and tabs from last time, then the code currently sets up sessionstore.js so all old closed popup windows will open the next time the user runs the browser.

Personally I find this highly annoying since I don't want old popups opening when I run Firefox.  That in itself seems like a bug.

Why exactly do we want to restore all closed window the next time Firefox is run, if the user shuts down with an empty window?


Anyway here's an updated steps to reproduce this problem:
1. Open browser and close all but one tab.
2. Set to restore windows and tabs from last time.
3. Go to http://www.yourhtmlsource.com/javascript/popupwindows.html in browser
4. Scroll down to the "Pop It" link and click it.
5. Close the popup link.
6. Close tab (so that all that's displaying is one "New Tab").
7. Closed Browser.

At this point without the patch, the sessionstore.js has the closed tab as being open and the closed window as being closed.  The state lists as "running".  This is because the browser failed to save when shutting down because of the exception.

With the patch, the sessionstore.js file has the closed popup window as the saved window.  The status lists as "stopped".  So when the browser is next run the closed popup window is opened.  I feel this is wrong from a user standpoint, but it's what the comments say should happen.  Personally I feel if the user shuts down with a blank browser, that's what should open the next time the browser is run.

I'm going to come up with a better patch that works the way users would expect it to work, but this patch fixes the exception which prevents Firefox from thinking it crashed when shutting down.

Comment 2

[Michael Kraft [:morac]]

Attachment: Better patch which doesn't restore closed popup windows

This is a better patch I came up with.  It handles the following cases the following way:

1. User closes browser with open windows (normal) - saves those windows as open and saves any closed windows as closed.
2. User closes browser with no open window (or only popup windows open) and at least one closed non-popup window - finds first non-popup window and uses that for the browser state; all other windows remain closed and stored in closed window list (including popup windows).
3. User closes browser with no open window (or only popup windows open) and no non-popup windows in closed window list - creates a blank session uses that for the browser state; all other windows remain closed and stored in closed window list (including popup windows).  This triggers loading of homepage next time browser runs, with all popup windows in the closed window list.

The only iffy case I found was when shutting down the browser with nothing, but a popup window open.  In that case the popup window was moved to the closed window list and a new browser window was opened at start up.  The problem was that the new window was opened with the size of the old popup window.  There's probably a better way of handling that, but the window can be resized so it's not terrible.

There's no way to test this automatically since it involves shutting down the browser and running it again, but the test cases are rather simple.

Comment 3

[Michael Kraft [:morac]]

I haven't seen any movement on this in 3 months.

Comment 4

[Michael Kraft [:morac]]

Still a problem in Firefox 6.

Note, in the steps to recreate, make sure the "browser.tabs.closeWindowWithLastTab" preference is set to false otherwise Firefox will close when you try to close the last tab.

Comment 5

[Michael Kraft [:morac]]

I'm still having daily issues with Firefox reporting "crashes" after I've shut down and ran Firefox again.  

Any reason why this bug report is being ignored?  It's a pretty simple bug as there's a reproducible test case, the reason for the problem is well documented and there's even a proposed patch (two even).

Comment 6

[u279076]

Sorry Michael. It's not being ignored, just missed due to the sheer amount of other work happening in the Mozilla project. I will ping Paul on IRC to make sure he is aware of this bug.

Comment 7

[Michael Kraft [:morac]]

Still nothing nearly 2 months later.  I hit this problem on a near daily basis do to fact that I never open new browser windows, so any time I end up with a popup window and close and restart the browser it says it crashed..

Comment 8

[Danial Horton]

easy fix,

install tabmix plus and set it to open all popups in a tab.

Comment 9

[Michael Kraft [:morac]]

(In reply to Danial Horton from comment #8)
> easy fix,
> 
> install tabmix plus and set it to open all popups in a tab.

That's a work around, not a fix.  Plus some links in web pages open new windows as "popups" (no toolbars) regardless of that setting.

Comment 10

[Danial Horton]

"Plus some links in web pages open new windows as "popups" (no toolbars) regardless of that setting."

if you've set it properly this never happens

Open Links that open in a new tab : New Tab
Javascript and popup restriction: Open all popups in tabs.

Never ever experienced popup.

I consider allowing the saving of popups to be a session a security issue though, since many banking sites pop up a HTTPS window.

Comment 11

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

Hey Michael, I'm really sorry for the delay. I did look at this a while back but never finished thinking it through. I think the patch is mostly ok, but my concern is the empty window. It may be fine (I think that will just open an empty window & then the popups as well) but I need to run through it / see if we have a test to cover it.

Comment 12

[Michael Kraft [:morac]]

Just for clarification, are you waiting on me for something at this point or are you still working on this?

Comment 13

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

It's still on my list. Some higher priority things have popped up over the past few days.

Comment 14

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

I just tried both of these patches and the end behavior was still the same: a single window was opened with the popup turned into a normal window. In that case I'm inclined to just take the first patch since it has the least churn and doesn't add a window that isn't used.

Michael, does that line up with the behavior you had intended while writing these patches?

Comment 15

[Michael Kraft [:morac]]

The patch I put together prevented the exception on shutdown which was causing the session state at the time of shutdown not to be saved. My original intent was to prevent all the old closed popup windows (usually from bank web sites and the like) from popping up the next time I ran Firefox.  A single old popup window is better than a bunch of them I guess.  

Personally I think that if the user closes Firefox with no popup windows open and a single blank tab in the browser, that's the way Firefox should open.  Basically the browser should look like it did when the user quit.

Comment 16

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

(In reply to Michael Kraft [:morac] from comment #15)
> Personally I think that if the user closes Firefox with no popup windows
> open and a single blank tab in the browser, that's the way Firefox should
> open.  Basically the browser should look like it did when the user quit.

I hear you, but I'm not going to change that behavior right now.

Comment 17

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

https://hg.mozilla.org/integration/fx-team/rev/392c2c86c5bc

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/392c2c86c5bc

Comment 19

[Michael Kraft [:morac]]

I think this is causing a new problem.  When I shut down Firefox, sometimes when I open it, I get nothing but popups opening.  Before I used to get the main window and popups, now I don't even get the main window.