479994 - Session not restored when exiting from PB mode using "X"

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug affects only Windows and Linux where you can quit a session using the "X" button. It does not affect Mac.

STR:
1. Open a regular browsing session and open some tabs.
2. Switch to PB mode.
3. While in PB mode, close the browser using the "X" button
4. Restart Firefox

Expected: My regular browsing session would be restored
Actual: Regular Browsing session is not restored

During my testing I have found that if you exit using the File menu command the session is properly restored.

Bug 468565 contains more information about others that have seen this bug.

Comment 1

[Nochum Sossonko [:Natch]]

Requesting blocking because bug 468565 was only meant as a stopgap so as not to confuse the user, while the effect was loss of a feature in pb.

Comment 2

[Simon Bünzli]

Please file Private Browsing related bugs in its own component.

Comment 3

[Marcia Knous [:marcia]]

Sorry, was unsure whether this was PB or Session Restore.

(In reply to comment #2)
> Please file Private Browsing related bugs in its own component.

Comment 4

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

Adjusting OS and severity. This is a very annoying dataloss bug. Users will normally use this way for closing Firefox.

I think that cannot be automatically tested right now and needs a Litmus test, right?

Comment 5

[(no longer active)]

After my initial investigation on this, it seems that this is not really a regression from bug 468565.  Even when I locally revert the fix to that bug, this seems to happen.  It seems to me that the private-browsing exit notification is not fired at all, which is odd...

Comment 6

[Nochum Sossonko [:Natch]]

Ehsan, it seems that this is a bug with or without any others, in that when a user quits the app with the [x] the normal sequence of events isn't necessarily *guaranteed*. I'm not 100% sure of this but I do know that exiting via the File menu *may* reduce the reproducibility of this bug.

The reason why I think this has been exacerbated now with the patch for bug 468565 is because now there is no dialog, which in turn makes for a clean exit of the app. When the dialog pops-up it seems that firefox has a chance to catch up on the notifications. This is all mostly conjecture from my experience, feel free to ignore it.

I'm experiencing this on Vista, so setting OS to All.

Comment 7

[Nochum Sossonko [:Natch]]

Another thing to think about is the fact that the pb service listens to its own notification, i haven't thought much about it yet, but what if a different observer got registered first? I'll look into this to see if this could be a problem...

Comment 8

[(no longer active)]

mconnor: sorry I accidentally flipped the blocking flag here.  Could you please re-set it?  Thanks!

Comment 9

[(no longer active)]

This is a regression from bug 476463.  The offending code is: <http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#174>.  nsIWindowMediator.getMostRecentWindow returns null, which causes that code to throw, which will cause the private-browsing notification with exit parameter not being sent.

Patch following shortly.

Comment 10

[(no longer active)]

Attachment: Patch (v1)

Actually the cause was a typo which we missed in the reviews.  I fixed that typo, and with that fix along this problem was solved.  But anyway I changed the code I mentioned in the last comment to check the returned object to see if it's null, FWIW.

Comment 11

[(no longer active)]

Let's try to take this for Beta 3...

Comment 12

[Nochum Sossonko [:Natch]]

Crap. I wish there was a way this could be tested, I think some of the places' tests send the quit-application notification, can we do that here?

Comment 13

[(no longer active)]

This needs to be tested as a browser chrome test (because an xpcshell unit test can't execute this code path).  Sending that notification from within a browser chrome test will lead to chaos because various parts of the browser try to react to that notification and continuing the test harness would not be reliable.

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 364527 [details] [diff] [review]
Patch (v1)

r=me on the first part of this patch only, the other bit is done in bug 479461

Comment 15

[(no longer active)]

Oh, sorry I had missed that bug...

Comment 16

[(no longer active)]

mconnor: can I land this on branch as well without needing approval?  I had mistakenly removed the blocking flag on this bug (see comment 8).

Comment 17

[Mike Connor [:mconnor]]

Fixed flags, but yeah, get this in please.

Comment 18

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/4d0d6a93cbe4

Comment 19

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0d2ba8c653fb

Comment 20

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

Verified fixed on Windows and Linux with:

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090301 Minefield/3.2a1pre

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1b3pre) Gecko/20090301 Shiretoko/3.1b3pre

Comment 21

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7546 added to Litmus.

Comment 22

[(no longer active)]

An automated test makes no sense for this bug.

Comment 23

[(no longer active)]

(In reply to comment #22)
> An automated test makes no sense for this bug.

Please ignore this comment, wrong tab.