Francois, could you try an experiment and see if it's possible to turn on TP in PB mode by inspecting the channel in nsChannelClassifier? An easy check would be to change https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsChannelClassifier.cpp#62 to QI if the underlying channel is https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPrivateBrowsingChannel.idl, then enable TP if so and if there is a setting privacy.trackingprotection.pbmode.enabled (so that we can hotfix it off).

If you have problems with this, #necko is a good place to ask questions and jdm and ehsan are both PB mode experts.

Here's a working patch for this. This is how the new pref interacts with the old one: - If privacy.trackingprotection.enabled is OFF, it's OFF everywhere including in PB mode. - If privacy.trackingprotection.pbonly is ON, then TP is only enforced in PB windows.

Comment on attachment 8572133 [details] [diff] [review] New privacy.trackingprotection.pbonly pref Review of attachment 8572133 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsChannelClassifier.cpp @@ +73,5 @@ > } > > nsresult rv; > + if (Preferences::GetBool("privacy.trackingprotection.pbonly", false)) { > + if (!NS_UsePrivateBrowsing(aChannel)) { Awesome! I think the behavior that we want is that this should be completely independent of privacy.trackingprotection.enabled, so that users don't have to enable both, and that if we do end up tying this to PB mode we don't have to touch privacy.trackingprotection.enabled. So if (!(Preferences::GetBool("privacy.trackingprotection.enabled", false) || (Preferences.GetBool("privacy.trackingprotection.pbonly", false) && NS_UsePrivateBrowsing(aChannel))) { return NS_OK; }

Comment on attachment 8572133 [details] [diff] [review] New privacy.trackingprotection.pbonly pref Review of attachment 8572133 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsChannelClassifier.cpp @@ +74,5 @@ > > nsresult rv; > + if (Preferences::GetBool("privacy.trackingprotection.pbonly", false)) { > + if (!NS_UsePrivateBrowsing(aChannel)) { > +#if defined(PR_LOGGING) Also no need to #ifdef PR_LOGGING anymore since it's available in opt builds.

Separating this to a new pref was slightly more involved because in addition to the TP enforcement, privacy.trackingprotection.enabled also controls the list updates.

Comment on attachment 8572251 [details] [diff] [review] New independent p.tp.pbmode.enabled pref Review of attachment 8572251 [details] [diff] [review]: ----------------------------------------------------------------- I think this change is worth a unittest change. Have a look at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/tests/mochitest/test_classified_annotations.html. You will have to force the iframe to load in and out of a private window (http://mxr.mozilla.org/mozilla-central/search?string=perwindowpb&find=ini) ::: netwerk/base/nsChannelClassifier.cpp @@ +69,5 @@ > *result = false; > > if (!Preferences::GetBool("privacy.trackingprotection.enabled", false)) { > + if (Preferences::GetBool("privacy.trackingprotection.pbmode.enabled", false)) { > + if (!NS_UsePrivateBrowsing(aChannel)) { This is easier to read with fewer nested ifs if (Preferences::GetBool(pbmode) && !NS_UsePrivateBrowsing)) { } @@ +70,5 @@ > > if (!Preferences::GetBool("privacy.trackingprotection.enabled", false)) { > + if (Preferences::GetBool("privacy.trackingprotection.pbmode.enabled", false)) { > + if (!NS_UsePrivateBrowsing(aChannel)) { > + LOG(("nsChannelClassifier[%p]: .pbmode.enabled is ON but channel is NOT in Private Browsing mode", this)); This is too spammy for my taste. We call this function twice for every http channel (once for ClassifyLocal and once for Classify). Removing the logs also lets you simplify the check quite a bit. @@ +81,1 @@ > } MOZ_ASSERT(Preferences::GetBool(privacy.tp.enabled) || (Prefs::GetBool(tpmode) && NS_UsePrivateBrowsing)); You can omit this if you remove the logs and the above check is a one-liner ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp @@ +1526,5 @@ > + } else if (NS_LITERAL_STRING(CHECK_TRACKING_PREF).Equals(aData) || > + NS_LITERAL_STRING(CHECK_TRACKING_PB_PREF).Equals(aData)) { > + mCheckTracking = > + Preferences::GetBool(CHECK_TRACKING_PREF, CHECK_TRACKING_DEFAULT) || > + Preferences::GetBool(CHECK_TRACKING_PB_PREF, CHECK_TRACKING_PB_DEFAULT); I believe these are removed in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1134284&attachment=8568312. However, this is not incorrect so it can stay until that other patch lands.

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #6) > Comment on attachment 8572251 [details] [diff] [review] > New independent p.tp.pbmode.enabled pref > > Review of attachment 8572251 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think this change is worth a unittest change. Have a look at > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url- > classifier/tests/mochitest/test_classified_annotations.html. You will have > to force the iframe to load in and out of a private window > (http://mxr.mozilla.org/mozilla-central/search?string=perwindowpb&find=ini) > > ::: netwerk/base/nsChannelClassifier.cpp > @@ +69,5 @@ > > *result = false; > > > > if (!Preferences::GetBool("privacy.trackingprotection.enabled", false)) { > > + if (Preferences::GetBool("privacy.trackingprotection.pbmode.enabled", false)) { > > + if (!NS_UsePrivateBrowsing(aChannel)) { > > This is easier to read with fewer nested ifs > > if (Preferences::GetBool(pbmode) && !NS_UsePrivateBrowsing)) { > } Except I wrote this logic wrong :)

Monica, I've changed the pref as you asked and added a mochitest which tests three things: - normal mode with the new pref ON (trackers loaded) - private browsing mode with the new pref ON (trackers blocked) - private browsing mode with the new pref OFF (trackers loaded)

/me attaches the right patch this time

Comment on attachment 8579842 [details] [diff] [review] New pref and mochitest Review of attachment 8579842 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Just a couple minor comments below. Can you please include a green try run? Also, even though this patch does not change the behavior of PB mode because the pref defaults to off, we should give ehsan and jdm a heads up that it might happen. Can you do that? ::: toolkit/components/url-classifier/tests/mochitest/classifiedAnnotatedPBFrame.html @@ +11,5 @@ > +<body> > + > +<script id="badscript" data-touched="no" src="http://tracking.example.com/tests/toolkit/components/url-classifier/tests/mochitest/evil.js" onload="this.dataset.touched = 'yes';"></script> > + > +<img id="badimage" data-touched="no" src="http://tracking.example.com/tests/toolkit/components/url-classifier/tests/mochitest/raptor.jpg?reload=true" onload="this.dataset.touched = 'yes';"/> Please change this to use a different urlparam or value (doesn't matter, could be foo=bar) and add a comment <!-- The image cache can cache JS handlers, so make sure we use a different URL for raptor.jpg each time --> For bonus points you could add a similar comment to http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/tests/mochitest/classifiedAnnotatedFrame.html. You could also use a copy of the image that has a different name. ::: toolkit/components/url-classifier/tests/mochitest/test_privatebrowsing_trackingprotection.html @@ +162,5 @@ > + testOnWindow(true, function(aWindow) { > + checkLoads(aWindow, true); > + aWindow.close(); > + > + // Private Browsing, without the pref (trackers should be loaded) This is OK, but Promises provide a better API for this kind of thing than callbacks. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise No need to change this implementation, though.

Added unique query string to the image as per comment 10. Carrying mmc's r+.

Comment on attachment 8580516 [details] [diff] [review] Add pref and mochitest Review of attachment 8580516 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Please note that we'd need to change some of the user facing messaging around private browsing if we ever decide to toggle this pref to true by default. That can happen in a follow-up bug, of course. ::: toolkit/components/url-classifier/tests/mochitest/classifiedAnnotatedPBFrame.html @@ +14,5 @@ > + > +<!-- The image cache can cache JS handlers, so make sure we use a different URL for raptor.jpg each time --> > +<img id="badimage" data-touched="no" src="http://tracking.example.com/tests/toolkit/components/url-classifier/tests/mochitest/raptor.jpg?pbmode=test" onload="this.dataset.touched = 'yes';"/> > + > +<iframe id="badframe" data-touched="no" src="http://tracking.example.com/tests/toolkit/components/url-classifier/tests/mochitest/track.html" onload="this.dataset.touched = 'yes';"></iframe> Hmm, you can probably use the onerror event on both img and iframe in order to verify that the loads indeed fail, instead of checking this indirectly through onload. ::: toolkit/components/url-classifier/tests/mochitest/test_privatebrowsing_trackingprotection.html @@ +67,5 @@ > + "a:524:32:" + testData.length + "\n" + > + testData; > + > +function cleanup() { > + SpecialPowers.clearUserPref("privacy.trackingprotection.enabled"); I suggest using SpecialPowers.pushPrefEnv for these too so that you don't need to do the manual cleanup.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #13) > Comment on attachment 8580516 [details] [diff] [review] > Add pref and mochitest > > Review of attachment 8580516 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks! > > Please note that we'd need to change some of the user facing messaging > around private browsing if we ever decide to toggle this pref to true by > default. That can happen in a follow-up bug, of course. Definitely -- there will be some work going on to update https://mana.mozilla.org/wiki/display/FIREFOX/Polaris+Tracking+Protection+UX if we decide to do this.

This addresses Ehsan's review feedback on comment 13.

Comment on attachment 8582197 [details] [diff] [review] Pref and mochitest Review of attachment 8582197 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks!

Added ehsan to the r= in the commit message. Carrying r+ from mmc and ehsan.

Verified Fx39/Fx40 2015-05-05.