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

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Infocatcher]

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.

Comment 1

[Infocatcher]

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/");

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-956826-Check-docShell-s-private-browsing-state-w.patch

Simple patch.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

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?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/60992f5bcf26

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/60992f5bcf26

Comment 8

[Christian Ascheberg]

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)?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(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?

Comment 10

[Christian Ascheberg]

(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.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 12

[Infocatcher]

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

Comment 13

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(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.

Comment 14

[YUKI "Piro" Hiroshi]

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.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Filed bug 961380 to discuss bringing undo closing tabs back.

Comment 16

[Manuela Muntean [Away]]

> 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?

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 18

[Manuela Muntean [Away]]

(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?

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 20

[Manuela Muntean [Away]]

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.