368677 - Sessionstore should ignore popup windows

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P5)

Description

[Volkmar Kostka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070129 Minefield/3.0a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070129 Minefield/3.0a2pre

Sessionstore should ignore page generated popup windows. It is simply annoying if you have forgotton that a popup is open and you closed your main window to find out that there is still a popup window. In this case your sessionstore will only contain this popup window but not the content of the main window. So you lose your complete session.
I think it is safe to assume that popup windows almost never contain important data so storing them is rather useless.

Reproducible: Always

Steps to Reproduce:
1. Visit a page where you can open a popup window e.g. http://www.mozillazine.org/forums/
2. Open the smilie popup.
3. Close the main window
4. Close the popup
5. restart firefox
Actual Results:  
Only the smilie window is restored

Expected Results:  
The last closed main window should be restored

Comment 1

[Dave Townsend [:mossop]]

I disagree that it is safe to assume that. I think in reality the ease of restoring multiple windows needs to be improved so that both the popup window and main window are restored.

The most common use case for closing multiple windows is closing them one at a time which sessionstore doesn't support, whereas it does handle closing them all at once with file - exit.

Comment 4

[Simon Bünzli]

Attachment: always restore at least one non-popup window (and all popups closed afterwards)

Comment 5

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

Blocking since we need to figure out what the right thing is here, and it might be something about handling session saving better to avoid the bad use case illustrated.

Comment 6

[Dave Townsend [:mossop]]

Just to throw in a thought here, possibly something slightly different. I use a Mac and I'm forever closing the window, which of course doesn't quit the app. So when the app is quitted and restarted as far as it is concerned there was nothing open previously so doesn't restore.

Comment 7

[Simon Bünzli]

(In reply to comment #6)
> possibly something slightly different.

Slightly enough that it doesn't have much to do with this bug at all. Please file a new one... so that we can introduce a pref browser.sessionstore.allow_empty_session or something to disable what we're currently doing (resp. what bug 354894 will even improve).

Comment 8

[Simon Bünzli]

(In reply to comment #6)
> So when the app is quitted and restarted as far as it is concerned there was
> nothing open previously so doesn't restore.

Filed bug 382006 to that end.

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #8)
> (In reply to comment #6)
> > So when the app is quitted and restarted as far as it is concerned there was
> > nothing open previously so doesn't restore.
> 
> Filed bug 382006 to that end.

I'm afraid you've misunderstood me, and apparently what I was after already does work, I was mistakenly thinking of something else.

Comment 10

[Mike Connor [:mconnor]]

So, the big problem I think we need to solve is (on Windows) how to handle the "I closed my app window, and found a lurking popup/Download Manager that trashed my session" case.  I'm not sure whether this helps here, or not.

Comment 11

[Simon Bünzli]

(In reply to comment #10)
> "I closed my app window, and found a lurking popup/Download Manager that
> trashed my session" case.  I'm not sure whether this helps here, or not.

The suggested solution (cf. comment #4) is to restore any popup together with the last closed app window (or only the app window, although then you might lose data the other way round). We already do that in case you've missed the Download Manager (which currently is only involved in bug 354894).

So at worst a user would have to close a popup twice (when closing down and after restoring the session).

An alternative would be to offer a Recently Closed Windows list, although we'd have to place it in a way that users will actually find it...

Comment 12

[Simon Bünzli]

Comment on attachment 265837 [details] [diff] [review]
always restore at least one non-popup window (and all popups closed afterwards)

Canceling review request as it's still not clear this is really the way to go and I might not have the time to implement it in a different way.

Comment 13

[Mike Connor [:mconnor]]

We need to figure this out, but doesn't need to happen for b1

Comment 14

[Florian Quèze [:florian]]

So what should be done here?

Is the solution suggested in comment 4 the way to go? In this case the patch probably needs to be updated.

Or is the solution suggested by the summary (ignore popup windows) prefered? Btw this would fix bug 393466.

Comment 16

[Cor'e]

From Bug 410883
"Session Restore seems to only work when all of FireFox is completely closed
down and then restarted, i need it to either allow a session restore
immediately or to remember some short session history that i may choose when
restarted so that i may recover my working session(s) from accidental mishaps."

Comment 17

[Jonathan Jacobs]

Some of the proposals here seem to overlap with bug 409492.

Comment 20

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

I don't understand the behaviour of patch proposed in comment 4

Anyway, I think if this bug will be fixed, I hope this behaviour will be optional and disabled by default. Not all popups are evil and useless, and if Firefox closed for a crash or a blackout, you can lose some data.

Comment 21

[Simon Bünzli]

(In reply to comment #20)
> Anyway, I think if this bug will be fixed, I hope this behaviour will be
> optional and disabled by default.

If we can help it there should be no optional behavior here - either we do it right or not at all. Then again, have you read the attachment's description ("always restore at least one non-popup window (_and_ all popups closed afterwards)")?.

Comment 22

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

(In reply to comment #21)
> have you read the attachment's description?

Of course, but I don't understand what it means.

Comment 23

[Simon Bünzli]

Attachment: unbitrotted: always restore at least one non-popup window (and all popups closed afterwards)

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is definitely a dataloss bug. It's really hard in such cases to restore the set of tabs which were opened, before accidentally closing the main window. I also ran into this trouble several times now and it's frustrating every time. We should push this forward.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Simon, due to the dataloss we have here I believe we should let it stay at critical.

"Critical  	crashes, loss of data, severe memory leak"

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

According to the severity description setting back the severity to critical:
https://bugzilla.mozilla.org/page.cgi?id=fields.html#bug_severity

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 331774 [details] [diff] [review]
unbitrotted: always restore at least one non-popup window (and all popups closed afterwards)

r=me, thanks

Comment 28

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/54cfde8b90e2

Comment 29

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20081003 Minefield/3.1b1pre

Comment 30

[u279076]

What is the expected behaviour with this bug?  Do we now discriminate against pop-ups vs multiple open windows (i.e. multiple open windows will be restored but "pop-ups" will not)?

Please clarify for the purposes of writing a Litmus test case.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #30)
> Please clarify for the purposes of writing a Litmus test case.

See the attachment description in comment 23.

Simon, can this be automated? If yes, we would still need a Litmus test for testing a real restart?

Comment 32

[Simon Bünzli]

Attachment: testcase

(In reply to comment #30)
> What is the expected behaviour with this bug?

That closing popups after the last non-popup browser window doesn't lose you that non-popup window (instead it and all popups closed afterwards should be restored). The attached testcase should help for creating a Litmus test.

Comment 33

[Simon Bünzli]

(In reply to comment #31)
> can this be automated?

There's no way for automating crashes and restarts at the moment. Sorry.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #33)
> (In reply to comment #31)
> > can this be automated?
> 
> There's no way for automating crashes and restarts at the moment. Sorry.

Thanks Simon. Just one more question. Do you really need a crash for unit tests? Independently from the answer we can take care of it with a Mozmill test.

Comment 35

[Simon Bünzli]

(In reply to comment #34)
> Do you really need a crash for unit tests?

No, a controlled restart works for almost all of the Session Restore functionality as well, especially for verifying that everything that should be restored actually is.

Comment 36

[u279076]

Simon, in your test case you note that it does not work on OSX because the X button does not quit Firefox.  Would closing all windows with the X button then clicking Firefox menu > Quit satisfy the test for OSX?

Comment 37

[u279076]

I tested comment 36 on OSX...Minefield opens with a single window open to untitled using the following STR:

1. Open the Preferences dialog
2. Click the Content panel and uncheck "Block pop-up windows"
3. Click the Main panel and set "When Firefox starts" to "Show my windows and tabs from last time"
4. Close the Preferences dialog
5. Open this URL: <https://litmus.mozilla.org/testcase_files/firefox/5918/index.html>
6. Click the HOME button on the main window
7. Close the main window using the X button once your homepage loads
8. Close all pop-ups using the X button
9. Click Firefox menu > Quit
10. Start Firefox

Is this expected?

FYI, on Win/Lin, main window and pop-ups are restored (omitting step 9 of course).

Comment 38

[u279076]

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7806

This is a preliminary test case.  I'll update it as I get feedback on my previous comments.

Comment 39

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

(In reply to comment #37)
> I tested comment 36 on OSX...Minefield opens with a single window open to
> untitled using the following STR:
...
> Is this expected?
> 
> FYI, on Win/Lin, main window and pop-ups are restored (omitting step 9 of
> course).

Yes, this is expected. See bug 481090.

Comment 40

[Simon Bünzli]

(In reply to comment #38)
> https://litmus.mozilla.org/show_test.cgi?id=7806

Looks good, thanks.