1524938 - Get rid of nsContentUtils::StorageAllowedForPrincipal()

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement)

Description

[Andrea Marchesini [:baku]]

nsContentUtils::StorageAllowedForPrincipal() doesn't check if there are window exceptions, and permission for the current document. We should use nsContentUtils::StorageAllowedForWindow() instead.

I'm removing nsContentUtils::StorageAllowedForPrincipal() from our codebase, there are only 2 remaining uses: remoteWorker and serviceWorker.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: Bug 1524938 - RemoteWorker doesn't need to use nsContentUtils::StorageAllowedForPrincipal(), r=asuth

Depends on D18528

Comment 2

[Andrea Marchesini [:baku]]

Attachment: Bug 1524938 - nsContentUtils::StorageAllowedForPrincipal() renamed to nsContentUtils::StorageAllowedForServiceWorker(), r=asuth

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edd013998dc7
RemoteWorker doesn't need to use nsContentUtils::StorageAllowedForPrincipal(), r=asuth
https://hg.mozilla.org/integration/autoland/rev/9fb30b90e90d
nsContentUtils::StorageAllowedForPrincipal() renamed to nsContentUtils::StorageAllowedForServiceWorker(), r=asuth

Comment 4

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1524938) for Browser-chrome failures in dom/indexedDB/test/browser_private_idb.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=226033831&repo=autoland&lineNumber=4216

INFO - TEST-START | dom/indexedDB/test/browser_private_idb.js
23:23:07 INFO - GECKO(8728) | console.log: "opening db"
23:23:07 INFO - GECKO(8728) | console.log: "db req completed:" "created"
23:23:07 INFO - GECKO(8728) | console.log: "deleting database"
23:23:07 INFO - GECKO(8728) | console.log: "deleted database"
23:23:07 INFO - GECKO(8728) | console.log: "opening db"
23:23:07 INFO - GECKO(8728) | console.log: "db req completed:" "error"
23:23:08 INFO - GECKO(8728) | console.log: "worker completed test with result:" "created"
23:23:08 INFO - GECKO(8728) | console.log: "worker completed test with result:" "error"
23:23:08 INFO - GECKO(8728) | console.log: "worker completed test with result:" "created"
23:23:11 INFO - GECKO(8728) | [Parent 5788, Gecko_IOThread] WARNING: pipe error: 109: file z:/build/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 332
23:23:18 INFO - GECKO(8728) | [Parent 5788, Gecko_IOThread] WARNING: pipe error: 109: file z:/build/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 332
23:23:19 INFO - GECKO(8728) | [Parent 5788, Gecko_IOThread] WARNING: pipe error: 109: file z:/build/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 332
23:23:51 INFO - TEST-INFO | started process screenshot
23:23:51 INFO - TEST-INFO | screenshot: exit 0
23:23:51 INFO - Buffered messages logged at 23:23:06
23:23:51 INFO - Entering test bound
23:23:51 INFO - Buffered messages logged at 23:23:07
23:23:51 INFO - TEST-PASS | dom/indexedDB/test/browser_private_idb.js | IndexedDB works in a non-private-browsing page. -
23:23:51 INFO - TEST-PASS | dom/indexedDB/test/browser_private_idb.js | IndexedDB does not work in a private-browsing page. -
23:23:51 INFO - Buffered messages logged at 23:23:08
23:23:51 INFO - TEST-PASS | dom/indexedDB/test/browser_private_idb.js | IndexedDB works in a non-private-browsing Worker. -
23:23:51 INFO - TEST-PASS | dom/indexedDB/test/browser_private_idb.js | IndexedDB does not work in a private-browsing Worker. -
23:23:51 INFO - TEST-PASS | dom/indexedDB/test/browser_private_idb.js | IndexedDB works in a non-private-browsing SharedWorker. -
23:23:51 INFO - Buffered messages finished
23:23:51 INFO - TEST-UNEXPECTED-FAIL | dom/indexedDB/test/browser_private_idb.js | Test timed out -
23:23:51 INFO - GECKO(8728) | MEMORY STAT | vsize 1844MB | vsizeMaxContiguous 67804079MB | residentFast 275MB | heapAllocated 81MB
23:23:51 INFO - TEST-OK | dom/indexedDB/test/browser_private_idb.js | took 45033ms
23:23:51 INFO - checking window state
23:23:51 INFO - Not taking screenshot here: see the one that was previously logged
23:23:51 INFO - TEST-UNEXPECTED-FAIL | dom/indexedDB/test/browser_private_idb.js | Found a browser window after previous test timed out -
23:23:51 INFO - Not taking screenshot here: see the one that was previously logged
23:23:51 INFO - TEST-UNEXPECTED-FAIL | dom/indexedDB/test/browser_private_idb.js | Found a browser window after previous test timed out -
23:23:51 INFO - GECKO(8728) | must wait for focus
23:23:52 INFO - GECKO(8728) | [Parent 5788, Gecko_IOThread] WARNING: pipe error: 109: file z:/build/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 332
23:23:52 INFO - GECKO(8728) | [Parent 5788, Gecko_IOThread] WARNING: pipe error: 109: file z:/build/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 332
23:23:52 INFO - GECKO(8728) | JavaScript error: blob:http://example.com/7d71c6ae-a148-4f74-a82f-38f7770d3dee, line 10: ReferenceError: content is not defined
23:23:52 INFO - GECKO(8728) | JavaScript error: blob:http://example.com/7d71c6ae-a148-4f74-a82f-38f7770d3dee, line 10: ReferenceError: content is not defined
23:23:52 INFO - Console message: [JavaScript Error: "ReferenceError: content is not defined" {file: "blob:http://example.com/7d71c6ae-a148-4f74-a82f-38f7770d3dee" line: 10}]
23:23:52 INFO - Console message: [JavaScript Error: "ReferenceError: content is not defined" {file: "blob:http://example.com/7d71c6ae-a148-4f74-a82f-38f7770d3dee" line: 10}]
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 6840
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 7992
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 10136
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 9632
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 7416
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 7648
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 10012
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 7624
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 6948
23:23:53 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 4992
23:23:54 INFO - GECKO(8728) | Completed ShutdownLeaks collections in process 5788
23:23:54 INFO - TEST-START | Shutdown
23:23:54 INFO - Browser Chrome Test Summary
23:23:54 INFO - Passed: 38
23:23:54 INFO - Failed: 3
23:23:54 INFO - Todo: 0
23:23:54 INFO - Mode: e10s
23:23:54 INFO - *** End BrowserChrome Test Results ***

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9fb30b90e90db6e9f3668dc69a1c4b1943a72504

