Last Comment Bug 802274 - Starting Firefox with -private flag incorrectly claims you are not in Private Browsing mode
: Starting Firefox with -private flag incorrectly claims you are not in Private...
Status: VERIFIED FIXED
: relnote
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: Firefox 19
Assigned To: :Ehsan Akhgari
:
:
Mentors:
Depends on: 722983
Blocks: PBnGen
  Show dependency treegraph
 
Reported: 2012-10-16 11:27 PDT by Ori Avtalion (salty-horse)
Modified: 2014-08-29 15:27 PDT (History)
10 users (show)
anthony.s.hughes: in‑qa‑testsuite?
ehsan: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
verified


Attachments
Patch (686 bytes, patch)
2012-10-16 18:43 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (10.43 KB, patch)
2012-10-30 13:01 PDT, :Ehsan Akhgari
gavin.sharp: review-
josh: review+
Details | Diff | Splinter Review
Part 1 (2.68 KB, patch)
2012-11-07 11:06 PST, :Ehsan Akhgari
ehsan: checkin+
Details | Diff | Splinter Review
Part 2 (6.35 KB, patch)
2012-11-08 14:55 PST, :Ehsan Akhgari
gavin.sharp: review+
Details | Diff | Splinter Review
Aurora patch (8.92 KB, patch)
2012-11-13 10:33 PST, :Ehsan Akhgari
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description Ori Avtalion (salty-horse) 2012-10-16 11:27:02 PDT
Starting Firefox with the -private flag shows a tab with this message:
 
> Would you like to start Private Browsing?
> Nightly is not currently in Private Browsing mode.

This is likely to be incorrect. Private Browsing mode is on according to my cursory tests: History is not saved, the "Tools" menu has a "Stop Private Browsing" option, greyed out since the -private flag was used.

Instead of the "Would you like to start ..." question tab, the regular "You are now in private browsing mode" tab should be displayed.

This appears to be a regression from bug 799526, which introduced the "Would you like to start Private Browsing" message.
Comment 1 :Ehsan Akhgari 2012-10-16 11:30:16 PDT
Hmm, my initial reaction is that this might be fixed by bug 722983.  But I need to test to make sure...
Comment 2 :Ehsan Akhgari 2012-10-16 18:42:00 PDT
OK this was very easy to fix on top of bug 722983.  Josh reviewed this over my shoulder.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9343aa86ae7e
Comment 3 :Ehsan Akhgari 2012-10-16 18:42:48 PDT
Similar to bug 722983, Firefox 18 material.
Comment 4 :Ehsan Akhgari 2012-10-16 18:43:39 PDT
Created attachment 672126 [details] [diff] [review]
Patch

Very low risk (to be applied on top of the patch in bug 722983)
Comment 6 :Ehsan Akhgari 2012-10-18 06:58:17 PDT
I was confused here as well, this does not affect Aurora.
Comment 7 Ori Avtalion (salty-horse) 2012-10-22 02:56:04 PDT
Now the "Would you like to start Private Browsing" tab no longer appears, but the "You are now in Private Browsing mode" tab also does not appear.

Is this the desired behavior?
Comment 8 :Ehsan Akhgari 2012-10-22 11:14:12 PDT
Hmm, no it's not.  I need to look into this more.
Comment 9 :Ehsan Akhgari 2012-10-30 13:01:05 PDT
So this entire setup based on a pref will not work in per-window PB mode.  The reason is that prefs are persistent, and we don't want launching ./firefox -private once to set a pref which will be used in later sessions.  I was not thinking enough about -private apparently.  :(

So what we need to do is to store this value in memory as opposed to in the pref, and then modify the browser content handler logic in order to display about:privatebrowsing on the first window opened in PB mode.  I have a patch which does that.

Also, I tested and Aurora is affected by this too, so we need to backport this patch there as well.
Comment 10 :Ehsan Akhgari 2012-10-30 13:01:59 PDT
Created attachment 676727 [details] [diff] [review]
Patch

Requesting review from Gavin on nsBrowserContentHandler and from Josh on the rest.
Comment 11 Josh Matthews [:jdm] 2012-10-30 14:52:42 PDT
Comment on attachment 676727 [details] [diff] [review]
Patch

