832325 - Undo close tab no longer works in permanent Private Browsing mode (“Never remember history”).

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[Kurian Thampy]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20130113 Firefox/20.0
Build ID: 20130113042017

Steps to reproduce:

Closed a tab in Private Browsing mode and pressed Ctrl-Shift-T.


Actual results:

Nothing.


Expected results:

The last tab should have reopened even in Private Browsing mode and should only have been cleared when the window was closed. It's always worked like this from the beginning.

Comment 1

[Josh Matthews [:jdm]]

Hmm, I don't see any problems on nightly. Kurian, if you run a build from http://nightly.mozilla.org/, do you see the same problem?

Comment 2

[Kurian Thampy]

I did some tests and this only occurs when you go to Tools > Options and set Aurora to Never Remember History. When this is enabled the colour of the title bar changes to the same as Private Browsing mode so I assumed it was the same. Never the less it's inconsistent behaviour.

Comment 3

[Kurian Thampy]

(In reply to Josh Matthews [:jdm] from comment #1)
> Hmm, I don't see any problems on nightly. Kurian, if you run a build from
> http://nightly.mozilla.org/, do you see the same problem?

I did some tests and this only occurs when you go to Tools > Options and set Aurora to Never Remember History. When this is enabled the colour of the title bar changes to the same as Private Browsing mode so I assumed it was the same. Never the less it's inconsistent behaviour.

Comment 4

[[:Aleksej]]

Confirmed with Aurora Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20130207 Firefox/20.0

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this a design issue?  What is supposed to happen with the history when you go from one window of private browsing, closing the tab and then re-opening it with this shortcut?

Comment 6

[Josh Matthews [:jdm]]

It's not a design issue; it's a regression. Undoing the close tab operation works in regular private browsing windows, but not when permanent private browsing mode is enabled.

Comment 7

[Andres Hernandez [:andreshm]]

I tested it and confirmed it. Researching, I found that the nsSessionStartup is not initializing because of the following condition: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStartup.js#75. There are some promises related and waiting in the SessionStore. Therefore, the SessionStore doesn't initialized the window and is not receiving any event in the handleEvent method. Without the TabClose event, it's not possible to undo the closed tab. 

Not really sure how to tackle this, without removing that condition, any ideas?

Comment 8

[(no longer active)]

Attachment: Patch (v1)

This check was added in bug 482334, so I don't think we can easily remove it without regressing that bug.  We should just resolve the promise once we don't have any initialization work to do.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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

Review of attachment 715296 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/nsSessionStartup.js
@@ +72,5 @@
>     */
>    init: function sss_init() {
>      // do not need to initialize anything in auto-started private browsing sessions
> +    if (PrivateBrowsingUtils.permanentPrivateBrowsing) {
> +      gOnceInitializedDeferred.resolve();

You also need to set this._initialized=true, otherwise |_ensureInitialized()| would synchronously read the state and let |get state()| return it.

I also wonder if we should to notify observers and send "sessionstore-state-read" with an empty string, like _onSessionFileRead() does.

Comment 10

[(no longer active)]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> Comment on attachment 715296 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 715296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/sessionstore/src/nsSessionStartup.js
> @@ +72,5 @@
> >     */
> >    init: function sss_init() {
> >      // do not need to initialize anything in auto-started private browsing sessions
> > +    if (PrivateBrowsingUtils.permanentPrivateBrowsing) {
> > +      gOnceInitializedDeferred.resolve();
> 
> You also need to set this._initialized=true, otherwise
> |_ensureInitialized()| would synchronously read the state and let |get
> state()| return it.

OK, will do.

> I also wonder if we should to notify observers and send
> "sessionstore-state-read" with an empty string, like _onSessionFileRead()
> does.

I didn't do this on purpose, since that would be sort of lying, since we actually don't read the sessionstate here, right?

Comment 11

[(no longer active)]

Attachment: Patch (v2)

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 717333 [details] [diff] [review]
Patch (v2)

Review of attachment 717333 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/nsSessionStartup.js
@@ +73,5 @@
>    init: function sss_init() {
>      // do not need to initialize anything in auto-started private browsing sessions
> +    if (PrivateBrowsingUtils.permanentPrivateBrowsing) {
> +      gOnceInitializedDeferred.resolve();
> +      this._initialized = true;

Please swap those lines. Resolving the promise may call callbacks that may in turn call nsSessionStartup again and we'd better be initialized by then :)

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Ehsan Akhgari from comment #10)
> > I also wonder if we should to notify observers and send
> > "sessionstore-state-read" with an empty string, like _onSessionFileRead()
> > does.
> 
> I didn't do this on purpose, since that would be sort of lying, since we
> actually don't read the sessionstate here, right?

Yes, I agree. I had the same thoughts and was just thinking out loud. Let's not do this.

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4631558de6c

Comment 15

[(no longer active)]

Comment on attachment 717333 [details] [diff] [review]
Patch (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): pbngen
User impact if declined: See comment 0
Testing completed (on m-c, etc.): Locally
Risk to taking this patch (and alternatives if risky): Should be very low risk, this just finishes the initialization of the nsSessionStartup component.
String or UUID changes made by this patch: none.

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/a4631558de6c

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 717333 [details] [diff] [review]
Patch (v2)

low risk, we can take this on beta 2 with time to backout should there be regressions.

Comment 18

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ce84c1fcfc7
https://hg.mozilla.org/releases/mozilla-beta/rev/eef562ec97c8

Comment 19

[Ioana (away)]

Verified as fixed on:
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0

Comment 20

[u279076]

Flagging for verification against Firefox 21 and 22.

Comment 21

[Ioana (away)]

Is this issue covered by an automated test?

Comment 22

[Mitza]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0

Verified as fixed on Firefox 21 Beta2(Build ID:20130408165307).

Comment 23

[Mitza]

Mozilla/5.0 (Windows NT 6.2; WOW64; rv:22.0) Gecko/20130415 Firefox/22.0
Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:23.0) Gecko/20130415 Firefox/23.0

Also verified as fixed on Firefox 22(Build ID:20130415004014) and on Firefox 23 (Build ID:20130415030812).

Comment 24

[u279076]

Thank you Mitza.