460346 - Privacy pref for "Always on" Private Browsing Mode

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Johnathan Nightingale [:johnath]]

In discussions with faaborg and mconnor today, the idea came up of having a pref that basically puts Firefox in "always-private" mode - rather than having to set it explicitly each time.

Whether we do this or not, we need a bug for the discussion. :)

Comment 1

[(no longer active)]

Actually adding an about:config pref to that end has come up on bug 248970, and it's currently in my todo list <https://wiki.mozilla.org/User:Ehsan/PrivateBrowsingTodo>.  Exposing this to the user is another point, though.  :-)

The code for implementing this pref should be fairly trivial, it might just require some fiddling with the session store component...

Comment 2

[Mike Connor [:mconnor]]

Why session store?  If we stick stuff early enough in startup, it'll be before session store inits, and it should just work.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

Here's the patch to implement this behavior.

Setting browser.privatebrowsing.autostart to true causes the browser to start into the private browsing mode each time it starts up.  I also made sure to initialize it before session store, so that it doesn't interfere with anything that this service does.

The patch includes a unit test.

Comment 4

[(no longer active)]

Try server builds available here: <https://build.mozilla.org/tryserver-builds/2008-10-26_16:[e-mail]-try-2ddbcd278de/>

Comment 5

[Marcia Knous [:marcia]]

Ehsan: I have tested your tryserver build on my Mac. After setting the pref I purposely crashed the browser as well as exited abnormally and the pref sticks upon restart, as well as when you restart normally. I did see Error: uncaught exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)"  location: "http://i.cdn.turner.com/cnn/.element/js/2.0/StorageManager.js Line: 165"] in the error console, but there is already Bug 389002 filed regarding that.

Comment 6

[(no longer active)]

