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

VERIFIED FIXED in Firefox 20

Status

()

VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: kathampy, Assigned: Ehsan)

Tracking

({regression})

20 Branch
Firefox 22
x86_64
Windows 8
regression
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox20+ verified, firefox21 verified, firefox22 verified)

Details

(Whiteboard: [appcoast])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Component: Untriaged → Private Browsing

Comment 1

6 years ago
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?
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

6 years ago
(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.

Updated

6 years ago
Blocks: 463027
Summary: Undo close tab no longer works in Private Browsing mode. → Undo close tab no longer works in permanent Private Browsing mode.
Whiteboard: [appcoast]
(Assignee)

Updated

6 years ago
Assignee: nobody → andres

Comment 4

6 years ago
Confirmed with Aurora Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20130207 Firefox/20.0
Summary: Undo close tab no longer works in permanent Private Browsing mode. → Undo close tab no longer works in permanent Private Browsing mode (“Never remember history”).

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

6 years ago
tracking-firefox20: --- → ?
Keywords: regression
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?
tracking-firefox20: ? → +
Flags: needinfo?(ehsan)

Comment 6

6 years ago
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.
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 8

6 years ago
Created attachment 715296 [details] [diff] [review]
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.
Assignee: andres → ehsan
Attachment #715296 - Flags: review?(ttaubert)
Flags: needinfo?(ehsan)
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.
(Assignee)

Comment 10

6 years ago
(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?
(Assignee)

Comment 11

6 years ago
Created attachment 717333 [details] [diff] [review]
Patch (v2)
Attachment #715296 - Attachment is obsolete: true
Attachment #715296 - Flags: review?(ttaubert)
Attachment #717333 - Flags: review?(ttaubert)
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 :)
Attachment #717333 - Flags: review?(ttaubert) → review+
(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.
(Assignee)

Comment 15

6 years ago
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.
Attachment #717333 - Flags: approval-mozilla-beta?
Attachment #717333 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/a4631558de6c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
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.
Attachment #717333 - Flags: approval-mozilla-beta?
Attachment #717333 - Flags: approval-mozilla-beta+
Attachment #717333 - Flags: approval-mozilla-aurora?
Attachment #717333 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 18

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ce84c1fcfc7
https://hg.mozilla.org/releases/mozilla-beta/rev/eef562ec97c8
status-firefox20: --- → fixed
status-firefox21: --- → fixed
status-firefox22: --- → fixed

Comment 19

6 years ago
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
status-firefox20: fixed → verified
QA Contact: ioana.budnar
Flagging for verification against Firefox 21 and 22.
Keywords: verifyme

Comment 21

6 years ago
Is this issue covered by an automated test?
Flags: in-testsuite?

Comment 22

5 years ago
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

5 years ago
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).
Thank you Mitza.
Status: RESOLVED → VERIFIED
status-firefox21: fixed → verified
status-firefox22: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.