Closed Bug 368677 Opened 18 years ago Closed 16 years ago

Sessionstore should ignore popup windows

Categories

(Firefox :: Session Restore, defect, P5)

Product:
Firefox
Component:
Session Restore
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3.1b1

People

(Reporter: volkmarkostka, Assigned: zeniko)

References

(Blocks 1 open bug)

Details

(Keywords: dataloss)

Attachments

(2 files, 1 obsolete file)

unbitrotted: always restore at least one non-popup window (and all popups closed afterwards)
6.32 KB, patch
dietrich
: review+
Details | Diff | Splinter Review
testcase
564 bytes, text/html
Details 
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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
OS: Windows XP → All
Hardware: PC → All
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.
Flags: blocking-firefox3? → blocking-firefox3+
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.
(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).
(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.
(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.
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.
(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...
Target Milestone: --- → Firefox 3 beta1
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Keywords: uiwanted
Target Milestone: Firefox 3 M8 → Firefox 3 M9
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.
Attachment #265837 - Flags: review?(dietrich)
We need to figure this out, but doesn't need to happen for b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Target Milestone: Firefox 3 M10 → Firefox 3 M11
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.
Blocks: 393466
Priority: -- → P5
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Target Milestone: Firefox 3 Mx → Firefox 3 M11
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."
Some of the proposals here seem to overlap with bug 409492.
Target Milestone: Firefox 3 beta3 → ---
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.
(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)")?.
(In reply to comment #21)
> have you read the attachment's description?

Of course, but I don't understand what it means.
Attachment #331774 - Flags: ui-review?(beltzner)
Attachment #331774 - Flags: review?(gavin.sharp)
Attachment #331774 - Flags: ui-review?(beltzner) → review?(mano)
Attachment #331774 - Flags: review?(gavin.sharp)
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.
Severity: normal → critical
Flags: blocking-firefox3.1?
Keywords: dataloss
Version: unspecified → Trunk
Assignee: nobody → zeniko
Severity: critical → major
Keywords: uiwanted
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"
Attachment #331774 - Flags: review?(dietrich)
According to the severity description setting back the severity to critical:
https://bugzilla.mozilla.org/page.cgi?id=fields.html#bug_severity
Severity: major → critical
Comment on attachment 331774 [details] [diff] [review]
unbitrotted: always restore at least one non-popup window (and all popups closed afterwards)

r=me, thanks
Attachment #331774 - Flags: review?(dietrich) → review+
Keywords: checkin-needed
Attachment #331774 - Flags: review?(mano)
http://hg.mozilla.org/mozilla-central/rev/54cfde8b90e2
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b1
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20081003 Minefield/3.1b1pre
Status: RESOLVED → VERIFIED
Flags: blocking-firefox3.1?
Flags: in-litmus?
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.
(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?
Flags: in-testsuite?
Attached file 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.
(In reply to comment #31)
> can this be automated?

There's no way for automating crashes and restarts at the moment. Sorry.
(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.
(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.
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?
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).
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.
Flags: in-litmus? → in-litmus+
(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.
(In reply to comment #38)
> https://litmus.mozilla.org/show_test.cgi?id=7806

Looks good, thanks.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: