In my surora build running on a nexus4 i have set the option to clear all data on quit. All the checkboxes are selected. However when i quit Firefox and reopen it the recent tabs panel still shows my tabs from last time. This is a form of history leakage and is unexpected behavior.

This is kinda tricky. We have a separate pref for "Restore tabs" that defaults to off. Should we still restore tabs if you have that set to true, but history set to clear on exit? Should we disable that pref is "Clear on exit" is set to clear history? But that's slightly different than the 'Tabs from last time' box too. I guess we'll have to add something to the SessionStore interface to fix that. I think the right thing to do is not write this session file in that case, which will also cause the Restore tabs pref to no longer work.

This adds a method to SessionStore, removeWindow, that removes a window from its data tracking and then forces a resave of the latest database. We call it if we're shutting down and clearing history.

Comment on attachment 8473224 [details] [diff] [review] Patch Review of attachment 8473224 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Wesley Johnston (:wesj) from comment #1) > This is kinda tricky. We have a separate pref for "Restore tabs" that > defaults to off. Should we still restore tabs if you have that set to true, > but history set to clear on exit? Should we disable that pref is "Clear on > exit" is set to clear history? IMO, we should clear the history, but restore the tabs. This is basically the same thing that happens if you clear private data while Firefox is running: you don't lose your tabs themselves, but you lose everything else. Either way, I'm guessing we don't want to simply break the "Restore tabs" pref without fixing it somehow, so just giving f+ until we know how we want to handle the second half of this.

I agree with Brian. In this situation, the browser should clear the private data, bu restore the tabs. We should avoid using the word: "History" in these situations. Tabs can be considered part of the browsing history. In current Settings panel, we use "Clear Private Data", which makes sense for excluding tabs. I am also leaning towards moving tab restore setting out of "Customize" session and put it into "Privacy" session, assuming most people switch preferences because of privacy considerations. As a result, the users can see options for "Clear Private Data" and "Clear Tabs" in one place. This can potentially avoid the confusion. Thoughts?

(In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #4) > I am also leaning towards moving tab restore setting out of "Customize" > session and put it into "Privacy" session, assuming most people switch > preferences because of privacy considerations. As a result, the users can > see options for "Clear Private Data" and "Clear Tabs" in one place. This can > potentially avoid the confusion. Can you elaborate on what the setting would look like if it were moved to Privacy? Would it be the same "Tabs" setting we have now -- with the same always/don't restore options -- that's just moved into the Privacy section? Or would we add an additional checkbox to the Clear Now dialog that would allow clearing "Tabs" as one of the items?

(In reply to Brian Nicholson (:bnicholson) from comment #5) > (In reply to Yuan Wang(:Yuan) – Firefox UX Team from comment #4) > > I am also leaning towards moving tab restore setting out of "Customize" > > session and put it into "Privacy" session, assuming most people switch > > preferences because of privacy considerations. As a result, the users can > > see options for "Clear Private Data" and "Clear Tabs" in one place. This can > > potentially avoid the confusion. > > Can you elaborate on what the setting would look like if it were moved to > Privacy? Would it be the same "Tabs" setting we have now -- with the same > always/don't restore options -- that's just moved into the Privacy section? > Or would we add an additional checkbox to the Clear Now dialog that would > allow clearing "Tabs" as one of the items? "Clear Private Data" section and "Restore Tabs/Clear Tabs" would be two separate controls in the Privacy section. Nothing should change for the "Clear Private Data". For cleaning tabs, I think the format as simple as a switch with some descriptive texts. Clear tabs on exit - ON/OFF Choose whether you would like Firefox to clear your open tabs every time you exit the browser (And by default it's OFF) ---------------------- The rationales: 1. It's important to maintain the mental model that private data doesn't include tabs. It needs to be consistent across desktop and mobile Firefox. 2. Changed the wording from "restore" to "clear" for a few reasons. - "Clear" matches the other control: "Clear Private Data". And it has a stronger emphasis on privacy than restore does. - Second, "Clear" is a disruptive term, which raises people's attention. Since by default, the option is off. This will make sure the users think carefully before opting in. - Lastly, as I understand, the "restore tabs from last time" is not really a restore from user's perspective. Technically it's a restore. But the users should just expect to see the browser as the same state as they exited it last time. Fennec doesn't have or need a restore page that requires user actions as Firefox desktop. "Clear" is the feedback which the users will truly notice. ------------------------------- I can file a separate bug to make that change. And I will talk to Robin since she has been thinking about setting panel redesign on Bug 965377.

OK. Just to be clear, with this new setup, if the user sets: 1.) Clear History (on Exit) AND Restore my tabs, we'll store your data about tabs on disk when Fennec exits 2.) Clear History AND DONT Restore my tabs, we will won't store any data about your tabs when Fennec exits 3.) DONT clear history AND Restore my tabs, we'll store data about your tabs on exit 4.) DONT clear history AND DONT Restore my tabs, we'll still store data about your tabs on exit for the "Tabs from last time page". Makes sense to me. Will edit this. We should file a separate bug for moving the pref I think.

