Privacy pref for "Always on" Private Browsing Mode

VERIFIED FIXED in Firefox 3.1b2

Status

()

defect
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: johnath, Assigned: Ehsan)

Tracking

({verified1.9.1})

Trunk
Firefox 3.1b2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments)

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. :)
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...
OS: Mac OS X → All
Hardware: PC → All
Why session store?  If we stick stuff early enough in startup, it'll be before session store inits, and it should just work.
Posted patch Patch (v1)Splinter Review
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.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #344838 - Flags: review?(mconnor)
Depends on: 461747
Depends on: 461748
Depends on: 461749
Depends on: 461750
No longer depends on: 461747, 461748, 461749, 461750
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.
(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 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
Attachment #344838 - Flags: review?(mconnor) → review-
Posted patch Patch (v2)Splinter Review
Attachment #345457 - Flags: review?
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!  :-)
Attachment #345457 - Flags: review? → review?(mconnor)
Comment on attachment 345457 [details] [diff] [review]
Patch (v2)

that looks a lot better. :)
Attachment #345457 - Flags: review?(mconnor) → review+
No longer depends on: PrivateBrowsing
Whiteboard: [pb-ready-for-landing]
http://hg.mozilla.org/mozilla-central/rev/a48936e198bf
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [pb-ready-for-landing]
Target Milestone: --- → Firefox 3.1b2
Duplicate of this bug: 463006
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.
Status: RESOLVED → VERIFIED
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?
Keywords: verified1.9.1
(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?
(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?
(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...
Duplicate of this bug: 487868
Ehsan: Is it expected that the (Private Browsing) text does not show next to the page title when you automatically flip this preference?
(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
Marcia: yes, as Kurt mentioned, this is by design.
This appears to have caused bug 497578
Depends on: 497578
You need to log in before you can comment on or make changes to this bug.