601253 - Ignore -private-toggle on the command line if permanent private browsing is on

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[(no longer active)]

We currently toggle the PB mode if you specify -private-toggle on the command line.  This shouldn't happen in permanent PB mode.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Justin Dolske [:Dolske]]

Hmm, instead of just ignoring the flag, should we throw (per nsICommandLineHandler.idl) so that startup aborts and the app quits?

[I guess it's not a big  deal, though. Starting in PB mode when you were expecting non-PB mode isn't a big deal. Obviously this would be different if the situation was about starting in non-PB mode when the user expected PB mode.]

Comment 3

[(no longer active)]

(In reply to comment #2)
> Hmm, instead of just ignoring the flag, should we throw (per
> nsICommandLineHandler.idl) so that startup aborts and the app quits?

Hmm, I'm not sure if that's needed.  The app will quit anyways, right?  However, if you want, I can definitely make that change...

Comment 4

[Justin Dolske [:Dolske]]

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

Yeah, worth throwing I think since it's a nonsensical commandline, according to how permanent PB mode works.

Looks fine otherwise, but I'm going to bounce over to gavin since I've not really familiar with the command line stuff. Why does the existing code have both an observer and nsICommandLineHandler? Does one get called for the initial app launch, and the other for remote invocations or something like that?

Comment 5

[(no longer active)]

(In reply to comment #4)
> Comment on attachment 480306 [details] [diff] [review]
> Patch (v1)
> 
> Yeah, worth throwing I think since it's a nonsensical commandline, according to
> how permanent PB mode works.

OK, will submit a new patch with that change.

> Looks fine otherwise, but I'm going to bounce over to gavin since I've not
> really familiar with the command line stuff. Why does the existing code have
> both an observer and nsICommandLineHandler? Does one get called for the initial
> app launch, and the other for remote invocations or something like that?

"command-line-startup" is fired before "final-ui-startup".  After that notification, it's too late for us to initialize permanent PB mode.

Comment 6

[(no longer active)]

Attachment: Patch (v2)

Comment 7

[:Gavin Sharp [email: [e-mail]]]

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

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js

>       case "command-line-startup":
>         this._obs.removeObserver(this, "command-line-startup");
>         aSubject.QueryInterface(Ci.nsICommandLine);
>         if (aSubject.findFlag("private", false) >= 0) {
>           this.privateBrowsingEnabled = true;
>           this._autoStarted = true;
>           this._lastChangedByCommandLine = true;
>         }
>-        else if (aSubject.findFlag("private-toggle", false) >= 0) {
>+        else if (aSubject.findFlag("private-toggle", false) >= 0 &&
>+                 !this._autoStarted) {
>           this._lastChangedByCommandLine = true;
>         }

I don't really understand the reasoning behind this change. If the idea is to avoid setting lastChangedByCommandLine if we're already in PB mode due to the pref being set, then why not also do this for the -private flag?

Comment 8

[(no longer active)]

(In reply to comment #7)
> Comment on attachment 482625 [details] [diff] [review]
> Patch (v2)
> 
> >diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
> 
> >       case "command-line-startup":
> >         this._obs.removeObserver(this, "command-line-startup");
> >         aSubject.QueryInterface(Ci.nsICommandLine);
> >         if (aSubject.findFlag("private", false) >= 0) {
> >           this.privateBrowsingEnabled = true;
> >           this._autoStarted = true;
> >           this._lastChangedByCommandLine = true;
> >         }
> >-        else if (aSubject.findFlag("private-toggle", false) >= 0) {
> >+        else if (aSubject.findFlag("private-toggle", false) >= 0 &&
> >+                 !this._autoStarted) {
> >           this._lastChangedByCommandLine = true;
> >         }
> 
> I don't really understand the reasoning behind this change. If the idea is to
> avoid setting lastChangedByCommandLine if we're already in PB mode due to the
> pref being set, then why not also do this for the -private flag?

That is the idea behind this change, and the reason why we don't do this for -private is that firstly I'm not changing the behavior of -private in this bug, and secondly the -private flag is not remotable, and therefore is not subject to this problem in the first place.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

The command-line-startup case is never hit at all for remoting.

Comment 10

[(no longer active)]

(In reply to comment #9)
> The command-line-startup case is never hit at all for remoting.

Is it not?

<http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3584>

Comment 11

[:Gavin Sharp [email: [e-mail]]]

That's after we do the remoting stuff (nativeApp->Start() and RemoteCommandLine()).

Comment 12

[:Gavin Sharp [email: [e-mail]]]

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

But even ignoring all of that, the change isn't related to this bug at all, as far as I can see. Given that, and the fact that it also introduces an inconsistency between -private and -private-browsing, it seems pretty clear that it shouldn't be made here.

The other hunk in this patch looks fine, you can have r+ on just that.

Comment 13

[(no longer active)]

Attachment: Patch (v3)

Right, my bad.

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/488177edfe60