1484876 - Refactor the code responsible for checking whether the top window URI is on the content blocking allow list into AntiTrackingCommon

Status: RESOLVED FIXED

(Firefox :: Protections UI, enhancement, P2)

Description

[(no longer active)]

This is the required refactoring for bug 1484788.

Comment 1

[(no longer active)]

Attachment: Part 1: Refactor the PrivateBrowsingTrackingProtectionWhitelist service on top of the permission manager

Session permissions aren't persisted to disk, so we can easily add a
new permission type and only store session permissions in the permission
manager database, and drop the in-memory allow list that this service
maintains.

This has the advantage that the permission manager already has the IPC
machinery to make this information available in the content processes,
so we can also check this allow list in the content process.

Comment 2

[(no longer active)]

Attachment: Part 2: Check the trackingprotection-pb permission in the channel classifier code instead of accessing nsIPrivateBrowsingTrackingProtectionWhitelist

Comment 3

[(no longer active)]

Attachment: Part 3: Refactor the code responsible for checking whether the top window URI is on the content blocking allow list into AntiTrackingCommon

Comment 4

[(no longer active)]

Part 1 depends on the API added in bug 1484868.

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 9002632 [details] [diff] [review]
Part 1: Refactor the PrivateBrowsingTrackingProtectionWhitelist service on top of the permission manager

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

Thanks!

Comment 6

[(no longer active)]

Attachment: Part 4: Remove nsIPrivateBrowsingTrackingProtectionWhitelist.existsInAllowList()

Comment 7

[(no longer active)]

Attachment: Part 5: Merge the PrivateBrowsingTrackingProtectionWhitelist service with PrivateBrowsingUtils.jsm

Comment 8

[François Marier [:francois]]

Comment on attachment 9002634 [details] [diff] [review]
Part 3: Refactor the code responsible for checking whether the top window URI is on the content blocking allow list into AntiTrackingCommon

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

Looks fine, but I'd like to double-check any changes to the noisy warning to ensure we don't change the behaviour.

::: netwerk/base/nsChannelClassifier.cpp
@@ -469,5 @@
> -      if (permissions == nsIPermissionManager::ALLOW_ACTION) {
> -        if (LOG_ENABLED()) {
> -          nsCString chanSpec = chanURI->GetSpecOrDefault();
> -          chanSpec.Truncate(std::min(chanSpec.Length(), sMaxSpecLength));
> -          LOG(("nsChannelClassifier[%p]: User override on channel[%p] (%s) for %s (%s)",

It would be good to preserve the type of override (normal v. pb) in the log message. It has been useful in the past in debugging some bad behaviours.

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +530,5 @@
> +  // allowlisting to depend on scheme.
> +  nsresult rv = NS_ERROR_FAILURE;
> +  nsCOMPtr<nsIURL> url = do_QueryInterface(aTopWinURI, &rv);
> +  if (NS_FAILED(rv)) {
> +    return rv; // normal for some loads, no need to print a warning

This will now print a warning due to the NS_ENSURE_SUCCESS() after AntiTrackingCommon::IsOnContentBlockingAllowList() in nsChannelClassifier::ShouldEnableTrackingProtectionInternal().

It used to be a very noisy warning (bug 1205138).

Comment 9

[(no longer active)]

(In reply to François Marier [:francois] from comment #8)
> ::: netwerk/base/nsChannelClassifier.cpp
> @@ -469,5 @@
> > -      if (permissions == nsIPermissionManager::ALLOW_ACTION) {
> > -        if (LOG_ENABLED()) {
> > -          nsCString chanSpec = chanURI->GetSpecOrDefault();
> > -          chanSpec.Truncate(std::min(chanSpec.Length(), sMaxSpecLength));
> > -          LOG(("nsChannelClassifier[%p]: User override on channel[%p] (%s) for %s (%s)",
> 
> It would be good to preserve the type of override (normal v. pb) in the log
> message. It has been useful in the past in debugging some bad behaviours.

I couldn't figure out a way to do that without leaking the implementation detail out of AntiTrackingCommon.cpp which I didn't want to do, but earlier today I added logging to that file as well, so now it's easy to log this information.

> ::: toolkit/components/antitracking/AntiTrackingCommon.cpp
> @@ +530,5 @@
> > +  // allowlisting to depend on scheme.
> > +  nsresult rv = NS_ERROR_FAILURE;
> > +  nsCOMPtr<nsIURL> url = do_QueryInterface(aTopWinURI, &rv);
> > +  if (NS_FAILED(rv)) {
> > +    return rv; // normal for some loads, no need to print a warning
> 
> This will now print a warning due to the NS_ENSURE_SUCCESS() after
> AntiTrackingCommon::IsOnContentBlockingAllowList() in
> nsChannelClassifier::ShouldEnableTrackingProtectionInternal().
> 
> It used to be a very noisy warning (bug 1205138).

Nice catch!

Comment 10

[(no longer active)]

Attachment: Part 3: Refactor the code responsible for checking whether the top window URI is on the content blocking allow list into AntiTrackingCommon

Comment 11

[François Marier [:francois]]

Comment on attachment 9002957 [details] [diff] [review]
Part 3: Refactor the code responsible for checking whether the top window URI is on the content blocking allow list into AntiTrackingCommon

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

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +521,5 @@
> +                                                 bool& aIsAllowListed)
> +{
> +  aIsAllowListed = false;
> +
> +  LOG_SPEC(("Deciding whether the user has overridden content blocking for %s",

I'd recommend truncating the URL like in nsChannelClassifier.cpp (see https://hg.mozilla.org/mozilla-central/rev/58c256b60254).

Some of the tracking URLs have enormous query strings and it's super-annoying to scroll past them in the debug logs.

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 9002761 [details] [diff] [review]
Part 4: Remove nsIPrivateBrowsingTrackingProtectionWhitelist.existsInAllowList()

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

Yay!

::: browser/base/content/browser-contentblocking.js
@@ +437,5 @@
>      let hasException = false;
> +    let type = PrivateBrowsingUtils.isBrowserPrivate(gBrowser.selectedBrowser) ?
> +                 "trackingprotection-pb" :
> +                 "trackingprotection";
> +    hasException = Services.perms.testExactPermission(baseURI, type) ==

nit: just do "let hasException" here and remove the line that initializes this

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 9002762 [details] [diff] [review]
Part 5: Merge the PrivateBrowsingTrackingProtectionWhitelist service with PrivateBrowsingUtils.jsm

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

🤩

::: toolkit/modules/PrivateBrowsingUtils.jsm
@@ +28,5 @@
> +  /**
> +   * Remove the provided URI from the list of allowed tracking sites.
> +   *
> +   * @param uri nsIURI
> +   *        The URI to add to the list.

nit: to remove from the list

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c9895ab06c6
Part 1: Refactor the PrivateBrowsingTrackingProtectionWhitelist service on top of the permission manager; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/babf6abc7f4c
Part 2: Check the trackingprotection-pb permission in the channel classifier code instead of accessing nsIPrivateBrowsingTrackingProtectionWhitelist; r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31e39a47704
Part 3: Refactor the code responsible for checking whether the top window URI is on the content blocking allow list into AntiTrackingCommon; r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd2ced689895
Part 4: Remove nsIPrivateBrowsingTrackingProtectionWhitelist.existsInAllowList(); r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1cb63d8c8bb
Part 5: Merge the PrivateBrowsingTrackingProtectionWhitelist service with PrivateBrowsingUtils.jsm; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/e80737d6af55
Part 6: Truncate the tracking URIs in the anti-tracking logs to 128 characters since they may be really long; r=francois

Comment 15

[Cosmin Sabou [:CosminS]]

Backed out for eslint failure on PrivateBrowsingUtils.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=e80737d6af552187ffeb8099c8d6fd936febed5e&tochange=41ebcb085bb01e54c1a7eea9e794cde45afbe734&selectedJob=195293076&filter-searchStr=Linting%20opt%20source-test-mozlint-eslint%20(ES)

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=195293076&repo=mozilla-inbound&lineNumber=241

Backout link: 
https://hg.mozilla.org/integration/mozilla-inbound/rev/41ebcb085bb01e54c1a7eea9e794cde45afbe734

[task 2018-08-22T13:14:16.995Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-08-22T13:19:59.693Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/modules/PrivateBrowsingUtils.jsm:8:1 | 'XPCOMUtils' is defined but never used. (no-unused-vars)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bddf9e4f6f3a
Part 1: Refactor the PrivateBrowsingTrackingProtectionWhitelist service on top of the permission manager; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/66519e95427e
Part 2: Check the trackingprotection-pb permission in the channel classifier code instead of accessing nsIPrivateBrowsingTrackingProtectionWhitelist; r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ffff7652f87
Part 3: Refactor the code responsible for checking whether the top window URI is on the content blocking allow list into AntiTrackingCommon; r=francois
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3668fde5e5
Part 4: Remove nsIPrivateBrowsingTrackingProtectionWhitelist.existsInAllowList(); r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7ecee484b0
Part 5: Merge the PrivateBrowsingTrackingProtectionWhitelist service with PrivateBrowsingUtils.jsm; r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4222536f35f
Part 6: Truncate the tracking URIs in the anti-tracking logs to 128 characters since they may be really long; r=francois

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/bddf9e4f6f3a
https://hg.mozilla.org/mozilla-central/rev/66519e95427e
https://hg.mozilla.org/mozilla-central/rev/5ffff7652f87
https://hg.mozilla.org/mozilla-central/rev/5c3668fde5e5
https://hg.mozilla.org/mozilla-central/rev/9b7ecee484b0
https://hg.mozilla.org/mozilla-central/rev/d4222536f35f