Review of attachment 676727 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserContentHandler.js
@@ +684,5 @@
>        catch (e) {
>        }
> +
> +      // The global PB Service consumes this flag, so only eat it in per-window
> +      // PB builds.

This comment doesn't really make sense.

::: toolkit/content/PrivateBrowsingUtils.jsm
@@ +40,5 @@
>      }
>  #endif
> +  },
> +
> +  // These should only be used form nsBrowserContentHandler

nit: s/form/from/
Comment 12 :Ehsan Akhgari 2012-10-30 16:06:45 PDT
(In reply to Josh Matthews [:jdm] from comment #11)
> Comment on attachment 676727 [details] [diff] [review]
> Patch
> 
> Review of attachment 676727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/nsBrowserContentHandler.js
> @@ +684,5 @@
> >        catch (e) {
> >        }
> > +
> > +      // The global PB Service consumes this flag, so only eat it in per-window
> > +      // PB builds.
> 
> This comment doesn't really make sense.

Why is that?
Comment 13 Josh Matthews [:jdm] 2012-10-30 16:09:49 PDT
Note the context - there does not appear to be any code relating to command line flags nearby, only a query of a PrivateBrowsingUtils getter.
Comment 14 :Ehsan Akhgari 2012-10-30 16:54:00 PDT
(In reply to Josh Matthews [:jdm] from comment #13)
> Note the context - there does not appear to be any code relating to command
> line flags nearby, only a query of a PrivateBrowsingUtils getter.

Oh, right, sorry, I was looking at the first instance of this comment.  The second instance is probably just an incorrect paste.  I'll take it out before landing.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-31 16:15:21 PDT
Ugh, making this already code grosser to handle the private browsing special case would be really unfortunate. Do we really need to change the page shown on startup this way? Aren't the other indications of PB mode sufficient?
Comment 16 :Ehsan Akhgari 2012-10-31 16:19:09 PDT
The other indications of private browsing mode are very subtle, which is the reason why we showed this page when Firefox would start up with -private before.  :/
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-06 11:36:51 PST
Comment on attachment 676727 [details] [diff] [review]
Patch

I really don't want to complicate this needHomePageOverride code - we should be able to handle this more in the front-end (browser.js).
Comment 18 :Ehsan Akhgari 2012-11-06 12:54:59 PST
(In reply to comment #17)
> Comment on attachment 676727 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=676727
> Patch
> 
> I really don't want to complicate this needHomePageOverride code - we should be
> able to handle this more in the front-end (browser.js).

Do you have any suggestion on where I would add that code?  I don't think that we currently have any specific handling for the home page override in browser.js.  I can go ahead and hack my way into this, but I wanna make sure that I'm not overlooking an existing facility for this.
Comment 19 :Ehsan Akhgari 2012-11-07 11:06:53 PST
Created attachment 679264 [details] [diff] [review]
Part 1

I'm going to land the PBUtils part of this patch separately because it fixes a bunch of other stuff too.
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-07 12:16:17 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #18)
> Do you have any suggestion on where I would add that code?

needHomePageOverride controls what eventually ends up as uriToLoad (window.arguments[0]) in browser.js. The tricky part is knowing what kind of window open this is in that context - which you generally do from the nsBrowserContentHandler code. Maybe nsBrowserContentHandler is the right place, but I'd want to minimize interaction with needHomepageOverride as much as possible (see bug 699573).
Comment 22 :Ehsan Akhgari 2012-11-07 13:10:24 PST
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #21)
> (In reply to Ehsan Akhgari [:ehsan] from comment #18)
> > Do you have any suggestion on where I would add that code?
> 
> needHomePageOverride controls what eventually ends up as uriToLoad
> (window.arguments[0]) in browser.js. The tricky part is knowing what kind of
> window open this is in that context - which you generally do from the
> nsBrowserContentHandler code. Maybe nsBrowserContentHandler is the right
> place, but I'd want to minimize interaction with needHomepageOverride as
> much as possible (see bug 699573).

Hmm, I see.  What if I do the first window check in the defaultArgs getter itself?  That should work fine as well.  If you think that's a good idea, I can update the patch pretty easily.

(Really, I think nsBrowserContentHandler is a much better place for this detection code to live than browser.js).
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-11-07 17:30:22 PST
https://hg.mozilla.org/mozilla-central/rev/76f6e27d3a71
Comment 24 :Ehsan Akhgari 2012-11-08 14:55:51 PST
Created attachment 679845 [details] [diff] [review]
Part 2
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-11 11:03:33 PST
Comment on attachment 679845 [details] [diff] [review]
Part 2

>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js

>   get defaultArgs() {

>+    if (!gFirstWindow) {
>+      gFirstWindow = true;
>+      if (PrivateBrowsingUtils.isInTemporaryAutoStartMode) {
>+        return "about:privatebrowsing";
>+      }
>+    }

This is a little fragile, in that there's no garantee that the first caller of this getter will be used for the first window, but it's no more fragile than our existing needHomePageOverride code, so if that ends up breaking at some point I guess we'd notice. I'd feel better if there was an assertion or test of some sort, but I can't really figure out how you'd write one given the various callers of defaultArgs. I've been meaning to clean this up for a long time...

>+      // The global PB Service consumes this flag, so only eat it in per-window
>+      // PB builds.
>+      if (PrivateBrowsingUtils.isInTemporaryAutoStartMode) {
>+        this.mFeatures = ",private";

This comment doesn't make any sense here. I don't really know what the "private" feature flag does, or what "TemporaryAutoStartMode" is supposed to be, so hopefully you don't need my review on this part.

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

Similarly I don't fully understand the logic changes here, but I trust someone with more PB knowledge (Josh?) can review that. r=me on the part that I originally r-ed for!
Comment 26 :Ehsan Akhgari 2012-11-12 09:24:35 PST
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #25)
> Comment on attachment 679845 [details] [diff] [review]
> Part 2
> 
> >diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js
> 
> >   get defaultArgs() {
> 
> >+    if (!gFirstWindow) {
> >+      gFirstWindow = true;
> >+      if (PrivateBrowsingUtils.isInTemporaryAutoStartMode) {
> >+        return "about:privatebrowsing";
> >+      }
> >+    }
> 
> This is a little fragile, in that there's no garantee that the first caller
> of this getter will be used for the first window, but it's no more fragile
> than our existing needHomePageOverride code, so if that ends up breaking at
> some point I guess we'd notice. I'd feel better if there was an assertion or
> test of some sort, but I can't really figure out how you'd write one given
> the various callers of defaultArgs. I've been meaning to clean this up for a
> long time...

Yeah, this is not great, I agree.  I can't think of how we'd test this either, since we can't run tests on the startup sequence.  :(

> >+      // The global PB Service consumes this flag, so only eat it in per-window
> >+      // PB builds.
> >+      if (PrivateBrowsingUtils.isInTemporaryAutoStartMode) {
> >+        this.mFeatures = ",private";
> 
> This comment doesn't make any sense here. I don't really know what the
> "private" feature flag does, or what "TemporaryAutoStartMode" is supposed to
> be, so hopefully you don't need my review on this part.

The -private flag puts Firefox in autostart PB mode, regardless of the value of the browser.privatebrowsing.autostart pref.  This comment will probably need adjusting when we remove the code for the global PB mode.

> >diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
> 
> Similarly I don't fully understand the logic changes here, but I trust
> someone with more PB knowledge (Josh?) can review that. r=me on the part
> that I originally r-ed for!

Yeah, Josh reviewed these parts.

Thanks!
Comment 28 Phil Ringnalda (:philor) 2012-11-12 21:20:38 PST
https://hg.mozilla.org/mozilla-central/rev/4d4abb8e1419
Comment 29 :Ehsan Akhgari 2012-11-13 10:33:29 PST
Created attachment 681112 [details] [diff] [review]
Aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 722983 and others
User impact if declined: see comment 0
Testing completed (on m-c, etc.): locally/m-c
Risk to taking this patch (and alternatives if risky): This is somewhat risky, but it's not clear how we can avoid this by backing stuff out without running the risk of breaking other things, so I recommend that we take this on Aurora soon and deal with any possible regressions throughout the beta cycle.
String or UUID changes made by this patch: none
Comment 30 Alex Keybl [:akeybl] 2012-11-13 15:08:05 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #29)
> Risk to taking this patch (and alternatives if risky): This is somewhat
> risky, but it's not clear how we can avoid this by backing stuff out without
> running the risk of breaking other things, so I recommend that we take this
> on Aurora soon and deal with any possible regressions throughout the beta
> cycle.

Is there any impact to temporarily disabling -private instead of uplifting this risky patch? Or is there more user impact outside of the command line?
Comment 31 :Ehsan Akhgari 2012-11-13 15:27:05 PST
(In reply to comment #30)
> Is there any impact to temporarily disabling -private instead of uplifting this
> risky patch? Or is there more user impact outside of the command line?

Yes, that would mean that users who rely on the -private command line argument will not be protected any more, which is pretty bad.
Comment 32 Alex Keybl [:akeybl] 2012-11-14 11:36:23 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #31)
> (In reply to comment #30)
> > Is there any impact to temporarily disabling -private instead of uplifting this
> > risky patch? Or is there more user impact outside of the command line?
> 
> Yes, that would mean that users who rely on the -private command line
> argument will not be protected any more, which is pretty bad.

I'm questioning the proposal to take a change that's risky to all PB users, for a very small % of users who use the command-line option. I'd rather just deprecate that command-line feature (throw when -private is specified) or wontfix (PB is on but isn't reported as such), at least temporarily. Perhaps Asa can speak on behalf of the user benefit here.
Comment 33 Asa Dotzler [:asa] 2012-11-14 11:43:16 PST
because this is a command line only mode and we're doing the right thing even if we're labeling it incorrectly, I'd rather we just release note it and avoid additional risk.
Comment 34 Ori Avtalion (salty-horse) 2012-11-14 11:58:57 PST
(In reply to Asa Dotzler [:asa] from comment #33)
> because this is a command line only mode and we're doing the right thing
> even if we're labeling it incorrectly, I'd rather we just release note it
> and avoid additional risk.

It's no longer "labeled incorrectly" (see my Comment 7). It's just unlabeled - there is no tab with a big message about being in private browsing.
Comment 35 :Ehsan Akhgari 2012-11-14 12:08:57 PST
(In reply to comment #34)
> (In reply to Asa Dotzler [:asa] from comment #33)
> > because this is a command line only mode and we're doing the right thing
> > even if we're labeling it incorrectly, I'd rather we just release note it
> > and avoid additional risk.
> 
> It's no longer "labeled incorrectly" (see my Comment 7). It's just unlabeled -
> there is no tab with a big message about being in private browsing.

Yeah, and I'm not convinced if it's a good idea to ship Firefox 18 with that, as it might make it seem like -private no longer works...
Comment 36 Alex Keybl [:akeybl] 2012-11-14 15:43:29 PST
(In reply to Ehsan Akhgari [:ehsan] from comment #35)
> Yeah, and I'm not convinced if it's a good idea to ship Firefox 18 with
> that, as it might make it seem like -private no longer works...

That's why we'd like to release note this, as opposed to taking a fix that could regress non-CL PB Firefox users and cause us to chemspill post-release.
Comment 37 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-01-03 12:08:33 PST
Is this something we can test in-testsuite?
Comment 38 :Ehsan Akhgari 2013-01-03 15:24:16 PST
No.
Comment 39 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-01-03 15:36:27 PST
(In reply to :Ehsan Akhgari from comment #38)
>> (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #37)
>> Is this something we can test in-testsuite?
>
> No.

Henrik, do you think this is something we can automate in Mozmill?
Comment 40 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2013-01-03 22:39:21 PST
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #39)
> Henrik, do you think this is something we can automate in Mozmill?

Not that easily yet given that we do not support starting Firefox with different cmd line options, but please file a bug for it.
Comment 41 Cornel Ionce [QA] (:cornel_ionce) 2013-02-12 04:35:45 PST
Verified as fixed, using Firefox 19 beta 5 on Ubuntu 12.04.

User Agent: Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130206083616
Comment 42 Henrik Skupin (:whimboo) [away 09/30 - 10/06] 2014-08-29 15:27:18 PDT
Removing my name from in-qa-testsuite flag for a better query.

Note You need to log in before you can comment on or make changes to this bug.