So besides the wording change, it sounds like we'll have the functional change of restore being *on* by default. The last time I tested, this was also what Chrome and the stock browser did. I've advocated this several times in the past, so I'm happy with this change. In the past, though, there's been some pushback since it will result in slower startup times; about:home appears much faster than a restoring page since we have to wait for Gecko, then the page, to load. CC'ing mfinkle since I think he was part of the opposition.

I added this as a type of data to "sanitize". I think moving the pref or changing its default is probably a separate bug.

Comment on attachment 8473511 [details] [diff] [review] Patch v2 Review of attachment 8473511 [details] [diff] [review]: ----------------------------------------------------------------- Instead of exposing restoreWindow in SessionRestore.idl and calling that from Sanitizer, have you considered sending a message from GeckoApp? That is, GeckoApp sends a "Session:Destroy" message, and SessionStore adds a handler to take care of this internally. Sanitizer.jsm wouldn't be involved, which might make more sense considering "clear tabs" isn't in the private data list. ::: mobile/android/base/GeckoApp.java @@ +461,5 @@ > + if ("quit".equals(sessionRestore)) { > + try { > + clearObj.put("private.data.sessionRestore", true); > + } catch(JSONException ex) { > + Log.i(LOGTAG, "Error adding session restore data", ex); Nit: Log.e ::: mobile/android/modules/Sanitizer.jsm @@ +259,5 @@ > return true; > } > + }, > + > + // This removes the current window from participating in session restore. Its useful Nit: Its -> It's

Brian talked me out of abusing the Sanitizer.

Comment on attachment 8476217 [details] [diff] [review] Patch Review of attachment 8476217 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +449,5 @@ > for (String clear : clearSet) { > try { > clearObj.put(clear, true); > } catch(JSONException ex) { > + Log.e(LOGTAG, "Error adding clear object " + clear ,ex); Nit: " ,ex" -> ", ex" @@ +457,5 @@ > + final JSONObject res = new JSONObject(); > + try { > + res.put("sanitize", clearObj); > + } catch(JSONException ex) { > + Log.e(LOGTAG, "Error adding sanitize object", ex); Nit: Fix indentation ::: mobile/android/chrome/content/browser.js @@ +1123,5 @@ > evt.initUIEvent("TabSelect", true, false, window, null); > aTab.browser.dispatchEvent(evt); > }, > > + quit: function quit(aClear = { sanitize: {}, dontSaveSession: true }) { Shouldn't dontSaveSession be false by default? @@ +1146,5 @@ > > + BrowserApp.sanitize(aClear.sanitize, function() { > + if (aClear.dontSaveSession) { > + let ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore); > + ss.removeWindow(window); I wonder if it would make sense to do this before the sanitize() call. sanitize() can fire "browser:purge-session-history", which iterates through all the windows and purges them. ss.removeWindow() destroys the window entirely, making the purge pointless, so it would be more efficient to call that first.

(In reply to Brian Nicholson (:bnicholson) from comment #12) > Shouldn't dontSaveSession be false by default? dontSaveSession is kind of confusing because "not not saving the session" is a double negative. Maybe rename this var to destroySession or clearSession?

Ooh. Missed the second comment. I agree its a bit strange. I originally had saveSession, but I wanted to make sure whatever we had defaulted to false in case it was, for some reason, not present: https://hg.mozilla.org/integration/fx-team/rev/176ee3cee1ca

Comment on attachment 8476217 [details] [diff] [review] Patch Approval Request Comment [Feature/regressing bug #]: 1001309 [User impact if declined]: Some data is not cleared [Describe test coverage new/current, TBPL]: Needs some. This is tough to test since it involves quitting fennec... [Risks and why]: Pretty low risk. Opt in feature. Worst case scenario, you have to manually clear history, but can hurt privacy if it fails. [String/UUID change made/needed]: none.

Comment on attachment 8476217 [details] [diff] [review] Patch FYI, this patch had a UUID change.

Needs rebasing for Aurora uplift.

Verified as fixed in builds: Firefox for Android 34.0a1 (2014-09-01) Firefox for Android 33.0a2 (2014-09-01) Device: Asus Transformer Pad TF300T (Android 4.2.1)