Closed Bug 956826 Opened 6 years ago Closed 6 years ago

[Session Restore] Private tabs are saved in the session sometimes

Categories

(Firefox :: Session Restore, defect)

29 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 29

People

(Reporter: infocatcher.bugs, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131205075310

Steps to reproduce:

(from https://bugzilla.mozilla.org/show_bug.cgi?id=899276#c48)

Testcase for Scratchpad (devtools.chrome.enabled = true + Environment – Browser):


// Works fine, private tab isn't saved:
window.addEventListener("TabOpen", function makeTabPrivate(e) {
	window.removeEventListener("TabOpen", makeTabPrivate, false);
	var tab = e.target;
	var browser = tab.linkedBrowser;
	var privacyContext = PrivateBrowsingUtils.privacyContextFromWindow(browser.contentWindow);
	privacyContext.usePrivateBrowsing = true;
	// Wait some time and try restart
	browser.addEventListener("load", function waitForLoading(e) {
		browser.removeEventListener("load", waitForLoading, true);
		setTimeout(function() {
			Application.restart();
		}, 5e3);
	}, true);
}, false);
gBrowser.addTab("https://addons.mozilla.org/");


// Doesn't work, private tab saved in the session:
window.addEventListener("TabOpen", function makeTabPrivate(e) {
	window.removeEventListener("TabOpen", makeTabPrivate, true /* !!! */);
	var tab = e.target;
	var browser = tab.linkedBrowser;
	var privacyContext = PrivateBrowsingUtils.privacyContextFromWindow(browser.contentWindow);
	privacyContext.usePrivateBrowsing = true;
	// Wait some time and try restart
	browser.addEventListener("load", function waitForLoading(e) {
		browser.removeEventListener("load", waitForLoading, true);
		setTimeout(function() {
			Application.restart();
		}, 5e3);
	}, true);
}, true /* !!! */);
gBrowser.addTab("https://addons.mozilla.org/");


Actual results:

Private tabs are saved in the session in second case with useCapture flag in the event listener.


Expected results:

Private tabs shouldn't be saved in both cases.
Blocks: 886447, 944557
Component: Untriaged → Session Restore
OS: Windows 7 → All
Hardware: x86 → All
Also works wrong in following case:


var tab = gBrowser.addTab("about:blank");
var browser = tab.linkedBrowser;
browser.stop();
browser.addEventListener("load", function waitForLoading(e) {
	browser.removeEventListener("load", waitForLoading, true);
	browser.stop();
	setTimeout(function() {
		var privacyContext = PrivateBrowsingUtils.privacyContextFromWindow(browser.contentWindow);
		privacyContext.usePrivateBrowsing = true;
		browser.addEventListener("load", function waitForLoading2(e) {
			browser.removeEventListener("load", waitForLoading2, true);
			setTimeout(function() {
				Application.restart();
			}, 5e3);
		}, true);
		//browser.reload();
		browser.loadURI("https://mozilla.org/");
	}, 500);
}, true);
browser.loadURI("https://addons.mozilla.org/");
Do you know whether |privacyContext.usePrivateBrowsing = true| succeeds at broadcasting "SessionStore:update" with |isPrivate: true|?

Side-note: we probably need a good way to debug such things.
Assignee: nobody → ttaubert
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8360308 [details] [diff] [review]
0001-Bug-956826-Check-docShell-s-private-browsing-state-w.patch

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

Good catch.

::: browser/components/sessionstore/test/browser_privatetabs.js
@@ +69,5 @@
> +    "docShell.QueryInterface%28Ci.nsILoadContext%29.usePrivateBrowsing%3Dtrue";
> +
> +  // Create a new window to attach our frame script to.
> +  let win = yield promiseNewWindowLoaded();
> +  win.messageManager.loadFrameScript(FRAME_SCRIPT, true);

I'm not sure I understand the loading of frame scripts.
Will FRAME_SCRIPT be loaded before content-sessionStore.js?
Attachment #8360308 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #4)
> I'm not sure I understand the loading of frame scripts.
> Will FRAME_SCRIPT be loaded before content-sessionStore.js?

Yes it will be. loadFrameScript() does something similar to sending a message to the content process and they're processed in order afaict. I checked that the test is failing without the fix.
https://hg.mozilla.org/mozilla-central/rev/60992f5bcf26
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Is this bug the reason for why it is not possible anymore to undo close tabs and windows in private browsing and permanent private browsing mode? If so, is this intended (in both cases)?
(In reply to Christian Ascheberg from comment #8)
> Is this bug the reason for why it is not possible anymore to undo close tabs
> and windows in private browsing and permanent private browsing mode? If so,
> is this intended (in both cases)?

This has been the case for quite some time, isn't it?
(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #9)
> This has been the case for quite some time, isn't it?

It worked in yesterdays nightly, and I could not find any other specific bug in that range.
Yeah, since bug 899276 we don't put private tabs into the closed tabs array anymore. Starting with this bug this took effect for private windows. The question is now: what is the desired behavior? Firefox used to be able to undo close tabs in private windows. Chrome in its current version does not support restoring closed tabs in private windows.

Boriss, are you the right UX person to ask for this? What is the desired behavior for "Undo Close Tab" in private windows? In permanent private browsing mode?

I'm betting money that not being able to restore closed tabs in permanent private browsing mode would piss of some people, including TorBrowser users. Should we revert and have the same behavior in private windows for consistency? Not remembering closed private tabs in a non-private window however would make sense to me.
Flags: needinfo?(jboriss)
I think, it's much better to leave all tabs and windows in internal state object as is, but save only non-private data to the disk. So, we still can undo closed tab/window, but don't save anything private.
I'm not sure about restoring of closed private windows... May be it's good to clear all data only after last private tab/window will be closed.
And do nothing special for private tabs, just because there is no official support for them. Also I already have some similar enhancements for Private Tab extension: https://github.com/Infocatcher/Private_Tab/issues/112
(In reply to Tim Taubert [:ttaubert] from comment #11)
> Yeah, since bug 899276 we don't put private tabs into the closed tabs array
> anymore. Starting with this bug this took effect for private windows. The
> question is now: what is the desired behavior? Firefox used to be able to
> undo close tabs in private windows. Chrome in its current version does not
> support restoring closed tabs in private windows.
> 
> Boriss, are you the right UX person to ask for this? What is the desired
> behavior for "Undo Close Tab" in private windows? In permanent private
> browsing mode?
> 
> I'm betting money that not being able to restore closed tabs in permanent
> private browsing mode would piss of some people, including TorBrowser users.
> Should we revert and have the same behavior in private windows for
> consistency? Not remembering closed private tabs in a non-private window
> however would make sense to me.

I think Firefox should be able to restore closed tabs in a private window. The behavior of Chrome in its current version is just wrong. We should not follow Chrome about this point. Everyone sometimes close their tabs by their mistake in both normal window and private one. We should to provide the way to catch its mistake. Do you want to use the text editor which does not have any "undo" feature? No! I don't want it.

Of course some people would like to disable to restore tabs in a private window. But I don't think to make sense it as default way. It should be provided as an optional feature by some pref. Basically, We should not do provide the rigid way for human error.

By the way, I don't need to provide restoring last closed private windows. Because we adopt per-window private browsing model. We can assume that each windows are disappear when closing each of them. But this thinking may be wrong. I don't fussy about the behavior of a private window.
I agree to two points: 1) Firefox should provide "undo close tab" feature always by the reason told in the comment #13, 2) but Firefox should not save private sessions to the disk. So, private tabs should be restorable with on-memory volatile session data, while the process is alive.
Blocks: 899276
Depends on: 961380
Filed bug 961380 to discuss bringing undo closing tabs back.
Flags: needinfo?(jboriss)
Keywords: verifyme
> Steps to reproduce:
> 
> (from https://bugzilla.mozilla.org/show_bug.cgi?id=899276#c48)
> 
> Testcase for Scratchpad (devtools.chrome.enabled = true + Environment –
> Browser):
> 
> 
> // Works fine, private tab isn't saved:
> window.addEventListener("TabOpen", function makeTabPrivate(e) {
> 	window.removeEventListener("TabOpen", makeTabPrivate, false);
> 	var tab = e.target;
> 	var browser = tab.linkedBrowser;
> 	var privacyContext =
> PrivateBrowsingUtils.privacyContextFromWindow(browser.contentWindow);
> 	privacyContext.usePrivateBrowsing = true;
> 	// Wait some time and try restart
> 	browser.addEventListener("load", function waitForLoading(e) {
> 		browser.removeEventListener("load", waitForLoading, true);
> 		setTimeout(function() {
> 			Application.restart();
> 		}, 5e3);
> 	}, true);
> }, false);
> gBrowser.addTab("https://addons.mozilla.org/");

With the Nightly from 2013-12-05 (build ID: 20131205030207) on Win 8 x64, after running the above code, when the browser restarts, the tab containing https://addons.mozilla.org/ is still visible
 
> // Doesn't work, private tab saved in the session:
> window.addEventListener("TabOpen", function makeTabPrivate(e) {
> 	window.removeEventListener("TabOpen", makeTabPrivate, true /* !!! */);
> 	var tab = e.target;
> 	var browser = tab.linkedBrowser;
> 	var privacyContext =
> PrivateBrowsingUtils.privacyContextFromWindow(browser.contentWindow);
> 	privacyContext.usePrivateBrowsing = true;
> 	// Wait some time and try restart
> 	browser.addEventListener("load", function waitForLoading(e) {
> 		browser.removeEventListener("load", waitForLoading, true);
> 		setTimeout(function() {
> 			Application.restart();
> 		}, 5e3);
> 	}, true);
> }, true /* !!! */);
> gBrowser.addTab("https://addons.mozilla.org/");

With the Nightly from 2013-12-05 (build ID: 20131205030207) on Win 8 x64, after running this code, when the browser restarts, the tab containing https://addons.mozilla.org/ is not visible anymore.

Is this the intended behavior?
Flags: needinfo?(ttaubert)
Running both test cases, I don't see the extra tab after restarting with Nightly. You seem to be using a Nightly from before this landed to verify?
Flags: needinfo?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #17)
> Running both test cases, I don't see the extra tab after restarting with
> Nightly. You seem to be using a Nightly from before this landed to verify?

I first wanted to reproduce the issue, but I don't think I've succeeded, because from my point of view, the results I've posted in comment 16 are different from the ones in comment 0. From your comment 17, I'm understanding that my results from comment 16 are expected and they describe the issue, right?
Yes, the issue seems to be somewhat reproducible before we fixed it. So I think what you're seeing in comment #16 is expected. We should never restore the private tab in the current version though.
Verified as fixed with Firefox 29 beta 5 on Win 7 x64, Mac OS X 10.9 and Ubuntu 12.10 x86: after running both pieces of code from comment 0, the private tab isn't restored.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.