Since each docShell has an attribute |usePrivateBrowsing|, we should make sure that we do not collect/save private tabs.

Here's a simple implementation.

Comment on attachment 783203 [details] [diff] [review] Don't collect private tabs Review of attachment 783203 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionStore.jsm @@ +1337,5 @@ > } > > + // Private tabs should not be saved > + let loadContext = aTab.linkedBrowser.docShell.QueryInterface(Ci.nsILoadContext); > + if (loadContext.usePrivateBrowsing) { _collectBaseTabData() indicates that aTab.linkedBrowser might be undefined. The same is probably true for the docShell property. You could put this in a simple helper function at the top, like isPrivateTab(). @@ +2589,5 @@ > * Bool collect pinned tabs only > * @returns object > */ > + _getCurrentState: function ssi_getCurrentState(aOptions = {}) { > + let includeAllWindows = !!aOptions.includeAllWindows; includeAllWindows is quite a misleading name. In fact it rather is about *updating* data for all windows. They all are included anyway. @@ +2662,2 @@ > // perform a deep copy so that existing session variables are not changed. > total = JSON.parse(this._toJSONString(total)); Sorry for the rot but this has been removed \o/. @@ +2740,5 @@ > // update the internal state data for this window > for (let tab of tabs) { > + // Generally, we want to skip private tabs > + if (!includePrivateTabs) { > + let loadContext = tab.linkedBrowser.docShell.QueryInterface(Ci.nsILoadContext); I think we should just always collect data for private tabs but remove them in .saveState() just like we do for private windows. It will certainly help if we'd add a .private property to tabData to make the filter step a little faster. Also because that .private flag will be sent with the data collected in the content process (in the future).

