The default bug view has changed. See this FAQ.

Starting Firefox with -private flag incorrectly claims you are not in Private Browsing mode

VERIFIED FIXED in Firefox 19

Status

()

Firefox
Private Browsing
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Ori Avtalion (salty-horse), Assigned: Ehsan)

Tracking

({relnote})

Trunk
Firefox 19
All
Linux
relnote
Points:
---
Dependency tree / graph
Bug Flags:
in-qa-testsuite ?
in-testsuite -

Firefox Tracking Flags

(firefox18+ wontfix, firefox19+ verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Hmm, my initial reaction is that this might be fixed by bug 722983.  But I need to test to make sure...
Blocks: 463027
Depends on: 722983
(Assignee)

Comment 2

5 years ago
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
Assignee: nobody → ehsan
(Assignee)

Comment 3

5 years ago
Similar to bug 722983, Firefox 18 material.
status-firefox18: --- → affected
status-firefox19: --- → affected
tracking-firefox18: --- → ?
tracking-firefox19: --- → +
(Assignee)

Comment 4

5 years ago
Created attachment 672126 [details] [diff] [review]
Patch

Very low risk (to be applied on top of the patch in bug 722983)
Attachment #672126 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/9343aa86ae7e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox19: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19

Updated

5 years ago
tracking-firefox18: ? → +
(Assignee)

Comment 6

5 years ago
I was confused here as well, this does not affect Aurora.
status-firefox18: affected → ---
tracking-firefox18: + → ---
(Assignee)

Updated

5 years ago
Attachment #672126 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 7

5 years ago
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?
(Assignee)

Comment 8

5 years ago
Hmm, no it's not.  I need to look into this more.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

5 years ago
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.
status-firefox18: --- → affected
tracking-firefox18: --- → ?
(Assignee)

Comment 10

5 years ago
Created attachment 676727 [details] [diff] [review]
Patch

Requesting review from Gavin on nsBrowserContentHandler and from Josh on the rest.
Attachment #672126 - Attachment is obsolete: true
Attachment #676727 - Flags: review?(josh)
Attachment #676727 - Flags: review?(gavin.sharp)
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/
Attachment #676727 - Flags: review?(josh) → review+

Updated

5 years ago
tracking-firefox18: ? → +
(Assignee)

Comment 12

5 years ago
(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?
Note the context - there does not appear to be any code relating to command line flags nearby, only a query of a PrivateBrowsingUtils getter.
(Assignee)

Comment 14

5 years ago
(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.
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?
(Assignee)

Comment 16

5 years ago
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 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).
Attachment #676727 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 18

4 years ago
(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.
(Assignee)

Comment 19

4 years ago
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.
(Assignee)

Comment 20

4 years ago
Comment on attachment 679264 [details] [diff] [review]
Part 1

https://hg.mozilla.org/integration/mozilla-inbound/rev/76f6e27d3a71
Attachment #679264 - Flags: checkin+
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
(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).
(Assignee)

Comment 22

4 years ago
(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).
https://hg.mozilla.org/mozilla-central/rev/76f6e27d3a71
(Assignee)

Comment 24

4 years ago
Created attachment 679845 [details] [diff] [review]
Part 2
Attachment #676727 - Attachment is obsolete: true
Attachment #679845 - Flags: review?(gavin.sharp)
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!
Attachment #679845 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 26

4 years ago
(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!
(Assignee)

Comment 27

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d4abb8e1419
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/4d4abb8e1419
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 29

4 years ago
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
Attachment #681112 - Flags: approval-mozilla-aurora?
(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?
(Assignee)

Comment 31

4 years ago
(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.
(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.
Flags: needinfo?(asa)

Comment 33

4 years ago
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.
Flags: needinfo?(asa)
(Reporter)

Comment 34

4 years ago
(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.
(Assignee)

Comment 35

4 years ago
(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...
(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.

Updated

4 years ago
Attachment #681112 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

4 years ago
status-firefox18: affected → wontfix
Keywords: relnote
Is this something we can test in-testsuite?
Flags: in-testsuite?
(Assignee)

Comment 38

4 years ago
No.
Flags: in-testsuite? → in-testsuite-
(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?
Flags: in-qa-testsuite?(hskupin)
(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.
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
Status: RESOLVED → VERIFIED
status-firefox19: fixed → verified
Removing my name from in-qa-testsuite flag for a better query.
Flags: in-qa-testsuite?(hskupin) → in-qa-testsuite?
You need to log in before you can comment on or make changes to this bug.