1321874 - Potentially save one url classifier lookup for script loads

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

In bug 1321868, I'm adding a URL classifier lookup for each script we load to determine whether it's tracking or not.

bkelly suggested that perhaps we can save this lookup and do one lookup as part of the existing safe-browsing/Necko integration and mark the channel somehow if that lookup determines that the load is from a tracking source.

I'm not too familiar with the url-classifier internals, so I'm not sure if doing that would actually save a lookup.  Francois, do you have any suggestions here?

Thanks!

Comment 1

[François Marier [:francois]]

We run through the URL classifier in these three places:

1. BeginConnect() [1]
2. Open() [2]
3. AsyncOpen() [3]

#2 and #3 are for all Safe Browsing lists (including malware and phishing), but the first one is only for tracking protection. If we tagged the channel with TP info earlier, then we could skip potentially skip #1.

[1] https://dxr.mozilla.org/mozilla-central/rev/f8086bd3c84fc1a42c3625cf3cc2253f0a5e8cfd/netwerk/protocol/http/nsHttpChannel.cpp#5185
[2] https://dxr.mozilla.org/mozilla-central/rev/f8086bd3c84fc1a42c3625cf3cc2253f0a5e8cfd/netwerk/base/nsBaseChannel.cpp#611
[3] https://dxr.mozilla.org/mozilla-central/rev/f8086bd3c84fc1a42c3625cf3cc2253f0a5e8cfd/netwerk/base/nsBaseChannel.cpp#672

Comment 2

[François Marier [:francois]]

These links are from https://wiki.mozilla.org/Phishing_Protection#Code_walkthrough

Comment 3

[(no longer active)]

Thanks Francois!

I think we may be able to achieve the goal here by also doing the classification when we're loading the script and tag the channel but not block the load if TP isn't enabled, sort of along the lines of the approach in bug 1141814.  I'll give this a shot next week.

Comment 4

[(no longer active)]

OK, I think I have a plan for how to do this.  Bug 1141814 effectively added most of the support for annotating tracking channels, but only used it for lowering the channel priority.  So I'm planning to do the following:

1. Fix bug 1324053 because I would like to turn the privacy.trackingprotection.annotate_channels pref on by default.
2. Fix bug 1170190 to provide an API for querying the tracking status of a channel.
3. Redo the way that we mark scripts as tracking to consume that API.

Comment 5

[(no longer active)]

Attachment: Part 1: Revert the script loader changes from bug 1321868

Comment 6

[(no longer active)]

Attachment: Part 2: Use the passive tracking protection API to determine whether a script is tracking

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8819558 [details] [diff] [review]
Part 2: Use the passive tracking protection API to determine whether a script is tracking

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

::: dom/base/nsScriptLoader.h
@@ +138,5 @@
> +    return mIsTracking;
> +  }
> +  void SetIsTracking()
> +  {
> +    mIsTracking = true;

Can we MOZ_ASSERT(!mIsTracking)?  Or can this legitimately be called twice?

Comment 8

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #7)
> ::: dom/base/nsScriptLoader.h
> @@ +138,5 @@
> > +    return mIsTracking;
> > +  }
> > +  void SetIsTracking()
> > +  {
> > +    mIsTracking = true;
> 
> Can we MOZ_ASSERT(!mIsTracking)?  Or can this legitimately be called twice?

I _think_ that should be fine...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcb169203786
Part 1: Revert the script loader changes from bug 1321868; r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea336ca346cd
Part 2: Use the passive tracking protection API to determine whether a script is tracking; r=bkelly

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fcb169203786

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ea336ca346cd

Comment 12

[Alison Shiue]

This patch showed tp5o perf improvements on perfherder, thanks.
For up to date refer: https://treeherder.mozilla.org/perf.html#/alerts?id=4626