1624268 - Implementing using the HasStoragePermission flag to check storage permission.

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, task, P1)

Description

[Tim Huang[:timhuang]]

We introduce a flag HasStoragePermission' in Bug 1616788 which represents whether the document has the storage permission. This flag has to be updated when we add storage permission.

Comment 1

[Tim Huang[:timhuang]]

There is one thing that is unclear to me.

I don't know if we need to update the hasStoragePermission flag in different tab when we grant the storage permission for a given tracking origin. Let's say, we grant the storage permission for the tracker.com which is loaded under the example.com. If there is another tab that has the same structure, example.com embeds tracker.com. Does the storage permission be considered as granted for the tracker.com in this tab? Or it should take effect after this tab reloads.

Baku and steve, could you provide insight on this?

Comment 2

[Tim Huang[:timhuang]]

According to our discussion, the storage in one tab doesn't have to reflect the storage permission grant from the other tab who has the same configuration. We would expect users to refresh the tab to really get their storage updated in this case. So, we don't really need to update the hasStoragePermission for the other tab.

Comment 3

[Tim Huang[:timhuang]]

Per discussion with Dimi, we don't really need to update the 'hasStoragePermission' flag when allowing the storage access since the 'StorageAccessGranted' cache will handle the updating situation. So, we change this bug to implement using the 'hasStoragePermission' flag to check the storage permission.

Comment 5

[Tim Huang[:timhuang]]

Attachment: Bug 1624268 - Part 1: Add checking the HasStorageAccessGranted() for the ContentBlocking::ShouldAllowAccessFor(channel). r?dimi

We need to check the HasStorageAccessGranted() in the channel version of
the ContentBlocking::ShouldAllowAccessFor(). It's because we will move
to use the flag 'HasStoragePermission' to check the permission. But, this
flag won't get updated if the storage is allowed by the heuristic. So,
we need to rely on the HasStorageAccessGranted() to check the storage
access in the channel check as well.

Comment 6

[Tim Huang[:timhuang]]

Attachment: Bug 1624268 - Part 2: Moving the setting of 'HasStoragePermission' flag to the WindowContext a bit earlier. r?dimi

We need the 'hasStoragePermission' flag for calculating the
HasStorageAccess() value. Right now the setting of the flag is later
than the calculation. So, we move the setting before the calculation.

Depends on D71031

Comment 7

[Tim Huang[:timhuang]]

Attachment: Bug 1624268 - Part 3: Use the 'HasStoragePermission' flag to check the storage permission. r?dimi

We move to use the 'HasStoragePermission' flag to check the storage
permission. For the storage permission check of windows, the flag would
come from the WindowContext. For the check of channels, the flag would
come from the loadInfo of the channel.

Depends on D71032

Comment 8

[Tim Huang[:timhuang]]

Attachment: Bug 1624268 - Part 4: Remove unnecessary code for ContentBlocking::ShouldAllowAccessFor(). r?dimi

As we change to use the 'HasStoragePermission' flag, there are some code
became uncessary. This patch removes those code.

Depends on D71033

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8af99a5ecd54
Part 1: Add checking the HasStorageAccessGranted() for the ContentBlocking::ShouldAllowAccessFor(channel). r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/c947c72829a6
Part 2: Moving the setting of 'HasStoragePermission' flag to the WindowContext a bit earlier. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/06adb66489b8
Part 3: Use the 'HasStoragePermission' flag to check the storage permission. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/d58da2438eee
Part 4: Remove unnecessary code for ContentBlocking::ShouldAllowAccessFor(). r=dimi,baku

Comment 10

[Daniel Varga [:dvarga]]

Backed out 4 changesets (Bug 1624268) for causing brwoser-chrome failure at toolkit/components/antitracking/test/browser/browser_partitionedSharedWorkers.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=298095356&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=d58da2438eee32708ab04a2a2313c9d6a5d27fbe

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298089873&repo=autoland&lineNumber=50228

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=0c70ad5623dc33fc7af66625ebcde34bd73f984e

[task 2020-04-17T13:07:37.679Z] 13:07:37     INFO - TEST-PASS | toolkit/components/antitracking/test/browser/browser_partitionedSharedWorkers.js | We expected 1 connection - true == true - 
[task 2020-04-17T13:07:37.680Z] 13:07:37     INFO - Buffered messages finished
[task 2020-04-17T13:07:37.680Z] 13:07:37     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_partitionedSharedWorkers.js | We expected 2 connection for 3rd party SharedWorker - false == true - got false, expected true (operator ==)
[task 2020-04-17T13:07:37.680Z] 13:07:37     INFO - Stack trace:
[task 2020-04-17T13:07:37.680Z] 13:07:37     INFO - ok@resource://specialpowers/SpecialPowersSandbox.jsm:86:21
[task 2020-04-17T13:07:37.680Z] 13:07:37     INFO - msg@chrome://mochitests/content/browser/toolkit/components/antitracking/test/browser/dynamicfpi_head.js:95:19
[task 2020-04-17T13:07:37.680Z] 13:07:37     INFO - EventListener.handleEvent*@chrome://mochitests/content/browser/toolkit/components/antitracking/test/browser/dynamicfpi_head.js:87:21
[task 2020-04-17T13:07:37.680Z] 13:07:37     INFO - @chrome://mochitests/content/browser/toolkit/components/antitracking/test/browser/dynamicfpi_head.js:68:17
[task 2020-04-17T13:07:37.680Z] 13:07:37     INFO - execute@resource://specialpowers/SpecialPowersSandbox.jsm:140:12
[task 2020-04-17T13:07:37.680Z] 13:07:37     INFO - _spawnTask@resource://specialpowers/SpecialPowersChild.jsm:1729:15
[task 2020-04-17T13:07:37.681Z] 13:07:37     INFO - receiveMessage@resource://specialpowers/SpecialPowersChild.jsm:281:21
[task 2020-04-17T13:07:37.681Z] 13:07:37     INFO - JSWindowActor query*receiveMessage@resource://specialpowers/SpecialPowersParent.jsm:1063:12
[task 2020-04-17T13:07:37.681Z] 13:07:37     INFO - JSWindowActor query*spawn@resource://specialpowers/SpecialPowersChild.jsm:1684:17
[task 2020-04-17T13:07:37.681Z] 13:07:37     INFO - runTest/<@chrome://mochitests/content/browser/toolkit/components/antitracking/test/browser/dynamicfpi_head.js:58:27
[task 2020-04-17T13:07:37.681Z] 13:07:37     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1039:34
[task 2020-04-17T13:07:37.681Z] 13:07:37     INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:1074:11
[task 2020-04-17T13:07:37.681Z] 13:07:37     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:904:14
[task 2020-04-17T13:07:37.681Z] 13:07:37     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:918:23
[task 2020-04-17T13:07:37.681Z] 13:07:37     INFO - Removing the tab

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/601005e66807
Part 1: Add checking the HasStorageAccessGranted() for the ContentBlocking::ShouldAllowAccessFor(channel). r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/8b7137730c6b
Part 2: Moving the setting of 'HasStoragePermission' flag to the WindowContext a bit earlier. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/9d97ffda84cb
Part 3: Use the 'HasStoragePermission' flag to check the storage permission. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/b53169070af1
Part 4: Remove unnecessary code for ContentBlocking::ShouldAllowAccessFor(). r=dimi,baku

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/601005e66807
https://hg.mozilla.org/mozilla-central/rev/8b7137730c6b
https://hg.mozilla.org/mozilla-central/rev/9d97ffda84cb
https://hg.mozilla.org/mozilla-central/rev/b53169070af1