Closed Bug 1170190 Opened 4 years ago Closed 3 years ago

Add a passive (detection only) mode for Tracking Protection

Categories

(Toolkit :: Safe Browsing, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: snorp, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.
tracking-fennec: --- → ?
Karen - We need some Product guidance for this
Flags: needinfo?(krudnitski)
This requires a higher level discussion about what we want to do with tracking protection beyond Fx42.
tracking-fennec: ? → ---
Adding Barbara for tracking and figuring out our plan for beyond Fx42
Flags: needinfo?(krudnitski) → needinfo?(bbermes)
snorp, what's the different between this and 1170194 (tracking detection)
Flags: needinfo?(bbermes) → needinfo?(snorp)
Bug 1170194 would use this to implement the feature.
Flags: needinfo?(snorp)
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.
Flags: needinfo?(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.
Flags: needinfo?(francois) → needinfo?(sworkman)
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.
Flags: needinfo?(sworkman)
Component: DOM: Security → Safe Browsing
Product: Core → Toolkit
Blocks: 1141814
Priority: -- → P5
Summary: Add a passive mode for Tracking Protection → Add a passive (detection only) mode for Tracking Protection
No longer blocks: 1141814
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.
Assignee: nobody → ehsan
Blocks: 1321874
Depends on: 1324053
Attachment #8819434 - Flags: review?(honzab.moz)
Attachment #8819432 - Flags: review?(honzab.moz) → review+
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!
Attachment #8819433 - Flags: review?(honzab.moz) → review-
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
(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.
(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.
Attachment #8819433 - Attachment is obsolete: true
Attachment #8819924 - Flags: review?(honzab.moz)
Attachment #8819434 - Attachment is obsolete: true
Attachment #8819434 - Flags: review?(honzab.moz)
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?
Attachment #8819924 - Flags: review?(honzab.moz) → review?(mcmanus)
Attachment #8819923 - Flags: review?(honzab.moz) → review?(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.
Attachment #8819923 - Flags: review?(mcmanus) → review+
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.
Attachment #8819924 - Flags: review?(mcmanus) → review+
(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!
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.
Pushed by eakhgari@mozilla.com:
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
Shoot, I accidentally pushed these without amending the commit messages.  Sorry about that.  :/
Blocks: 1333994
Depends on: 1334241
See Also: → 1342265
You need to log in before you can comment on or make changes to this bug.