1138979 - Pref to turn TP on when in Private Browsing mode

Status: VERIFIED FIXED

(Firefox :: Private Browsing, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 2

[François Marier [:francois]]

Attachment: New privacy.trackingprotection.pbonly pref

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 3

[[:mmc] Monica Chew (no longer reading bugmail)]

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 4

[[:mmc] Monica Chew (no longer reading bugmail)]

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.

Comment 5

[François Marier [:francois]]

Attachment: New independent p.tp.pbmode.enabled pref

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 6

[[:mmc] Monica Chew (no longer reading bugmail)]

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.

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 8

[François Marier [:francois]]

Attachment: New pref and mochitest

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)

Comment 9

[François Marier [:francois]]

Attachment: New pref and mochitest

/me attaches the right patch this time

Comment 10

[[:mmc] Monica Chew (no longer reading bugmail)]

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.

Comment 11

[François Marier [:francois]]

Attachment: Add pref and mochitest

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

Comment 12

[François Marier [:francois]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33e72aa11bcd

Comment 13

[(no longer active)]

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.

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 15

[François Marier [:francois]]

Attachment: Pref and mochitest

This addresses Ehsan's review feedback on comment 13.

Comment 16

[François Marier [:francois]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e46e05ef7f72

Comment 17

[(no longer active)]

Comment on attachment 8582197 [details] [diff] [review]
Pref and mochitest

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

Nice, thanks!

Comment 18

[François Marier [:francois]]

Attachment: Pref and test

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6f3151d4ff03

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6f3151d4ff03

Comment 21

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified Fx39/Fx40 2015-05-05.