Selectively sending the DNT header to sites on a DNT-compliant list wasn't going to work with the DOM API for DNT, so we decided to instead send all sites a DNT header when the user is in TP mode.

There are two places where we send the DNT signal: - the DoNotTrack header: https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/netwerk/protocol/http/nsHttpHandler.cpp#492-498 - the DoNotTrack DOM API: https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/dom/base/Navigator.cpp#760-770 They currently just look at privacy.donottrackheader.enabled and that needs to be replaced with something like: enableDNT = getbool(privacy.donottrackheader.enabled) || TrackingProtectionEnabled() where TrackingProtectionEnabled() essentially does this check: if (!Preferences::GetBool("privacy.trackingprotection.enabled", false) && (!Preferences::GetBool("privacy.trackingprotection.pbmode.enabled", false) || !NS_UsePrivateBrowsing(aChannel))) { return false; } else { return true; } See: https://dxr.mozilla.org/mozilla-central/rev/c67dc1f9fab86d4f2cf3224307809c44fe3ce820/netwerk/base/nsChannelClassifier.cpp#69-73 Ideally, instead of duplicating that check, we should refactor it into a separate function and then call it from both places. Maybe that function should live in the URI Classifier or in the HttpChannel itself, I'm not sure. The four cases we need to test are: - DNT turned on, TP turned off -> DNT signal sent both in private browsing and normal mode - DNT turned off, TP turned on globally -> DNT signal sent both in private browsing and normal mode - DNT turned off, TP in Private Browsing only -> DNT signal sent in private browsing mode only - DNT turned off, TP turned off -> DNT signal is never sent It would be good to test that the "DNT" request header is the right one and that the "navigator.DoNotTrack()" JavaScript API: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/doNotTrack returns the right value too.

Hi Francois, The reason i move set DNT header from |Init| to |begineConnect| is because now we will set 'DNT' based on private browsing mode and in the |Init| it is too early to check if this channel is in private browsing mode. But I am not really sure where is the best place to put the header check, i guess we will need a necko peer to review this also.

The testcase covers suggestions by francois in comment2

Comment on attachment 8756683 [details] [diff] [review] P2. Set DNT header based on preference and private browsing mode Review of attachment 8756683 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me, but we should ask a necko peer to review this one. I don't know the implications of moving the place where we check the pref and set the header. ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +7548,5 @@ > +nsHttpChannel::SetDoNotTrack() > +{ > + /** > + * 'DoNotTrack' header should be added if 'privacy.donottrackheader.enabled' > + * is true or tracking protection is enabled. Let's reference this bug in the comment by adding "(see bug 1258033)" at the end of this comment.

Comment on attachment 8756684 [details] [diff] [review] P3. Return navigator.doNotTrack based on preference and private browsing mode Review of attachment 8756684 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Navigator.cpp @@ +777,5 @@ > + nsCOMPtr<nsILoadContext> loadContext; > + win->GetInterface(NS_GET_IID(nsILoadContext), getter_AddRefs(loadContext)); > + > + bool isPrivateBrowsing = false; > + loadContext->GetUsePrivateBrowsing(&isPrivateBrowsing); I wonder if we couldn't put the IsTrackingProtectionEnabled util function in the load context or the channel instead? It would be good to avoid duplicating the pref checking if we can because there might be other ways for tracking protection to be enabled in the future. This (if it's possible) would be quite nice: bool isTrackingProtectionEnable = false loadContext->GetTrackingProtectionEnabled(&isTrackingProtectionEnabled)

