Closed Bug 1539641 Opened 9 months ago Closed 8 months ago

Surface storage access API exceptions in the content blocking log

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: englehardt, Assigned: xeonchen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.

See Also: → 1539536

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

Attachment #9055998 - Flags: feedback?(amarchesini)
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.
Attachment #9055998 - Flags: feedback?(amarchesini) → feedback+
Assignee: nobody → xeonchen
Priority: -- → P2

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!

Blocks: 1543114
Pushed by xeonchen@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c3b55b97b9ae
Log and report storage access granted reason; r=Ehsan,chutten
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.