1404804 - Tracking black list is checked twice when TP is enabled

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[Kershaw Chang [:kershaw]]

[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x11dfd14c0]: Enabling tracking protection checks on channel[0x115b37050] with uri http://c.amazon-adsystem.com/aax2/apstag.js for toplevel window https://edition.cnn.com/
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x11dfd14c0]:CheckIsTrackerWithLocalTable for uri=http://c.amazon-adsystem.com/aax2/apstag.js
[Main Thread]: D/nsChannelClassifier IsTrackerBlacklistedCallback[0x11dfd14c0]:OnClassifyComplete  status=0x805d0022, tpEnabled=1 prefix=
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x11dfd14c0]: Classifying principal http://c.amazon-adsystem.com/aax2/apstag.js on channel with uri http://c.amazon-adsystem.com/aax2/apstag.js
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x11dfd14c0]: suspended channel 0x115b37050
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x11dfd14c0]: Looking for http://edition.cnn.com/?resource=amazon-adsystem.com in the whitelist
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x11dfd14c0]: http://edition.cnn.com/?resource=amazon-adsystem.com is not in the whitelist
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x11dfd14c0]:OnClassifyComplete NS_ERROR_TRACKING_URI (suspended channel)
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x11dfd14c0]: cancelling channel 0x115b37050 for http://c.amazon-adsystem.com/aax2/apstag.js with error code NS_ERROR_TRACKING_URI, prefix=tT128Q==
[Main Thread]: D/nsChannelClassifier nsChannelClassifier[0x11dfd14c0]: resuming channel 0x115b37050 from OnClassifyComplete
[Main Thread]: D/nsChannelClassifier nsChannelClassifier::~nsChannelClassifier 0x11dfd14c0

See the log above, the first time the black list is checked at [1] and the second time is at [2].

I think we should only check the table once.

[1] https://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/netwerk/base/nsChannelClassifier.cpp#1303
[2] https://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/netwerk/base/nsChannelClassifier.cpp#639

Comment 1

[Kershaw Chang [:kershaw]]

The current behavior of TP channel classification is like:

TP disabled:
1. Check local black list.
2. Check white list if found a hit in the black list.
3. Mark the channel as tracking resource.
4. Begin connect.
5. Start classifer again (with phishing and malware table, without TP list)

TP enabled:
1. Check local black list.
2. Disable DNS prefetch and speculative connection if there is a hit.
3. Begin connect.
4. Start classifer again (with phishing and malware and TP table)
5. Check whitelist.
6. Cancel the channel.

I was thinking about whether we can not do step 2 when TP is enabled. Can we just check the white list? If we can't find a match in white list, let's cancel the channel directly. I think this way would make the code easier to maintain and readable.

What do you think, Francois?

Comment 2

[François Marier [:francois]]