Comment on attachment 8756685 [details] [diff] [review] P4. Testcase Review of attachment 8756685 [details] [diff] [review]: ----------------------------------------------------------------- Very thorough test, thanks! Only a small issue around pref cleanup at the end of the test. ::: toolkit/components/url-classifier/tests/mochitest/test_donottrack.html @@ +73,5 @@ > +const contentPage = > + "http://mochi.test:8888/tests/toolkit/components/url-classifier/tests/mochitest/dnt.html"; > + > +function executeTest(test) { > + SpecialPowers.setBoolPref(DNT_PREF, test.setting.dntPref); Maybe use SpecialPowers.pushPrefEnv() and popPrefEnv() instead? We should clean up the prefs at the end to avoid interfering with any other tests.

Comment on attachment 8756682 [details] [diff] [review] P1. Support IsTrackingProtectionEnabled util function Review of attachment 8756682 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but it may need to move somewhere else if you find a way to address comment 9. ::: netwerk/base/nsChannelClassifier.cpp @@ +587,5 @@ > +// static > +bool > +nsChannelClassifier::IsTrackingProtectionEnabled(nsIChannel *aChannel) > +{ > + if (!Preferences::GetBool("privacy.trackingprotection.enabled", false) && nit: it might be a good time to rewrite this check so that it's easier to read For example: if (privacy.trackingprotection.enabled) { return true } if (privacy.trackingprotection.pbmode && NS_UsePrivateBrowsing(channel)) { return true } return false

hi Olli, Could you help review if it is ok to add new API for nsILoadContext ? The purpose for this API is to know if tracking protection is on for the load context based on preference and private browsing mode. Thanks!

Hi Patrick, The patch is to add DNT header based on preference and tracking protection mode. Comment 5 explains why i change the place to set DNT header. Another question is that i keep |mDoNotTrackEnabled| for telemetry[1] But should i also update how we record the Telemetry::DNT_USAGE since now if DNT header should be sent is changed. [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp#2144

Address Comment 10

Comment on attachment 8757183 [details] [diff] [review] P1. Add IsTrackingProtectionOn for nsILoadContext >+ >+ /** >+ * Returns true if tracking protection is enabeld for the load context. enabled >+ */ >+ [noscript] boolean IsTrackingProtectionOn(); Why noscript? A tad odd in an interface which isn't marked to be builtinclass >+ >+%{C++ >+ /** >+ * De-XPCOMed getter to make call-sites cleaner. >+ */ >+ bool UseTrackingProtection() { >+ bool usingTP; >+ IsTrackingProtectionOn(&usingTP); >+ return usingTP; >+ } >+%} Ahaa, [infallible] readonly attribute wouldn't work since the interface isn't builtinclass.

Comment on attachment 8757190 [details] [diff] [review] P3. navigator.doNotTrack should be determinded by preference and tracking protection Review of attachment 8757190 [details] [diff] [review]: ----------------------------------------------------------------- I would suggest getting a DOM peer to review this one too once you've added the null check. ::: dom/base/Navigator.cpp @@ +757,5 @@ > Navigator::GetDoNotTrack(nsAString &aResult) > { > + aResult.AssignLiteral("unspecified"); > + > + nsGlobalWindow* win = nsGlobalWindow::Cast(mWindow); I would add a null check like NS_ENSURE_TRUE(win, NS_OK);

Comment on attachment 8757192 [details] [diff] [review] P4. Use LoadContext API for channel classifier Review of attachment 8757192 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, perhaps roll it into the other necko patch so that mcmanus looks at both of them at once?

Comment on attachment 8757194 [details] [diff] [review] P5. Testcase Review of attachment 8757194 [details] [diff] [review]: ----------------------------------------------------------------- Looks great!

Merge P2 and P4 to review based on francois's suggestion. Purpose of this patch is describe in Comment 5 and Comment 14

Hi Olli, Could you also help review this patch ? thanks!

Comment on attachment 8757192 [details] [diff] [review] P4. Use LoadContext API for channel classifier These got merged (see comment 22).

Comment on attachment 8757537 [details] [diff] [review] P3. navigator.doNotTrack should be determinded by preference and tracking protection v2 Could we figure out a way to not lose the boolvarcache part. Maybe add AddBoolVarCache to nsContentUtils and use the same value in dochshell and in this code. This is not hot code or anything, but one never knows what some silly DOM benchmarks do. And there isn't any reason to not use varcache. And besides, that reduces code duplication when the reading of the pref is handled in one place. I know, this is über-nit.

(In reply to Olli Pettay [:smaug] from comment #18) > >+ */ > >+ [noscript] boolean IsTrackingProtectionOn(); > Why noscript? A tad odd in an interface which isn't marked to be builtinclass Yes, you are right, i shouldn't add noscript, will remove it in next patch

(In reply to Olli Pettay [:smaug] from comment #25) > Could we figure out a way to not lose the boolvarcache part. > Maybe add AddBoolVarCache to nsContentUtils and use the same value in > dochshell and in this code. The docshell did not use the "privacy.donottrackheader.enabled", but in both navigator and nsHttpChannel in patch P2 will use this preference. I will add boolvarcache in nsContentUtils, thanks for you suggestion!

Address Comment 18

The |nsContentUtils::DoNotTrackEnabled| should be also used by |nsHttpChannel| in patch Part2. I will add it after the patch is reviewed.

Comment on attachment 8757772 [details] [diff] [review] P3. navigator.doNotTrack should be determinded by preference and tracking protection v3 > Navigator::GetDoNotTrack(nsAString &aResult) > { >- if (sDoNotTrackEnabled) { >+ aResult.AssignLiteral("unspecified"); >+ >+ nsGlobalWindow* win = nsGlobalWindow::Cast(mWindow); >+ NS_ENSURE_TRUE(win, NS_OK); So this is a change to the behavior, returning unspecified for disconnected Navigator. >+ >+ if ((loadContext && loadContext->UseTrackingProtection()) || >+ nsContentUtils::DoNotTrackEnabled()) { > aResult.AssignLiteral("1"); >- } else { >- aResult.AssignLiteral("unspecified"); > } Perhaps you could write this: bool doNotTrack = nsContentUtils::DoNotTrackEnabled(); if (!doNotTrack) { nsCOMPtr<nsILoadContext> loadContext = do_GetInterface(mWindow); doNotTrack = loadContext && loadContext->UseTrackingProtection(); } if (doNotTrack) { aResult.AssignLiteral("1"); } else { aResult.AssignLiteral("unspecified"); } return NS_OK; >+nsContentUtils::DoNotTrackEnabled() >+{ Given that we're planning to use this in Necko, want to add MOZ_ASSERT(NS_IsMainThread()); with that all, r+

Hi Olli, Sorry i missed the serialized load context part for review, could you help review again ? thanks! One thing I am not sure is in |TabParent::GetLoadContext|[1], in this patch I didn't pass "tracking protection on/off" to LoadContext constructor, should I pass it ? If yes, do you have any suggestion what is a better way to do it. Because I am not sure if check private browsing mode and preference here is a good idea. [1]https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#2776

(In reply to Dimi Lee[:dimi][:dlee] from comment #31) > Because I am not sure if check private browsing mode and > preference here is a good idea. Why not? That is what I'd expect. Then we'd get consistent handling with docshell.

Comment on attachment 8758648 [details] [diff] [review] Missing serialized load context(This should be merged to P1) So based on that I assume a new patch. Or if you think we for some reason should use the pb flag and/or pref in loadcontext, please explain why not.

(In reply to Olli Pettay [:smaug] from comment #32) > (In reply to Dimi Lee[:dimi][:dlee] from comment #31) > > Because I am not sure if check private browsing mode and > > preference here is a good idea. > Why not? > That is what I'd expect. Then we'd get consistent handling with docshell. Since IsTrackingProtectionOn is based on pref and pb flag, and LoadContext already has pb flag(mUsePrivateBrowsing). I am thinking if we should implement the check directly in the LoadContext::IsTrackingProtectionOn instead of passing it in TabParent.

oh you mean that. Yes I was thinking too that it should be implemented in LoadContext::IsTrackingProtectionOn based on the existing pb flag and pref.

(In reply to Olli Pettay [:smaug] from comment #35) > oh you mean that. Yes I was thinking too that it should be implemented in > LoadContext::IsTrackingProtectionOn based on the existing pb flag and pref. Sorry I didn't explain it clearly in previous comment. In the TabParent case, it looks like a good idea implement it in the LoadContext::IsTrackingProtectionOn itself. But LoadContext is also used in ipc[1], in this case, it seems LoadContext::IsTrackingProtectionOn should represent the value passed in SerializedLoadContext, not pref + pb flag. I am not really familiar with loadContext, so not sure what is the best way here. [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#200

Ask for review again based on Olli's suggestion in Comment 35.

Merge P1 and "Implement IsTrackingProtection in LoadContext" patch

This is original P3, but according to Comment 25 and Comment 27. Add |nsContentUtils::DoNotTrackEnabled| to this patch and it will also be used by necko in P2. So reverse the order of p2 and p3.

Move this to P3

fix a error that testcase may timeout

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

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/e4a2e5f30099 Part 1: Add IsTrackingProtectionOn for nsILoadContext. r=smaug https://hg.mozilla.org/integration/fx-team/rev/9df3db7f35d8 Part 2: navigator.doNotTrack should be determinded by preference and tracking protection. r=smaug https://hg.mozilla.org/integration/fx-team/rev/1311e22e3ab4 Part 3: Set DNT header based on preference and tracking protection. r=mcmanus https://hg.mozilla.org/integration/fx-team/rev/001698fcd787 Part 4: Testcase for DNT. r=francois

https://hg.mozilla.org/mozilla-central/rev/e4a2e5f30099 https://hg.mozilla.org/mozilla-central/rev/9df3db7f35d8 https://hg.mozilla.org/mozilla-central/rev/1311e22e3ab4 https://hg.mozilla.org/mozilla-central/rev/001698fcd787

https://hg.mozilla.org/mozilla-central/rev/e4a2e5f30099 https://hg.mozilla.org/mozilla-central/rev/9df3db7f35d8 https://hg.mozilla.org/mozilla-central/rev/1311e22e3ab4 https://hg.mozilla.org/mozilla-central/rev/001698fcd787

Tested using steps in comment 2. I looked at the various combinations of tracking protection, private browsing and DNT. I also verified the value of navigator.doNotTrack under the same conditions. All seem to behave as expected. We are sending the DNT header in the cases we want to, and not sending it otherwise. In addition, the return value of that JS API reflects the same. Verified on Nightly, 2016-06-08.

And now how to disable DNT when TP is enabled? Is there some way remained at least via about:config? The aforementioned loop-hole does not exist if I set TP to always on (instead of selected by the Disconnect list) and DNT to always off. And there is no longer a possibility to get such config?

(In reply to Stas Sergeev from comment #48) > And now how to disable DNT when TP > is enabled? Is there some way remained > at least via about:config? DNT cannot be disabled if TP is enabled. This is by design. See the "user story" field at the top of this bug.

(In reply to François Marier [:francois] from comment #49) > DNT cannot be disabled if TP is enabled. This is by design. See the "user > story" field at the top of this bug. And how about seeing my msg instead? --- The aforementioned loop-hole does not exist if I set TP to always on (instead of selected by the Disconnect list) and DNT to always off. --- Not sure if I can explain this even clearer. As always, useful config was removed for no good reason.

(In reply to Stas Sergeev from comment #50) > The aforementioned loop-hole does not > exist if I set TP to always on (instead > of selected by the Disconnect list) and > DNT to always off. I don't understand the configuration you're describing. Tracking protection relies on the list of trackers curated by Disconnect. That's how it determines whether or not a third-party resource will be blocked.

(In reply to François Marier [:francois] from comment #51) > I don't understand the configuration you're describing. Tracking protection > relies on the list of trackers curated by Disconnect. That's how it > determines whether or not a third-party resource will be blocked. OK, let me explain the setup I look for. https://disconnect.me/trackerprotection --- Tracking is the collection of data regarding a particular user's activity across multiple websites or applications that aren’t owned by the data collector, and the retention, use or sharing of that data. --- What I want is firefox to disallow the aforementioned sharing of any data between sites. For example the use of non-owned cookies. In this case even if the tracker is removed from Disconnect list, it will not be able to track. So I can keep DNT turned off and there is no aforementioned loop-hole. Is such setup possible?

> --- > Tracking is the collection of data regarding a particular user's activity across multiple websites > or applications that aren’t owned by the data collector, and the retention, use or sharing of that data. > --- > What I want is firefox to disallow the aforementioned sharing of any data between sites. That high-level goal is more-or-less what we are trying to do with our anti-tracking effort: https://blog.mozilla.org/futurereleases/2018/08/30/changing-our-approach-to-anti-tracking/ The hope is that once that work is complete, you'll be able to prevent tracking without turning ON tracking protection or DNT.

> The hope is that once that work is complete, you'll be able to prevent tracking without turning ON tracking protection I was assuming this is (or should be) a part of the tracking protection... Thanks for the clear answer! Waiting for ff-65 then.