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.

Hmm, my initial reaction is that this might be fixed by bug 722983. But I need to test to make sure...

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

Similar to bug 722983, Firefox 18 material.

Very low risk (to be applied on top of the patch in bug 722983)

I was confused here as well, this does not affect Aurora.

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?

Hmm, no it's not. I need to look into this more.

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.

Requesting review from Gavin on nsBrowserContentHandler and from Josh on the rest.

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/

(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.

(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?

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).

(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.

I'm going to land the PBUtils part of this patch separately because it fixes a bunch of other stuff too.

Comment on attachment 679264 [details] [diff] [review] Part 1 https://hg.mozilla.org/integration/mozilla-inbound/rev/76f6e27d3a71

(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).

(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 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!

(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!

[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

(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?

(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.

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.

(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.

(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.

Is this something we can test in-testsuite?

No.

(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?

(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

Removing my name from in-qa-testsuite flag for a better query.