Closed
Bug 832325
Opened 11 years ago
Closed 11 years ago
Undo close tab no longer works in permanent Private Browsing mode (“Never remember history”).
Categories
(Firefox :: Private Browsing, defect)
Tracking
()
VERIFIED
FIXED
Firefox 22
People
(Reporter: k, Assigned: ehsan.akhgari)
References
Details
(Keywords: regression, Whiteboard: [appcoast])
Attachments
(1 file, 1 obsolete file)
958 bytes,
patch
|
ttaubert
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•11 years ago
|
Component: Untriaged → Private Browsing
Comment 1•11 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•11 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•11 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•11 years ago
|
Blocks: PBnGen
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•11 years ago
|
Assignee: nobody → andres
Comment 4•11 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•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•11 years ago
|
tracking-firefox20:
--- → ?
Keywords: regression
Comment 5•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(ehsan)
Comment 6•11 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.
Updated•11 years ago
|
Status: NEW → ASSIGNED
Comment 7•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 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•11 years ago
|
||
Attachment #715296 -
Attachment is obsolete: true
Attachment #715296 -
Flags: review?(ttaubert)
Attachment #717333 -
Flags: review?(ttaubert)
Comment 12•11 years ago
|
||
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+
Comment 13•11 years ago
|
||
(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 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4631558de6c
Assignee | ||
Comment 15•11 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?
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a4631558de6c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Comment 17•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ce84c1fcfc7 https://hg.mozilla.org/releases/mozilla-beta/rev/eef562ec97c8
Comment 19•11 years ago
|
||
Verified as fixed on: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
QA Contact: ioana.budnar
Comment 22•11 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•11 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).
You need to log in
before you can comment on or make changes to this bug.
Description
•