1488635 - Add a test with the cookie permission for the image cache case

Status: RESOLVED FIXED

(Firefox :: Protections UI, enhancement, P1)

Description

[(no longer active)]

Our coverage data shows that we currently don't cover the image cache case when cookie permissions are set: https://codecov.io/gh/mozilla/gecko-dev/src/e126996/toolkit/components/antitracking/AntiTrackingCommon.cpp#L609

baku, do you have cycles to take this?

Comment 1

[Andrea Marchesini [:baku]]

Attachment: image_test.patch

Comment 2

[(no longer active)]

Comment on attachment 9006511 [details] [diff] [review]
image_test.patch

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

::: toolkit/components/antitracking/test/browser/browser_imageCache7.js
@@ +2,5 @@
> +
> +/* Setting a custom permission for this website */
> +let uriObj = Services.io.newURI(TEST_DOMAIN);
> +let cp = Cc["@mozilla.org/cookie/permission;1"]
> +           .getService(Ci.nsICookiePermission);

I just made this not work in bug 1488784.  :-)

Please use the permission manager directly instead, like the part 1 patch there does.

Comment 3

[(no longer active)]

Comment on attachment 9006511 [details] [diff] [review]
image_test.patch

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

Actually r- because of the below reason.

::: toolkit/components/antitracking/test/browser/imageCacheWorker.js
@@ +39,5 @@
>        await new Promise(resolve => { img.onload = resolve; });
>        ok(true, "Image 4 loaded");
>      },
>    },
>    null, // cleanup function

Actually, you need to add a cleanup function now, I think, to ensure the added permission is indeed cleared at the end of the test.

Comment 4

[Andrea Marchesini [:baku]]

> Actually, you need to add a cleanup function now, I think, to ensure the
> added permission is indeed cleared at the end of the test.

This is done by imageCacheWorker.js:
https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/toolkit/components/antitracking/test/browser/imageCacheWorker.js#58

Comment 5

[(no longer active)]

Ah right

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf109313acc3
Add a test with the cookie permission for the image cache case, r=ehsan

Comment 7

[Arthur Iakab [arthur_iakab]]

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

Comment 8

[(no longer active)]

Hmm, actually we don't cover these cases in the normal AntiTracking test suite in head.js, do we?