(In reply to Tim Taubert [:ttaubert] from comment #2) > > I think we should just always collect data for private tabs but remove them > in .saveState() just like we do for private windows. It will certainly help > if we'd add a .private property to tabData to make the filter step a little > faster. Also because that .private flag will be sent with the data collected > in the content process (in the future). If we do this, getBrowserState(), etc. will return the private tabs. Is this what you want?

(In reply to David Rajchenbach Teller [:Yoric] from comment #3) > If we do this, getBrowserState(), etc. will return the private tabs. Is this > what you want? Yes, just like it includes private windows.

Comment on attachment 811369 [details] [diff] [review] Don't collect private tabs, v2 Review of attachment 811369 [details] [diff] [review]: ----------------------------------------------------------------- Patch needs rebasing. ::: browser/components/sessionstore/src/SessionSaver.jsm @@ +234,5 @@ > } > } > > + // Removed private tabs from the list of closed tabs. > + for (let i = state.windows.length - 1; i >= 0; i--) { We don't need to do that because we never put them into _closedTabs in the first place, right? ::: browser/components/sessionstore/src/SessionStore.jsm @@ +4315,5 @@ > }, > > + _isPrivateTab: function(browser) { > + return (browser && browser.docShell && > + browser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing); Nit: two spaces and we don't really need the parenthesis. @@ +4369,5 @@ > } > > + if (TabState._isPrivateTab(browser)) { > + tabData.isPrivate = true; > + } // Don't set anything if the tab is not private, to save space Nit: the comment at the end looks weird

Applied feedback.

Comment on attachment 820233 [details] [diff] [review] Don't collect private tabs, v3 Review of attachment 820233 [details] [diff] [review]: ----------------------------------------------------------------- How up-to-date is your repo? browser/components/sessionstore/test/Makefile.in doesn't exist anymore here.

Rebased

What does this patch do? I thought we already handled private windows specially. Do we ever have private tabs that belong to non-private windows? I'm worried because this isn't going to work in e10s since it touches the docshell. We could find a way to do it, but it will be more work.

(In reply to Tim Taubert [:ttaubert] from comment #5) > (In reply to David Rajchenbach Teller [:Yoric] from comment #3) > > If we do this, getBrowserState(), etc. will return the private tabs. Is this > > what you want? > > Yes, just like it includes private windows. That seems really bad. Why is that desired?

(In reply to Bill McCloskey (:billm) from comment #10) > What does this patch do? I thought we already handled private windows > specially. Do we ever have private tabs that belong to non-private windows? We have private tabs, they are just not exposed to the UI. The reason we want this bug fixed is because there is a nice (and popular) add-on that adds the missing UI to Firefox, and this add-on works around the lack of support for private tabs in Session Restore by [ab]using a not-quite-documented notification that was included in Session Restore. We need to get rid of that notification for bug 886447, which should provide an additional performance boost. > I'm worried because this isn't going to work in e10s since it touches the > docshell. We could find a way to do it, but it will be more work. Sounds like I should rewrite my patch to get the information child-side, then.

(In reply to Bill McCloskey (:billm) from comment #10) > I'm worried because this isn't going to work in e10s since it touches the > docshell. We could find a way to do it, but it will be more work. Good point. Thanks for catching this! It's all too easy to forget about e10s :/ (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #11) > (In reply to Tim Taubert [:ttaubert] from comment #5) > > (In reply to David Rajchenbach Teller [:Yoric] from comment #3) > > > If we do this, getBrowserState(), etc. will return the private tabs. Is this > > > what you want? > > > > Yes, just like it includes private windows. > > That seems really bad. Why is that desired? I wouldn't say it's desired but that's just the way it's been ever since we had private windows. I think it is the correct thing to do for getBrowserState() to return the full state including private windows and tabs. Just like it returned the private state back when we had the PB *mode* (iirc). The method just returns the current state and it's the caller's responsibility what do to with that data now. We only guarantee to never save sensitive data to disk and that we won't be able to restore it.

Comment on attachment 820244 [details] [diff] [review] Don't collect private tabs, v4 We of course need something e10s compatible. Sorry that I didn't think of this earlier.

(In reply to comment #13) > (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #11) > > (In reply to Tim Taubert [:ttaubert] from comment #5) > > > (In reply to David Rajchenbach Teller [:Yoric] from comment #3) > > > > If we do this, getBrowserState(), etc. will return the private tabs. Is this > > > > what you want? > > > > > > Yes, just like it includes private windows. > > > > That seems really bad. Why is that desired? > > I wouldn't say it's desired but that's just the way it's been ever since we had > private windows. I think it is the correct thing to do for getBrowserState() to > return the full state including private windows and tabs. Just like it returned > the private state back when we had the PB *mode* (iirc). Hmm, I think that is my oversight. :( It's not necessarily what I wanted to happen here.

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #15) > > I wouldn't say it's desired but that's just the way it's been ever since we had > > private windows. I think it is the correct thing to do for getBrowserState() to > > return the full state including private windows and tabs. Just like it returned > > the private state back when we had the PB *mode* (iirc). > > Hmm, I think that is my oversight. :( It's not necessarily what I wanted > to happen here. We can of course re-think that decision. I still think it's not a big deal to include private state in getCurrentState (and IMO that's "correct" in terms of returning the current state) but we might as well always exclude it if that's the better thing to do from a privacy point of view.

In terms of the whole issue here, it seems that setting .usePrivateBrowsing is explicitly discouraged since bug 800193. bug 800193 comment #2 states that this might not at all be properly supported. Should we still support filtering out private tabs? If we're going to do this we should implement it based off of bug 919835 and collect the .private flag in the frame script to not break e10s.

Well, we are going to kill the Private Tabs add-on if we don't fix this bug. This is a maintained, very active, add-on, whose author is quite responsive. It would be a shame to do that. So, I would vote for collecting private flag in the frame script.

(In reply to comment #17) > In terms of the whole issue here, it seems that setting .usePrivateBrowsing is > explicitly discouraged since bug 800193. bug 800193 comment #2 states that this > might not at all be properly supported. Should we still support filtering out > private tabs? Yes, we should try to handle those cases properly when we can.

This will change to that we cannot restore tabs with "undo close tab" in private mode window, won't it? If it's so, I think this would cut off the useful feature. Chromium has the same "undo close tab" feature excluded in their private browsing windows. But it's not useful. Because we cannot restore the tab even if we close tabs by our mistake.

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #20) > This will change to that we cannot restore tabs with "undo close tab" in > private mode window, won't it? No, it won't. This is about private tabs in non-private windows that currently are saved to disk because we just don't expect them. This is only possible using add-ons as vanilla Firefox doesn't expose any UI to create private tabs (in a non-private window).

I believe that the right way to move this to the child process is to wait for bug 930967.

Rewrote the patch to make it e10s-friendly.

Comment on attachment 8338474 [details] [diff] [review] Don't collect private tabs, v5 Review of attachment 8338474 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/content/content-sessionStore.js @@ +322,5 @@ > + }, > + > + // Ci.nsIPrivacyTransitionObserver > + privateModeChanged: function(enabled) { > + MessageQueue.push("isPrivate", () => enabled?true:undefined); I would like (enabled || null) better as that's what the other collection routines use to signal 'no value'. Also, TabStateCache.updatePersistent() checks for === null. ::: browser/components/sessionstore/src/SessionSaver.jsm @@ +206,5 @@ > + } > + // The window is not private, but its tabs still might > + for (let j = win.tabs.length - 1; j >= 0 ; --j) { > + let tab = win.tabs[j]; > + if (tab.isPrivate) { if (tab.isPrivate || false) to avoid a warning in strict mode when the property doesn't exist. ::: browser/components/sessionstore/src/SessionStore.jsm @@ +1306,5 @@ > + // Don't save private tabs > + if (browser && browser.docShell && > + browser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing) { > + return; > + } We first need to flush (which is done below), then you can use |tabState.isPrivate| to see whether the tab has a private docShell. The code above doesn't work in e10s. ::: browser/components/sessionstore/test/browser.ini @@ +57,5 @@ > [browser_global_store.js] > [browser_input.js] > [browser_pageshow.js] > [browser_pageStyle.js] > +[browser_privatetabs.js] I think you forgot add the test file.

Applied feedback, except the flushing bit I don't understand.

Ok, got it.

Comment on attachment 8338508 [details] [diff] [review] Don't collect private tabs, v7 Review of attachment 8338508 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looking good! ::: browser/components/sessionstore/content/content-sessionStore.js @@ +313,5 @@ > + * By definition, tabs start in non-private mode. > + * > + * Causes a SessionStore:update message to be sent that contains > + * |true| if the tab is now private > + * |false| if the tab is now public. Maybe explain that we don't actually send the 'false' value to not store an .isPrivate=false property on every tab. ::: browser/components/sessionstore/src/SessionStore.jsm @@ +1308,5 @@ > // Get the latest data for this tab (generally, from the cache) > let tabState = TabState.collectSync(aTab); > > + // Don't save private tabs > + if (tabState.isPrivate) { if (tabState.isPrivate || false) :) ::: browser/components/sessionstore/test/browser_privatetabs.js @@ +17,5 @@ > + info("Setting up private tab"); > + tab2 = gBrowser.addTab(); > + yield promiseBrowserLoaded(tab2.linkedBrowser); > + let loadContext = tab2.linkedBrowser.docShell.QueryInterface(Ci.nsILoadContext); > + loadContext.usePrivateBrowsing = true; Can you use the /test/content.js frame script we now have for testing? This would otherwise break the test in e10s. @@ +22,5 @@ > + tab2.linkedBrowser.loadURI("about:config"); > + yield promiseBrowserLoaded(tab2.linkedBrowser); > + > + info("Checking out state"); > + yield forceSaveState(); We need a sync flush before this line here to ensure all the data has arrived at the chrome process before saving to disk. @@ +30,5 @@ > + ok(state.indexOf("about:robots") != -1, "State contains public tab"); > + ok(state.indexOf("about:config") == -1, "State does not contain private tab"); > + > + // Flush to make sure chrome received all data. > + SyncHandlers.get(tab2.linkedBrowser).flush(); gBrowser.removeTab() flushes synchronously, we don't need that line. @@ +46,5 @@ > + let canUndo = false; > + try { > + tab2 = ss.undoCloseTab(window, 0); > + canUndo = true; > + } catch (ex if ex = Cr.NS_ERROR_ILLEGAL_VALUE) { Might be easier to check that ss.getClosedTabCount() == 0.

Comment on attachment 8338508 [details] [diff] [review] Don't collect private tabs, v7 Review of attachment 8338508 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/test/browser_privatetabs.js @@ +46,5 @@ > + let canUndo = false; > + try { > + tab2 = ss.undoCloseTab(window, 0); > + canUndo = true; > + } catch (ex if ex = Cr.NS_ERROR_ILLEGAL_VALUE) { Also, when checking the number of tabs available to undoClose, you should do |while (ss.getClosedTabCount()) ss.forgetClosedWindow(0)| at the beginning of the test. This would probably break when running the whole test suite otherwise.

Applied feedback. Try: https://tbpl.mozilla.org/?tree=Try&rev=a499f2b97782

Thanks, the changes look good. Two nits to address before you push, please: +++ b/browser/components/sessionstore/test/browser_privatetabs.js + info("Setting up private tab"); + tab2 = gBrowser.addTab(); + yield promiseBrowserLoaded(tab2.linkedBrowser); + let loadContext = tab2.linkedBrowser.docShell.QueryInterface(Ci.nsILoadContext); That unused loadContext variable would still break us in e10s but I assume you just forgot to remove that line. + ok(false, "Unexpected rrror: " + ex); + info(ex.stack); *error. I would be ok with errror as well ;)

Applied feedback.

(In reply to Tim Taubert [:ttaubert] from comment #32) > https://hg.mozilla.org/integration/fx-team/rev/c284670371cc sorry, had to back this out for causing bc failures in Windows 7 like https://tbpl.mozilla.org/php/getParsedLog.php?id=31262090&tree=Fx-Team

I believe that this has the same cause as bug 919060. A background collection is saving state to disk that is stale - from the test's point of view.

You could maybe call SessionSaver.run()? That works synchronously and should be perfect for a test as long as we still have the async background collection.

Tweaked the test suite into compliance: https://tbpl.mozilla.org/?tree=Try&rev=40c91afa78e9

The test fails locally for me. There is at least one failure on your try run as well.

I'll take a look, thanks.

Looking at the logs, it seems that SessionSaver.run() produces a sessionstore.js that is completely wrong – presumably tuned to a previous state of the session store. That's a little worrying.

Ah, got it. Race condition in the test. Tim, I extended SessionSaver.run to return a promise, to ensure that we can synchronize with the full run, including the write. Is that ok with you?

Comment on attachment 8348260 [details] [diff] [review] Don't collect private tabs, v11 Review of attachment 8348260 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/test/browser_privatetabs.js @@ +2,5 @@ > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +let Imports = {}; > +Cu.import("resource://gre/modules/Task.jsm", Imports); > +Cu.import("resource://gre/modules/Promise.jsm", Imports); You don't seem to use Task.jsm or Promise.jsm here explicitly. @@ +26,5 @@ > + > + info("Setting up private tab"); > + tab2 = gBrowser.addTab(); > + yield promiseBrowserLoaded(tab2.linkedBrowser); > + setUsePrivateBrowsing(tab2.linkedBrowser, true); This should be yield setUsePrivateBrowsing() @@ +35,5 @@ > + SyncHandlers.get(tab2.linkedBrowser).flush(); > + > + info("Checking out state"); > + yield SessionSaver.run(); > + let state = new TextDecoder().decode((yield OS.File.read(OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js")))); Nit: can you please split this into multiple lines to make it more readable? ::: browser/components/sessionstore/test/content.js @@ +54,5 @@ > +addMessageListener("ss-test:setUsePrivateBrowsing", function (msg) { > + let loadContext = > + docShell.QueryInterface(Ci.nsILoadContext); > + loadContext.usePrivateBrowsing = msg.data; > + sendSyncMessage("ss-test:setAuthorStyleDisabled"); Please use sendAsyncMessage() and send the right message name.

(In reply to Tim Taubert [:ttaubert] from comment #41) > ::: browser/components/sessionstore/test/content.js > @@ +54,5 @@ > > +addMessageListener("ss-test:setUsePrivateBrowsing", function (msg) { > > + let loadContext = > > + docShell.QueryInterface(Ci.nsILoadContext); > > + loadContext.usePrivateBrowsing = msg.data; > > + sendSyncMessage("ss-test:setAuthorStyleDisabled"); > > Please use sendAsyncMessage() and send the right message name. Good catch. It's a little worrying that this worked, though.

(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #42) > Good catch. It's a little worrying that this worked, though. The load started before you set the docShell to private but by the time the load finished the frame script already received the message. So not too worrisome :)

Applied feedback. Try: https://tbpl.mozilla.org/?tree=Try&rev=8da759304ba3

Pushed a follow-up to fix mochitest-bc perma-orange: https://hg.mozilla.org/integration/fx-team/rev/638d0be07a5f

https://hg.mozilla.org/mozilla-central/rev/7289c4384fc7 https://hg.mozilla.org/mozilla-central/rev/638d0be07a5f

Hmmm, doesn't work for me sometimes. 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/");

(In reply to Infocatcher from comment #48) > Hmmm, doesn't work for me sometimes. Can you please file a new bug for this issue? I assume that the docShell is made private before the frame script is able to register its listener. An easy fix would be to check whether the docShell is already private in PrivacyListener.init() and call MessageQueue.push() if so.

(In reply to Tim Taubert [:ttaubert] from comment #49) > Can you please file a new bug for this issue? Done: https://bugzilla.mozilla.org/show_bug.cgi?id=956826