Cc: slangley@chromium.org noel@chromium.org Status: Started (was: Assigned)
From reading through content/browser/quota_dispatcher_host.cc (browser side) and content/renderer/quota_dispatcher.cc (renderer side), there are two sequences of calls:
1) Renderer requests storage quota from the browser
(a) QuotaDispatcher (renderer) --sends--> RequestStorageQuota --to--> QuotaDispatcherHost (browser)
- Parameters: Render Frame ID, Request ID, Origin URL, Storage Type, Requested Size
(b) QuotaDispatcherHost::OnRequestStorageQuota() is invoked
(c) If type is not supported, QuotaDispatcherHost --sends--> DidFail --to--> QuotaDispatcher
- Parameters: Request ID, kQuotaErrorNotSupported
(d) If quota request is not granted for some other reason, QuotaDispatcherHost --sends--> DidFail --to--> QuotaDispatcher
- Parameters: Request ID, Status
(e) If quota request is granted, QuotaDispatcherHost --sends--> DidGrantStorageQuota --to--> QuotaDispatcher
- Parameters: Request ID, Usage, Granted Quota
2) Renderer queries storage usage and quota from the browser
(a) QuotaDispatcher (renderer) --sends--> QueryStorageUsageAndQuota --to--> QuotaDisparcherHost (browser)
- Parameters: Request ID, Origin URL, Type
(b) QuotaDispatcherHost::OnQueryStorageUsageAndQuota() is invoked
(c) If the request fails, QuotaDispatcherHost --sends--> DidFail --to--> QuotaDispatcher
- Parameters: Request ID, Status
(d) If the request succeeds, QuotaDispatcherHost --sends--> DidGrantStorageQuota --to--> QuotaDispatcher
- Parameters: Request ID, Usage, Granted Quota
This can be recreated with just 2 mojo messages:
1) RequestStorageQuota
Parameters:
- Request ID*
- Render Frame ID^
- Origin URL^
- Storage Type
- Requested Size
Callback:
- Status**
- Usage (optional)
- Granted Quota (optional)
(the optional arguments are only on success)
2) RequestStorageUsageAndQuota
Parameters:
- Request ID*
- Origin URL^
- Type
Callback:
- Status**
- Usage (optional)
- Granted Quota (optional)
Notes:
* The Request ID may only be used for IPCs to keep track of which messages are being sent to which callers. Since mojo handles this automatically, as long as it isn’t used anywhere else, it can be removed.
^ Currently, the IPC connection is from the browser process to the renderer process. After converting the IPC to mojo, we will move the mojo pipe to be from the render frame itself to the render frame host. This way we don’t need to rely on potentially malicious values from the renderer such as URLs, instead mojo will know exactly which frame it’s talking to and can get the URL itself.
** The status return value (currently an int/enum QuotaStatusCode) is only used for StorageQuotaCallbacks::DidFail, and the only implementations convert it to an enum WebStorageQuotaError. So there is potential to make it a WebStorageQuotaError optional parameter, set only if the callback failed (and empty if it succeeded).
Starting work on this now. :)
Looking at https://chromium-review.googlesource.com/c/chromium/src/+/758194, a bunch of tests fail when the IPCs are modified:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/582413
Unexpected Failures Layout tests:
* external/wpt/storage/estimate-indexeddb-worker.https.html
* external/wpt/storage/estimate-indexeddb.https.html
* external/wpt/storage/opaque-origin.https.html
* external/wpt/storage/storagemanager-estimate.https.html
* storage/quota/storageinfo-no-callbacks.html
* storage/quota/storageinfo-query-usage.html
* storage/quota/storageinfo-request-quota.html
* storage/quota/storagequota-query-usage.html
* storage/quota/storagequota-request-quota.html
Browser Tests:
WebViewTests/WebViewTest.FileSystemAPIRequestFromMainThreadDefaultAllow/1
WebViewTests/WebViewTest.FileSystemAPIRequestFromMainThreadDefaultAllow/0
WebViewTests/WebViewTest.FileSystemAPIRequestFromMainThreadDeny/1
WebViewTests/WebViewTest.FileSystemAPIRequestFromMainThreadDeny/0
WebViewTests/WebViewTest.FileSystemAPIRequestFromMainThreadAllow/1
WebViewTests/WebViewTest.FileSystemAPIRequestFromMainThreadAllow/0
ExtensionApiTest.RequestQuotaInBackgroundPage
And others. So this IPC call should be safe to modify without adding additional test coverage.
Nice analysis, and there is test coverage (so we're good to go). Re: mojo naming
> 1) Renderer requests storage quota from the browser
mojo::RequestStorageQuota
> 2) Renderer queries storage usage and quota from the browser
mojo::QueryStorageUsageAndQuota
The following patches are in the process of landing, in order:
1. Move StorageType to WebKit/common - https://chromium-review.googlesource.com/826827
2. Move quota_types.mojom to WebKit/common - https://chromium-review.googlesource.com/826524
3. Move quota_dispatcher_host.mojom to WebKit/common - https://chromium-review.googlesource.com/826525
4. Remove WebStorageQuotaType - https://chromium-review.googlesource.com/828640
5. Remove WebStorageQuotaError - https://chromium-review.googlesource.com/828224
6. Remove QuotaDispatcherCallbacks - https://chromium-review.googlesource.com/833438
After that last patch is the main Onion Soup patches, which involve (may all have to be merged into one patch):
7. Removing Platform::QueryStorageAndQuota(), calling it directly with mojo
8. Removing WebFrameClient::RequestStorageQuota(), calling it directly with mojo
9. Moving QuotaDispatcherHost to be per-frame and per-worker, rather than per-process
a) Plumb the matching interface provider through to QuotaDispatcher
b) Change the lifetime of QuotaDispatcher to be a Supplement<ExecutionContext>, rather than per-thread
c) Move the Mojo registration code to RendererInterfaceBinders as well as RenderFrameHostImpl
(A WIP attempt at this patch is here - https://chromium-review.googlesource.com/c/chromium/src/+/807724)
After that, later cleanup todos involve:
10. Remove quota_status_code.h and storage_type.h and the typemaps file, using the generated mojo types instead
11. Moving StorageQuotaCallbacks to WebKit/common, removing WebStorageQuotaCallbacks and using this instead (and later using mojo callbacks)
12. Remove QuotaDispatcherHost altogether, moving logic to the callsite
Since my team is moving to ChromeOS, I'll work on landing patches 1 through 6, leaving whoever is available to pick up from where I left off :)
Once Patch 6 is landed, I'll unassign myself as owner of this bug.
Comment 1 by sashab@google.com
, Nov 10 2017Status: Started (was: Assigned)
From reading through content/browser/quota_dispatcher_host.cc (browser side) and content/renderer/quota_dispatcher.cc (renderer side), there are two sequences of calls: 1) Renderer requests storage quota from the browser (a) QuotaDispatcher (renderer) --sends--> RequestStorageQuota --to--> QuotaDispatcherHost (browser) - Parameters: Render Frame ID, Request ID, Origin URL, Storage Type, Requested Size (b) QuotaDispatcherHost::OnRequestStorageQuota() is invoked (c) If type is not supported, QuotaDispatcherHost --sends--> DidFail --to--> QuotaDispatcher - Parameters: Request ID, kQuotaErrorNotSupported (d) If quota request is not granted for some other reason, QuotaDispatcherHost --sends--> DidFail --to--> QuotaDispatcher - Parameters: Request ID, Status (e) If quota request is granted, QuotaDispatcherHost --sends--> DidGrantStorageQuota --to--> QuotaDispatcher - Parameters: Request ID, Usage, Granted Quota 2) Renderer queries storage usage and quota from the browser (a) QuotaDispatcher (renderer) --sends--> QueryStorageUsageAndQuota --to--> QuotaDisparcherHost (browser) - Parameters: Request ID, Origin URL, Type (b) QuotaDispatcherHost::OnQueryStorageUsageAndQuota() is invoked (c) If the request fails, QuotaDispatcherHost --sends--> DidFail --to--> QuotaDispatcher - Parameters: Request ID, Status (d) If the request succeeds, QuotaDispatcherHost --sends--> DidGrantStorageQuota --to--> QuotaDispatcher - Parameters: Request ID, Usage, Granted Quota This can be recreated with just 2 mojo messages: 1) RequestStorageQuota Parameters: - Request ID* - Render Frame ID^ - Origin URL^ - Storage Type - Requested Size Callback: - Status** - Usage (optional) - Granted Quota (optional) (the optional arguments are only on success) 2) RequestStorageUsageAndQuota Parameters: - Request ID* - Origin URL^ - Type Callback: - Status** - Usage (optional) - Granted Quota (optional) Notes: * The Request ID may only be used for IPCs to keep track of which messages are being sent to which callers. Since mojo handles this automatically, as long as it isn’t used anywhere else, it can be removed. ^ Currently, the IPC connection is from the browser process to the renderer process. After converting the IPC to mojo, we will move the mojo pipe to be from the render frame itself to the render frame host. This way we don’t need to rely on potentially malicious values from the renderer such as URLs, instead mojo will know exactly which frame it’s talking to and can get the URL itself. ** The status return value (currently an int/enum QuotaStatusCode) is only used for StorageQuotaCallbacks::DidFail, and the only implementations convert it to an enum WebStorageQuotaError. So there is potential to make it a WebStorageQuotaError optional parameter, set only if the callback failed (and empty if it succeeded). Starting work on this now. :)