Backout:
https://hg.mozilla.org/integration/autoland/rev/460aa3f0ead5bc21e8b6752db78bf2a9d663e830

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8484cf3c9879
RemoteWorker doesn't need to use nsContentUtils::StorageAllowedForPrincipal(), r=asuth
https://hg.mozilla.org/integration/autoland/rev/a69b39fc23e5
nsContentUtils::StorageAllowedForPrincipal() renamed to nsContentUtils::StorageAllowedForServiceWorker(), r=asuth

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

:baku, can you explain the change from comparing against ePrivateBrowsing to eDeny? That's a major change to make to a patch that was previously just moving code, and with only the note "This should be > eDeny!".

Comment 7

[Andrea Marchesini [:baku]]

The check before my patch was this:
https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/dom/workers/remoteworkers/RemoteWorkerChild.cpp#270-274

And this was reporting the result of:
https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/dom/base/nsContentUtils.cpp#8360

As you see, ePrivateBrowsing is a valid return value only in case we have a window:
https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/dom/base/nsContentUtils.cpp#8377,8385-8387

So, using the principal only, the value was already > "eDeny".
Now, let's compare similar checks, in worker-land: in workerPrivate, we allow storage access only if:
https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/dom/workers/WorkerPrivate.cpp#2431-2433

So, before my patches, in private-browsing, a SharedWorker was able to do |self.indexedDB.open(...)| because, storageAccess was allowed (in normal cookie configuration). With my patches, and without that fix, it was not because of:
https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/dom/workers/WorkerScope.cpp#392-396

So, we should be consistent in any worker scope and use > eDeny everywhere (as we always did, actually).
Maybe would be nice to centralize the check in a: |static bool WorkerPrivate::IsStorageAccessAllowed(nsContentUtils::StorageAccess);|

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8484cf3c9879
https://hg.mozilla.org/mozilla-central/rev/a69b39fc23e5

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Fantastic explanation and analysis, thank you! I'm very glad to have that documented here on the bug.

Agreed that it does make sense to use eDeny. ePrivateBrowsing is a bit awkward in the enum. (As is ePartitionedOrDeny, but that's so specialized that it's somewhat less of a concern.) Unfortunately we have a few dimensions flattened into a single ordering in the StorageAccess enum. (I hadn't realized it was originally proposed at https://bugzilla.mozilla.org/show_bug.cgi?id=1184973#c8 to be an EnumSet, but that's still not perfect either.)