1539641 - Surface storage access API exceptions in the content blocking log

Status: RESOLVED FIXED

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

Description

[Steven Englehardt [:englehardt]]

Document::mContentBlockingLog does not currently include any information about why a blocking exception is granted. There are currently three causes https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/toolkit/components/antitracking/AntiTrackingCommon.h#70.

Surfacing this information might be helpful when debugging, and could also be collected in Origin Telemetry.

Comment 1

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1539641-Part-1-Log-reason.patch

Hi Baku, do you think the patch is on the right direction?

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 9055998 [details] [diff] [review]
0001-Bug-1539641-Part-1-Log-reason.patch

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

::: dom/base/ContentBlockingLog.h
@@ +95,5 @@
>        }
>        if (!entry.mData->mLogs.IsEmpty()) {
>          auto& last = entry.mData->mLogs.LastElement();
> +        if (last.mType == aType && last.mBlocked == aBlocked &&
> +            last.mReason == aReason) {

I don't think we care about comparing the reason. Keep the if() as it was.
We need to decide if we want to 'overwrite' the reason to keep the last or the first one.
I would keep the first one.

Comment 3

[(no longer active)]

Gary, I think this could be done in a much cleaner way.

Here I assume aReason is StorageAccessGrantedReason, right? You actually don't need to pass it down to all of the ContentBlockingLog recording methods like you are doing now. StorageAccessGrantedReason is only meaningful when storage access is being granted (note the name of the type!). That means you only need to pass it down from here https://searchfox.org/mozilla-central/rev/6db0a6a56653355fcbb25c4fa79c6e7ffc6f88e9/toolkit/components/antitracking/AntiTrackingCommon.cpp#871 and for STATE_COOKIES_BLOCKED_TRACKER. That means you only need to modify Document::SetHasTrackingCookiesBlocked() and none of the other similar methods on Document. Inside that method you should probably assert that Nothing() is always passed when aBlocked != false.

I'd like to review your final patch here, if possible. Thanks!

Comment 4

[Gary Chen [:xeonchen]]

Attachment: Bug 1539641 - Log and report storage access granted reason;

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3b55b97b9ae
Log and report storage access granted reason; r=Ehsan,chutten

Comment 6

[Stefan Hindli [:stefan_hindli]]

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