1170190 - Add a passive (detection only) mode for Tracking Protection

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P5)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

It would be nice if we could have a mode for TP that would run the classifier as usual, but not block anything. It would just put the status on the channel (or somewhere), and we could use that information to inform the user that tracking is (potentially?) occurring.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Karen - We need some Product guidance for this

Comment 2

[:Margaret Leibovic]

This requires a higher level discussion about what we want to do with tracking protection beyond Fx42.

Comment 3

[Karen Rudnitski [:kar]]

Adding Barbara for tracking and figuring out our plan for beyond Fx42

Comment 4

[Barbara Bermes [:barbara] - NI please!]

snorp, what's the different between this and 1170194 (tracking detection)

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Bug 1170194 would use this to implement the feature.

Comment 6

[:Margaret Leibovic]

Francois, is this issue already on your team's radar somewhere else? Basically we want to be able to know if there are tracking elements loaded on a page, so we can show that data to the user as a reason to potentially want to enable tracking protection.

Comment 7

[François Marier [:francois]]

Steve told me that it did come up in Boston and that someone had found a way to do this today so it sounded like we wouldn't have to do anything on the platform side. I've forgotten the exact details though.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Right now if TP in PBM is disabled for a site, then Control Center shows two different messages if trackers are detected or not. The assumption was that if we can do that, then we can add new UI for Tracking Detection. I am not sure of the API they use here.

Comment 9

[(no longer active)]

Bug 1141814 actually ended up doing most of the work here.

I'm planning to make the privacy.trackingprotection.annotate_channels pref only control whether channels are annotated as tracking or non-tracking, and add an API to nsIChannel to query that information.  I'm going to create another pref (privacy.trackingprotection.lower_network_priority) to control the behavior in bug 1141814.

Comment 10

[(no longer active)]

Attachment: Part 1: Split out a new pref from privacy.trackingprotection.annotate_channels to explicitly control whether the channel priority is adjusted

Comment 11

[(no longer active)]

Attachment: Part 2: Add the nsIHttpChannel::IsTrackingResource() API to query the channel's tracking annotation

Comment 12

[(no longer active)]

Attachment: Part 3: Add tests for the e10s case too

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8819433 [details] [diff] [review]
Part 2: Add the nsIHttpChannel::IsTrackingResource() API to query the channel's tracking annotation

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

::: netwerk/base/nsChannelClassifier.cpp
@@ +714,5 @@
> +          }
> +          nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
> +          if (httpChannel) {
> +            static_cast<HttpBaseChannel*>(httpChannel.get())->
> +              SetIsTrackingResource();

can HttpBaseChannel have iid to use queryobject?  I think it can.  no static_cast on http channels please - a bad thing to do!

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8819434 [details] [diff] [review]
Part 3: Add tests for the e10s case too

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

hard to check the tests when there is no single word of explanation.  can you provide some please?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6147,5 @@
> +    nsCOMPtr<nsIParentChannel> parentChannel;
> +    NS_QueryNotificationCallbacks(this, parentChannel);
> +    RefPtr<HttpChannelParent> httpParent = do_QueryObject(parentChannel);
> +    if (httpParent) {
> +        Unused << httpParent->SendSetPriority(newValue);

check ipcclosed

::: netwerk/test/unit/test_trackingProtection_annotateChannels.js
@@ +33,5 @@
>  var httpServer;
>  var normalOrigin, trackingOrigin;
>  var testPriorityMap;
>  var currentTest;
> +var skipNormalPriority, skipLowestPriority;

add a comment what these are intended for

Comment 15

[(no longer active)]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> Comment on attachment 8819433 [details] [diff] [review]
> Part 2: Add the nsIHttpChannel::IsTrackingResource() API to query the
> channel's tracking annotation
> 
> Review of attachment 8819433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsChannelClassifier.cpp
> @@ +714,5 @@
> > +          }
> > +          nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
> > +          if (httpChannel) {
> > +            static_cast<HttpBaseChannel*>(httpChannel.get())->
> > +              SetIsTrackingResource();
> 
> can HttpBaseChannel have iid to use queryobject?  I think it can.

Yeah, I don't see why not!  New patches coming up.

Comment 16

[(no longer active)]

(In reply to Honza Bambas (:mayhemer) from comment #14)
> Comment on attachment 8819434 [details] [diff] [review]
> Part 3: Add tests for the e10s case too
> 
> Review of attachment 8819434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> hard to check the tests when there is no single word of explanation.  can
> you provide some please?


Sure, I'll add some comments.

Comment 17

[(no longer active)]

Attachment: Part 2: Add the nsIHttpChannel::IsTrackingResource() API to query the channel's tracking annotation

Comment 18

[(no longer active)]

Attachment: Part 3: Add tests for the e10s case too

Comment 19

[(no longer active)]

Comment on attachment 8819924 [details] [diff] [review]
Part 3: Add tests for the e10s case too

Honza is away until the next year.  Patrick, do you mind taking a second look at these patches please?

Comment 20

[Patrick McManus [:mcmanus]]

Comment on attachment 8819923 [details] [diff] [review]
Part 2: Add the nsIHttpChannel::IsTrackingResource() API to query the channel's tracking annotation

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

awesome.

how does tp* feel about this? iirc there was some notable overhead to TP being skirted by being default off. but maybe that's OBE.

Comment 21

[Patrick McManus [:mcmanus]]

Comment on attachment 8819924 [details] [diff] [review]
Part 3: Add tests for the e10s case too

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

I guess we can comment that this makes setpriority available in content as well.

Comment 22

[(no longer active)]

(In reply to Patrick McManus [:mcmanus] from comment #20)
> how does tp* feel about this? iirc there was some notable overhead to TP
> being skirted by being default off. but maybe that's OBE.

I'm doing this for bug 1322105 where I win ~3% back on Tpo.  :-)

(In reply to Patrick McManus [:mcmanus] from comment #21)
> I guess we can comment that this makes setpriority available in content as
> well.

Will do.  Thanks for the reviews!

Comment 23

[(no longer active)]

BTW I think you may be thinking of bug 1122691.  The thing that causes the regression is disabling speculative connect and DNS prefetching for channels because we don't want to do those things if after classification it becomes clear we want to block the connection.  I made sure in bug 1324053 that we don't disable those things for passive TP, so that concern is no longer applicable.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c65588f41f
Part 1: Split out a new pref from privacy.trackingprotection.annotate_channels to explicitly control whether the channel priority is adjusted; r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/891791aa2301
Part 2: Add the nsIHttpChannel::IsTrackingResource() API to query the channel's tracking annotation; r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac1af4f133b
Part 3: Add tests for the e10s case too; r=mayhemer

Comment 25

[(no longer active)]

Shoot, I accidentally pushed these without amending the commit messages.  Sorry about that.  :/

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d2c65588f41f
https://hg.mozilla.org/mozilla-central/rev/891791aa2301
https://hg.mozilla.org/mozilla-central/rev/1ac1af4f133b