(In reply to comment #5)
> Ehsan: I have tested your tryserver build on my Mac. After setting the pref I
> purposely crashed the browser as well as exited abnormally and the pref sticks
> upon restart, as well as when you restart normally. I did see Error: uncaught
> exception: [Exception... "Security error"  code: "1000" nsresult: "0x805303e8
> (NS_ERROR_DOM_SECURITY_ERR)"  location:
> "http://i.cdn.turner.com/cnn/.element/js/2.0/StorageManager.js Line: 165"] in
> the error console, but there is already Bug 389002 filed regarding that.

Great, thanks for testing this.

The error message might be due to the fact that we disable DOM storage inside the private browsing mode, so any call on it raises the NS_ERROR_DOM_SECURITY_ERR exception, which, if not handled by the website code, is shown in the console.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 344838 [details] [diff] [review]
Patch (v1)


> function PrivateBrowsingService() {
>-  this._obs.addObserver(this, "quit-application-granted", false);
>+  this._obs.addObserver(this, "prefservice:before-read-userprefs", true);
>+  this._obs.addObserver(this, "quit-application-granted", true);

I was going to smack you in the other review, but I saw this was here.  I don't think its in scope for _this_ bug, but s'ok

>   _onPrivateBrowsingModeChanged: function PBS__onPrivateBrowsingModeChanged() {
>-    // clear all auth tokens
>-    let sdr = Cc["@mozilla.org/security/sdr;1"].
>-              getService(Ci.nsISecretDecoderRing);
>-    sdr.logoutAndTeardown();
>+    // nothing needs to be done here if we're auto-starting


next time, use diff -w if you're doing something like this!


>@@ -185,20 +192,38 @@ PrivateBrowsingService.prototype = {
>   _canLeavePrivateBrowsingMode: function PBS__canLeavePrivateBrowsingMode() {
>     let cancelLeave = Cc["@mozilla.org/supports-PRBool;1"].
>                       createInstance(Ci.nsISupportsPRBool);
>     cancelLeave.data = false;
>     this._obs.notifyObservers(cancelLeave, "private-browsing-cancel-vote", "exit");
>     return !cancelLeave.data;
>   },
> 
>+  _shouldAutoStart: function PBS__shouldAutoStart() {
>+    let prefsService = Cc["@mozilla.org/preferences-service;1"].
>+                       getService(Ci.nsIPrefBranch);
>+    this._autoStart = prefsService.getBoolPref("browser.privatebrowsing.autostart");
>+    return this._autoStart;
>+  },

this is only used once, it doesn't need to be a function.

>   // nsIObserver
> 
>   observe: function PBS_observe(aSubject, aTopic, aData) {
>     switch (aTopic) {
>+      case "prefservice:before-read-userprefs":
>+        // If the autostart prefs has been set, simulate entering the
>+        // private browsing mode upon startup.
>+        // This won't interfere with the session store component, because
>+        // that component will be initialized on final-ui-startup.
>+        if (this._shouldAutoStart()) {
>+          this.privateBrowsingEnabled = true;
>+          this._autoStart = false;
>+        }
>+        this._obs.removeObserver(this, "prefservice:before-read-userprefs");
>+        break;

How is this supposed to work, exactly?  This is before user prefs are read...

>+// This test copies the prefs file to the appropriate location, to be read by
>+// test_privatebrowsing_autostart_main.js.
>+
>+function run_test() {
>+  var file = do_get_file("browser/components/privatebrowsing/test/unit/pb_test_pref_autostart.js");
>+  var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
>+               getService(Ci.nsIProperties);
>+  var dest = dirSvc.get("CurProcD", Ci.nsILocalFile);
>+  dest.append("defaults");
>+  dest.append("pref");
>+  do_check_true(dest.exists());
>+  do_check_true(dest.isDirectory());
>+  file.copyTo(dest, "0_pb_test_pref_autostart.js");
>+}

this adds it as an extra default pref ... but doesn't actually test what happens when its set as a user pref.  I can't see how the user pref would ever get read in time here, so this patch is kinda broken as far as I can see.

if you observe profile-after-change that's still before final-ui-startup and thus should beat SS to init

Comment 8

[(no longer active)]

Attachment: Patch (v2)

Comment 9

[(no longer active)]

Comment on attachment 345457 [details] [diff] [review]
Patch (v2)

Oops, hit enter too soon, and escape too late!

Anyway, this is the new version of the patch using profile-after-change, with a unit test which doesn't hurt your head!  :-)

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 345457 [details] [diff] [review]
Patch (v2)

that looks a lot better. :)

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a48936e198bf

Comment 13

[Marcia Knous [:marcia]]

verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre,  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre, and  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081104 Minefield/3.1b2pre.

When this pref is flipped, the "Private Browsing" text is greyed out and cannot be turned on/off.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ehsan, why I'm forced to stay in PB mode when having this pref set to true? "browser.privatebrowsing.autostart" tells me that the PB mode is started automatically on start-up but not that I can't leave it anymore during this session. Is this really an expected behavior? If yes, shouldn't we better find a better pref name?

Comment 15

[(no longer active)]

(In reply to comment #14)
> Ehsan, why I'm forced to stay in PB mode when having this pref set to true?
> "browser.privatebrowsing.autostart" tells me that the PB mode is started
> automatically on start-up but not that I can't leave it anymore during this
> session. Is this really an expected behavior? If yes, shouldn't we better find
> a better pref name?

Yes, this is by design.  This is a mode designed for cases where you want the browser to operate in PB mode all the time.

The "autostart" pref name is only half of the story here, I agree.  Can you think of a better name?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #15)
> Yes, this is by design.  This is a mode designed for cases where you want the
> browser to operate in PB mode all the time.

Were there also some needs pointed out to start the browser in PB mode automatically but handing-over the choice to leave the PB mode to the user? I was getting asked about that yesterday. Probably that could be reached by a command line argument. 

> The "autostart" pref name is only half of the story here, I agree.  Can you
> think of a better name?

As what the summary of this bug already states: browser.privatebrowsing.alwaysOn?

Comment 17

[(no longer active)]

(In reply to comment #16)
> Were there also some needs pointed out to start the browser in PB mode
> automatically but handing-over the choice to leave the PB mode to the user? I
> was getting asked about that yesterday. Probably that could be reached by a
> command line argument. 

Can you file a bug on that please?

> As what the summary of this bug already states:
> browser.privatebrowsing.alwaysOn?

Sounds logical.  However, we're past feature freeze, and I'm not sure if we can rename the prefs right now.  Perhaps it'd be best to file a bug on this and mark it wanted-firefox3.1 to see what we can do about it.  If we release 3.1 with this pref name, I really don't like changing it for 3.2...

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Done. Filed bug 471997 and bug 471998.

Comment 20

[Marcia Knous [:marcia]]

Ehsan: Is it expected that the (Private Browsing) text does not show next to the page title when you automatically flip this preference?

Comment 21

[u88484]

(In reply to comment #20)
> Ehsan: Is it expected that the (Private Browsing) text does not show next to
> the page title when you automatically flip this preference?
Bug 481785 in the dependency list

Comment 22

[(no longer active)]

Marcia: yes, as Kurt mentioned, this is by design.

Comment 23

[Robert Strong (they/them - no direct email)]

This appears to have caused bug 497578