Closed Bug 1603902 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 --- wontfix
firefox71 --- wontfix
firefox72 --- verified
firefox73 --- verified

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file)

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.

Priority: -- → P2
Blocks: 1490811

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

  1. Log in to Google docs using the email address that got access in step 1.
  2. Go to https://docs.google.com/document/d/1xmFHoMDSWHr24a21sW71FL8pi0AmZHOowIpaQiEMnGw/edit to make sure your access works.
  3. 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.
  4. 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
Attachment #9115959 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Pushed by eakhgari@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

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

Flags: needinfo?(ehsan)

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 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

Flags: needinfo?(ehsan)
Attachment #9115959 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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!

Flags: needinfo?(ehsan)
QA Whiteboard: [qa-triaged]

(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!

Flags: needinfo?(ehsan)

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

Flags: needinfo?(ehsan)

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

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

Yes.

Flags: needinfo?(ehsan)

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
Attachment #9115959 - Flags: approval-mozilla-esr68?

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.

Attachment #9115959 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

[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

Flags: needinfo?(ehsan)

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. :-/

Flags: needinfo?(ehsan)
Attachment #9115959 - Flags: approval-mozilla-esr68+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.