1522210 - FingerPrinting and Cryptomining classifiers need a separate content log id.

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

At the moment, fingerprinting and cryptomining classifiers use tracking protection content log ID and that makes impossible, for the front end code, to distinguish them.

Comment 1

[(no longer active)]

Andrea, we haven't really discussed how fingerprinting/cryptocurrency mining blocking would work in Gecko anywhere, so I'm going to do that here. This may be expanding the scope of this bug a bit, sorry about that. If you feel like we need more bugs to be filed, please feel free to do so.

Looking at UrlClassifierFeatureCryptomining.cpp and UrlClassifierFeatureFingerprinting.cpp I can see several problems with how they currently work.

• They currently hijack NS_ERROR_TRACKING_URI and also use nsIHttpChannelInternal::CancelForTrackingProtection(). They should stop doing that. We probably want to have a new error code for each of the two features, and cancel the channel for each one with the appropriate error code. You'd need to audit all usages of NS_ERROR_TRACKING_URI and see which ones require similar handling for these two categories, I would expect a minority of them would. LMK if you need help on that.

• Similar to STATE_LOADED_TRACKING_CONTENT and STATE_BLOCKED_TRACKING_CONTENT you'd need two nsIWebProgressListener content blocking event codes per url-classifier feature. Note that now only these flags need to use distinct bits https://searchfox.org/mozilla-central/rev/c035ee7d3a5cd6913e7143e1bce549ffb4a566ff/uriloader/base/nsIWebProgressListener.idl#289 so there are many new bits freed up for usage. Here some refactoring in url-classifier is also needed, for example UrlClassifierCommon::ShouldEnableClassifier() shouldn't be calling NotifyTrackingProtectionDisabled() directly, maybe it should go through the feature or something?

I would really appreciate if you did this in small incremental patches, since this will be a significant amount of code change which will be hard to review (since it will be easy to miss something somewhere) so keeping each patch focused on a single small functional change would make the review much simpler. :-) Thanks!

Comment 2

[Andrea Marchesini [:baku]]

Ehsan, thanks for this comment. This is I was thinking to do. Actually I already tried to introduce 2 separate error code (and 2 sepeate blocking state in nsWebProgressListener) when I was implementing these classifiers. I know that there is a lot of code moving.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 1 - UrlClassifierFeatureFactory::IsClassifierBlockingErrorCode, r=ehsan

Comment 4

[Andrea Marchesini [:baku]]

Attachment: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 3 - Canceling nsIChannel with error code, r=ehsan

Depends on D17637

Comment 5

[Andrea Marchesini [:baku]]

Attachment: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 4 - abstract blocking state codes, r=ehsan

Depends on D17638

Comment 6

[Andrea Marchesini [:baku]]

Attachment: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r=ehsan, r=johannh

Depends on D17639

Comment 7

[Andrea Marchesini [:baku]]

Attachment: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r=ehsan, r=johannh

Depends on D17640

Comment 8

[Andrea Marchesini [:baku]]

Attachment: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 2 - rename variables in nsHttpChannel, r=ehsan

Depends on D17637

Comment 9

[Andrea Marchesini [:baku]]

Attachment: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 5 - generalize content blocking notification, r=ehsan

Depends on D17639

Comment 10

[Andrea Marchesini [:baku]]

Attachment: Bug 1522210 - Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - no Tuple<> in ContentBlockingLog, r?ehsan

Depends on D17838

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00494efd99e7
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 1 - UrlClassifierFeatureFactory::IsClassifierBlockingErrorCode, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/1e7503dad26a
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 2 - rename variables in nsHttpChannel, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/c5f891b63342
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 3 - Canceling nsIChannel with error code, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/41bc4923fc47
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 4 - abstract blocking state codes, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/95acf4cd115f
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 5 - generalize content blocking notification, r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/8534212acce3
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r=Ehsan,johannh
https://hg.mozilla.org/integration/autoland/rev/22fd084b4286
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r=Ehsan,johannh

Comment 12

[Dorel Luca [:dluca]]

Backed out 7 changesets (bug 1522210) for build bustage. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=225028471&repo=autoland&lineNumber=13489

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=22fd084b4286e33257f6e9b2dd3589d16c60e5bd

Backout:
https://hg.mozilla.org/integration/autoland/rev/7def006b54215a2dc66248504286db39f981ff95

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d60130501cfe
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 1 - UrlClassifierFeatureFactory::IsClassifierBlockingErrorCode, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/72bf2b10aec9
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 2 - rename variables in nsHttpChannel, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a74ac91a0c08
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 3 - Canceling nsIChannel with error code, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8899b62062f
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 4 - abstract blocking state codes, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed3cfd2e655
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 5 - generalize content blocking notification, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa151834287d
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 6 - fingerprinting, r=ehsan,johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/1705824aa93e
Fingerprinting and cryptomining classifiers must have separate nsIWebProgressListener blocking state codes - part 7 - cryptomining, r=ehsan,johannh

Comment 14

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/d60130501cfe
https://hg.mozilla.org/mozilla-central/rev/72bf2b10aec9
https://hg.mozilla.org/mozilla-central/rev/a74ac91a0c08
https://hg.mozilla.org/mozilla-central/rev/a8899b62062f
https://hg.mozilla.org/mozilla-central/rev/2ed3cfd2e655
https://hg.mozilla.org/mozilla-central/rev/fa151834287d
https://hg.mozilla.org/mozilla-central/rev/1705824aa93e