(In reply to Kershaw Chang [:kershaw] from comment #1)
> I was thinking about whether we can not do step 2 when TP is enabled. Can we
> just check the white list? If we can't find a match in white list, let's
> cancel the channel directly. I think this way would make the code easier to
> maintain and readable.
> 
> What do you think, Francois?

If I understand correctly, you're proposing the following?

TP enabled:
1. Check local TP black list.
2. Check white list if found a hit in the black list.
3. Cancel the channel if not on the whitelist.
4. Begin connect.
5. Start classifier again (with phishing and malware, without TP table)
6. Cancel the channel if found on malware/phishing blacklists.

That would work. It would make the TP enabled case work pretty much exactly the same as the TP disabled case.

Comment 3

[Kershaw Chang [:kershaw]]

(In reply to François Marier [:francois] from comment #2)
> (In reply to Kershaw Chang [:kershaw] from comment #1)
> > I was thinking about whether we can not do step 2 when TP is enabled. Can we
> > just check the white list? If we can't find a match in white list, let's
> > cancel the channel directly. I think this way would make the code easier to
> > maintain and readable.
> > 
> > What do you think, Francois?
> 
> If I understand correctly, you're proposing the following?
> 
> TP enabled:
> 1. Check local TP black list.
> 2. Check white list if found a hit in the black list.
> 3. Cancel the channel if not on the whitelist.
> 4. Begin connect.
> 5. Start classifier again (with phishing and malware, without TP table)
> 6. Cancel the channel if found on malware/phishing blacklists.
> 
> That would work. It would make the TP enabled case work pretty much exactly
> the same as the TP disabled case.

Yes, that's what I proposed.

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Part1: Make the behavior of tracking protection closer to tracking annotation

Summary:
 - Simplify the code in nsChannelClassifier.
 - The flow of tracking protection is the same as tracking annotation, except the channel will be canceled.

Please take a look. Thanks.

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Part2: Remove mLocalBlocklist flag

Summary:
 - Remove mLocalBlocklist in nsHttpChannel and other related code.

I think this makes the code a bit easier to read. Thanks.

Comment 6

[Kershaw Chang [:kershaw]]

Comment on attachment 8915899 [details] [diff] [review]
Part1:  Make the behavior of tracking  protection closer to tracking annotation

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

::: netwerk/base/nsChannelClassifier.cpp
@@ +810,1 @@
>  

At my local test, I found that if there is a match in local black list, the prefix would be empty. So, I remove this check.
I think it's ok to remove this?

Comment 7

[François Marier [:francois]]

Comment on attachment 8915899 [details] [diff] [review]
Part1:  Make the behavior of tracking  protection closer to tracking annotation

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

The new flow makes sense to me.

::: netwerk/base/nsChannelClassifier.cpp
@@ +1127,2 @@
>  
> +      // Channel will be cancelled (page element blocked) due to tracking

I think that this comment is (now) wrong because it's not possible for the channel to be canceled for tracking reasons here. The MOZ_ASSERT() you added prevents this.

So it should just say "Safe Browsing".

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8915902 [details] [diff] [review]
Part2: Remove mLocalBlocklist flag

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

Thanks.  I think this is mostly right, I just need to confirm I understand how this works.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +552,2 @@
>  
> +    if (isTrackingResource) {

when exactly is the mIsTrackingResource flag set now?

@@ +901,5 @@
>      // don't speculate if we are on a local blocklist, on uses of the offline
>      // application cache, if we are offline, when doing http upgrade (i.e.
>      // websockets bootstrap), or if we can't do keep-alive (because then we
>      // couldn't reuse the speculative connection anyhow).
> +    if (mApplicationCache || gIOService->IsOffline() ||

ok, I think this needs an update.  instead mLocalBlocklist, should we use mIsTrackingResource here?  also the comment needs an update.  or is that we don't even get here when the channel is tracker/localblocklisted?

@@ +6565,5 @@
>      if (mCanceled) {
>          return mStatus;
>      }
>  
> +    if (!mConnectionInfo->UsingHttpProxy() &&

same here

@@ +6600,5 @@
>          GetOrCreateChannelClassifier();
>      LOG(("nsHttpChannel::Starting nsChannelClassifier %p [this=%p]",
>           channelClassifier.get(), this));
>      channelClassifier->Start();
> +

so, this swaps the order of starting the classifier and call to ContinueBeginConnectWithResult.  but since both do more or less an async thing, it should not have much influence.

Comment 9

[Honza Bambas (:mayhemer)]

comment 8

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #8)
> Comment on attachment 8915902 [details] [diff] [review]
> Part2: Remove mLocalBlocklist flag
> 
> Review of attachment 8915902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks.  I think this is mostly right, I just need to confirm I understand
> how this works.
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +552,2 @@
> >  
> > +    if (isTrackingResource) {
> 
> when exactly is the mIsTrackingResource flag set now?
> 

mIsTrackingResource is set at [1] and [2], which is the same as before.
Note that if TP is enabled, the channel is going to be canceled without setting mIsTrackingResource.

[1] https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/netwerk/base/nsChannelClassifier.cpp#1042
[2] https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/netwerk/base/nsChannelClassifier.cpp#1088

> @@ +901,5 @@
> >      // don't speculate if we are on a local blocklist, on uses of the offline
> >      // application cache, if we are offline, when doing http upgrade (i.e.
> >      // websockets bootstrap), or if we can't do keep-alive (because then we
> >      // couldn't reuse the speculative connection anyhow).
> > +    if (mApplicationCache || gIOService->IsOffline() ||
> 
> ok, I think this needs an update.  instead mLocalBlocklist, should we use
> mIsTrackingResource here?  also the comment needs an update.  or is that we
> don't even get here when the channel is tracker/localblocklisted?
> 

I am not sure if we want to use mIsTrackingResource here. Do we really want to disable DNS prefetch and speculative connection for a tracker?

If TP is enabled and the channel is localblocklisted, we won't get here. In other words, mIsTrackingResource only matters when TP is disabled.

> @@ +6565,5 @@
> >      if (mCanceled) {
> >          return mStatus;
> >      }
> >  
> > +    if (!mConnectionInfo->UsingHttpProxy() &&
> 
> same here
> 

Should we also use mIsTrackingResource here?

> @@ +6600,5 @@
> >          GetOrCreateChannelClassifier();
> >      LOG(("nsHttpChannel::Starting nsChannelClassifier %p [this=%p]",
> >           channelClassifier.get(), this));
> >      channelClassifier->Start();
> > +
> 
> so, this swaps the order of starting the classifier and call to
> ContinueBeginConnectWithResult.  but since both do more or less an async
> thing, it should not have much influence.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #10)
> (In reply to Honza Bambas (:mayhemer) from comment #8)
> > Comment on attachment 8915902 [details] [diff] [review]
> > Part2: Remove mLocalBlocklist flag
> > 
> > Review of attachment 8915902 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks.  I think this is mostly right, I just need to confirm I understand
> > how this works.
> > 
> > ::: netwerk/protocol/http/nsHttpChannel.cpp
> > @@ +552,2 @@
> > >  
> > > +    if (isTrackingResource) {
> > 
> > when exactly is the mIsTrackingResource flag set now?
> > 
> 
> mIsTrackingResource is set at [1] and [2], which is the same as before.
> Note that if TP is enabled, the channel is going to be canceled without
> setting mIsTrackingResource.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 1a4a26905f923458679a59a4be1e455ebc53c333/netwerk/base/nsChannelClassifier.
> cpp#1042
> [2]
> https://searchfox.org/mozilla-central/rev/
> 1a4a26905f923458679a59a4be1e455ebc53c333/netwerk/base/nsChannelClassifier.
> cpp#1088

Good!

> 
> > @@ +901,5 @@
> > >      // don't speculate if we are on a local blocklist, on uses of the offline
> > >      // application cache, if we are offline, when doing http upgrade (i.e.
> > >      // websockets bootstrap), or if we can't do keep-alive (because then we
> > >      // couldn't reuse the speculative connection anyhow).
> > > +    if (mApplicationCache || gIOService->IsOffline() ||
> > 
> > ok, I think this needs an update.  instead mLocalBlocklist, should we use
> > mIsTrackingResource here?  also the comment needs an update.  or is that we
> > don't even get here when the channel is tracker/localblocklisted?
> > 
> 
> I am not sure if we want to use mIsTrackingResource here. Do we really want
> to disable DNS prefetch and speculative connection for a tracker?

I checked this is called after untail, so I think we are good.

> 
> If TP is enabled and the channel is localblocklisted, we won't get here. In
> other words, mIsTrackingResource only matters when TP is disabled.

Good.

> 
> > @@ +6565,5 @@
> > >      if (mCanceled) {
> > >          return mStatus;
> > >      }
> > >  
> > > +    if (!mConnectionInfo->UsingHttpProxy() &&
> > 
> > same here
> > 
> 
> Should we also use mIsTrackingResource here?

I *think* something should change here.  When TP is off but annotation is on (default) while this is a tracking resource, we will likely get tailed AFTER this point.  Hence a DNS prefetched entry may expire until that (the DNS cache may have a short timeout) and also will eat, despite small, portion of the bandwidth.

But let's leave this for a followup.  I can see we are already suspending the channel between DNS prefetch and doing the actual request, so we probably either have the problem already or it's not a problem at all.

Conclusion: let's leave it as you wrote it.

Comment 12

[Kershaw Chang [:kershaw]]

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

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: Part1: Make the behavior of tracking protection closer to tracking annotation, r=francois

Summary:
 - Rebase
 - Carry r+.

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Part2: Remove mLocalBlocklist flag, r=mayhemer

Carry r+.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Part 2 needs rebasing.

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: Part2: Remove mLocalBlocklist flag, r=mayhemer

Rebased.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba74c117154
Part 1: Make the behavior of tracking protection closer to tracking annotation. r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/1377a75e3d6b
Part 2: Remove mLocalBlocklist flag. r=mayhemer

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2ba74c117154
https://hg.mozilla.org/mozilla-central/rev/1377a75e3d6b