Closed Bug 1645234 Opened 5 years ago Closed 5 years ago

requestStorageAccess really needs a lot more console warnings

Categories

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

Product:
Core
Component:
Privacy: Anti-Tracking
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla80
Tracking Flags:
Tracking Status
firefox80 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

Bug 1645234 - Add more warnings to document.requestStorageAccess(). r=baku!,annevk,englehardt
47 bytes, text/x-phabricator-request
Details | Review
Bug 1645234 - Add learn more link to storage access API warnings. r=nchevobbe
47 bytes, text/x-phabricator-request
Details | Review
Bug 1645234 - Add a test for document.requestStorageAccess error messages. r=nchevobbe
47 bytes, text/x-phabricator-request
Details | Review

It's really tedious to build something with requestStorageAccess and have absolutely no understanding of why calls are failing, since we only reject with undefined.

We should probably take proper error states to the spec level, but in the meantime we should just add better console warnings to our implementation.

See discussion in https://github.com/privacycg/storage-access/issues/37 about changing undefined to a proper exception. I suspect that we want to reject with a single value going forward as well, but we should definitely have optimal information for web developers debugging locally.

(In reply to Anne (:annevk) from comment #1)

See discussion in https://github.com/privacycg/storage-access/issues/37 about changing undefined to a proper exception. I suspect that we want to reject with a single value going forward as well, but we should definitely have optimal information for web developers debugging locally.

I see, thanks! I had expected that there might be concerns like this but I didn't realize there had been a discussion on the standard.

This shouldn't matter for console logging though, right? Since that's not perceivable by the site, IIUC.

Correct.

Blocks: dom-error

The only common failure case that's not being warned about now is when the user
rejected the prompt, which I think is expected behavior.

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e66593529bc3 Add more warnings to document.requestStorageAccess(). r=annevk,englehardt,baku https://hg.mozilla.org/integration/autoland/rev/16212aa46253 Add learn more link to storage access API warnings. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/75276e64701b Add a test for document.requestStorageAccess error messages. r=nchevobbe

Dimi since this was backed out an Johann is now on leave, would you mind to help get this landed?

Flags: needinfo?(jhofmann) → needinfo?(dlee)

(In reply to Steven Englehardt [:englehardt] from comment #9)

Dimi since this was backed out an Johann is now on leave, would you mind to help get this landed?

sure

Flags: needinfo?(dlee)
Pushed by dlee@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/87d5d468c7f2 Add more warnings to document.requestStorageAccess(). r=annevk,englehardt,baku https://hg.mozilla.org/integration/autoland/rev/6de9007aa306 Add learn more link to storage access API warnings. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/ad1f8a4d64cf Add a test for document.requestStorageAccess error messages. r=nchevobbe
Flags: needinfo?(jhofmann) → needinfo?(dlee)
Pushed by dlee@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a90a3c1efdc5 Add more warnings to document.requestStorageAccess(). r=annevk,englehardt,baku https://hg.mozilla.org/integration/autoland/rev/f74844ba692a Add learn more link to storage access API warnings. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/6097753dbefa Add a test for document.requestStorageAccess error messages. r=nchevobbe

Backed out for causing leaks

backout: https://hg.mozilla.org/integration/autoland/rev/b8ab2990c94f62fc6f16c406671231917e346145

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=windows%2C10%2Cx64%2Cdebug%2Cmochitests%2Ctest-windows10-64%2Fdebug-mochitest-devtools-chrome-e10s-3%2Cm%28dt3%29&revision=6097753dbefa0311b6bd614dea425d441ed93071

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309206837&repo=autoland&lineNumber=29838

[task 2020-07-09T22:43:43.085Z] 22:43:43 INFO -
[task 2020-07-09T22:43:43.086Z] 22:43:43 INFO - nsTraceRefcnt::DumpStatistics: 1444 entries
[task 2020-07-09T22:43:43.087Z] 22:43:43 INFO - TEST-INFO | leakcheck | tab leaked 47 DocAccessibleChild
[task 2020-07-09T22:43:43.087Z] 22:43:43 INFO - TEST-INFO | leakcheck | tab leaked 47 DocAccessibleChildBase
[task 2020-07-09T22:43:43.087Z] 22:43:43 INFO - TEST-INFO | leakcheck | tab leaked 1 Mutex
[task 2020-07-09T22:43:43.088Z] 22:43:43 INFO - TEST-INFO | leakcheck | tab leaked 47 PDocAccessibleChild
[task 2020-07-09T22:43:43.089Z] 22:43:43 INFO - TEST-INFO | leakcheck | tab leaked 47 nsTArray_base
[task 2020-07-09T22:43:43.089Z] 22:43:43 INFO - TEST-UNEXPECTED-FAIL | leakcheck | tab 7600 bytes leaked (DocAccessibleChild, DocAccessibleChildBase, Mutex, PDocAccessibleChild, nsTArray_base)
[task 2020-07-09T22:43:43.089Z] 22:43:43 INFO -

(In reply to Natalia Csoregi [:nataliaCs] from comment #14)

Backed out for causing leaks

just an update that I'm still working on fixing this leak issue.

Blocks: 1653057
Pushed by dlee@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/26991249097c Add more warnings to document.requestStorageAccess(). r=annevk,englehardt,baku https://hg.mozilla.org/integration/autoland/rev/5e37f27f9f9e Add learn more link to storage access API warnings. r=nchevobbe https://hg.mozilla.org/integration/autoland/rev/39b4605c1178 Add a test for document.requestStorageAccess error messages. r=nchevobbe
Flags: needinfo?(dlee)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: