1603902 - Storage access automatic delayed grants may get cancelled if not approved before the StorageAccessPermissionRequest object is cycle collected

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[(no longer active)]

STR:
Go to https://stirring-grease.glitch.me/ and click on "here". Some of the times, the request gets granted, some of the times it gets denied.

This pernosco session captures an instance of the failure and has a notebook that lets you follow the sequence of the events. The problem is that the promise created here is resolved with a time delay, and we have this mistaken call to Cancel() in the destructor of StorageAccessPermissionRequest which means the cycle-collector driven deletion of this object races with the aforementioned promise.

This bug is pretty bad, depending on the cycle collector load and the calculated simulated delay the automatic grant may fail.

Comment 1

[(no longer active)]

Attachment: Bug 1603902 - Remove a race condition where storage access would get denied when a delayed automatic grant is in progress;

Comment 2

[(no longer active)]

Comment on attachment 9115959 [details]
Bug 1603902 - Remove a race condition where storage access would get denied when a delayed automatic grant is in progress;

Beta/Release Uplift Approval Request

• User impact if declined: Users may not be able to sites using the Storage Access API, such as Google docs/spreadsheets/presentations.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Email [e-mail] for access to the test document (https://docs.google.com/document/d/1xmFHoMDSWHr24a21sW71FL8pi0AmZHOowIpaQiEMnGw/edit).

2. Log in to Google docs using the email address that got access in step 1.
3. Go to https://docs.google.com/document/d/1xmFHoMDSWHr24a21sW71FL8pi0AmZHOowIpaQiEMnGw/edit to make sure your access works.
4. Go to https://stirring-grease.glitch.me/ and click on "here". After a short delay you should see "Access Granted" followed by the document in step 3 displayed embedded.
5. Close the browser, open it again and repeat step 4 a few more times to make sure it works every time.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This code removes a call to a function that was originally added by mistake. Nothing depends on this call for correctness purposes, so the risk of removing it should be minimal.
• String changes made/needed: None

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9665f619ffd
Remove a race condition where storage access would get denied when a delayed automatic grant is in progress; r=baku

Comment 4

[Cristian Brindusan [:cbrindusan]]

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

Comment 5

[Julien Cristau [:jcristau]]

Is this a new issue? Or any particular reason we should still uplift this with little runway in beta?

Comment 6

[Steven Englehardt [:englehardt]]

Ehsan is out on PTO for this week and next, so I'll jump in here.

This is a new issue that prevents the Storage Access API from working correctly. In the failure condition, sites that request Storage Access will silently fail to receive it. This can lead to site breakage that impacts some very popular embedded content that uses the Storage Access API. These include Bug 1550899, Facebook integrations, Youtube integrations, or Amazon Pay integrations.

Since the patch is fairly simple, I think it makes sense to uplift it.

Comment 7

[Julien Cristau [:jcristau]]

Comment on attachment 9115959 [details]
Bug 1603902 - Remove a race condition where storage access would get denied when a delayed automatic grant is in progress;

ok, approved for 72.0b11

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d68034e4b542

Comment 9

[Daniel Bodea [:danibodea]]

I have attempted to reproduce this issue in order to also verify the fix, but I could not.
With the steps written in the uplift request (comment 2) on an affected build on Windows 10, this issue could not be seen. The document was always displayed as intended.
Is there something I am missing in order to properly reproduce the issue in question?

Thank you for your contribution!

Comment 10

[(no longer active)]

(In reply to Bodea Daniel [:danibodea] from comment #9)

I have attempted to reproduce this issue in order to also verify the fix, but I could not.
With the steps written in the uplift request (comment 2) on an affected build on Windows 10, this issue could not be seen. The document was always displayed as intended.
Is there something I am missing in order to properly reproduce the issue in question?

I don't know what step failed for you, but I can think of two possible reasons:

• The account you used to log in to Google Docs with didn't have access to the test document as mentioned in comment 2. (I used my own account for testing; I hope there aren't any other requirements for accessing that test document beyond what I mentioned.)
• The privacy.annotate_channels.strict_list.enabled pref was false (which is the case right now on beta). That pref needs to be set to true if it is false on your build.

At any rate I verified the fix on both Nightly and Beta myself, so I don't think any other action on your part is necessary. Thanks for looking into it!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Do we need this on ESR too given the impact shared in comment 6?

Comment 12

[(no longer active)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

Do we need this on ESR too given the impact shared in comment 6?

Yes.

Comment 13

[(no longer active)]

Comment on attachment 9115959 [details]
Bug 1603902 - Remove a race condition where storage access would get denied when a delayed automatic grant is in progress;

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Please see comment 6. This is a bug fix that can affect various websites and is virtually risk-free.
• User impact if declined: See comment 6.
• Fix Landed on Version: 73.
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This code removes a call to a function that was originally added by mistake. Nothing depends on this call for correctness purposes, so the risk of removing it should be minimal.
• String or UUID changes made by this patch: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9115959 [details]
Bug 1603902 - Remove a race condition where storage access would get denied when a delayed automatic grant is in progress;

Fixes possible storage issues on major websites. Approved for 68.5esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/c53c8c952ba8

Comment 16

[Natalia Csoregi [:nataliaCs]]

[Tracking Requested - why for this release]:

Backed out for failures on /browser_blockingCookies.js

backout: https://hg.mozilla.org/releases/mozilla-esr68/rev/f1339a22b68caa07bdd8c51a24572ddc9ba38d7c

push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&searchStr=browser-chrome&revision=737d24fb73b9fc408f39ba6569bed84683925bf3&group_state=expanded&selectedJob=284341792

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284341792&repo=mozilla-esr68&lineNumber=14561

[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO - Console message: [JavaScript Error: "remote browser crashed while on http://example.net/browser/toolkit/components/antitracking/test/browser/page.html
[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO - " {file: "chrome://mochikit/content/mochitest-e10s-utils.js" line: 10}]
[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO - e10s_init/<@chrome://mochikit/content/mochitest-e10s-utils.js:10:10
[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO -
[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO - Buffered messages logged at 06:35:42
[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO - Longer timeout required, waiting longer... Remaining timeouts: 2
[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO - Buffered messages logged at 06:37:12
[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO - Longer timeout required, waiting longer... Remaining timeouts: 1
[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO - Buffered messages finished
[task 2020-01-10T06:38:43.545Z] 06:38:43 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | Test timed out -
[task 2020-01-10T06:38:43.546Z] 06:38:43 INFO - GECKO(5073) | MEMORY STAT | vsize 749MB | residentFast 326MB | heapAllocated 83MB
[task 2020-01-10T06:38:43.546Z] 06:38:43 INFO - TEST-OK | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | took 270075ms
[task 2020-01-10T06:38:43.546Z] 06:38:43 INFO - Not taking screenshot here: see the one that was previously logged
[task 2020-01-10T06:38:43.549Z] 06:38:43 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/antitracking/test/browser/browser_blockingCookies.js | Found a tab after previous test timed out: http://example.net/browser/toolkit/components/antitracking/test/browser/page.html -
[task 2020-01-10T06:38:43.549Z] 06:38:43 INFO - GECKO(5073) | [Child 5191, Main Thread] WARNING: No active window: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 662
[task 2020-01-10T06:38:43.550Z] 06:38:43 INFO - GECKO(5073) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpvGAtH6.mozrunner/runtests_leaks_tab_pid5388.log
[task 2020-01-10T06:38:43.550Z] 06:38:43 INFO - checking window state

Comment 17

[(no longer active)]

This crash shows that on ESR there are cases where we may drop the promise returned from requestStorageAccess() on the floor and never resolve/reject it... Since fixing that would involve more (risky) surgery I am now inclined to say we should probably live with this bug unfixed there. :-/