Bug 46648 – Support different local and session storage quota for different security origins.

• Status: ASSIGNED
• Component: WebCore JavaScript

[Lyon Chen]

Based on http://dev.w3.org/html5/webstorage/#disk-space, the local storage should also have total amount limit and per origin limit. Patch for https://bugs.webkit.org/show_bug.cgi?id=43794 make local storage quota a per page group setting. We need to make a thing similar to DatabaseTracker (LocalStorageTracker?), so we can track the local storage usage for a security origin, even it has multiple pages opened at the same time.

[Jeremy Orlow]

You're confusing 2 things.  LocalStorage already tracks usage on a per-origin basis.  The thing we don't do is allow different quotas for different origins.

Any code should apply to session storage and make sure it's easy to set a global default (that's overridden by the per-origin limits).

[Lyon Chen]

(In reply to comment #1)
> You're confusing 2 things.  LocalStorage already tracks usage on a per-origin basis.  The thing we don't do is allow different quotas for different origins.
> 
> Any code should apply to session storage and make sure it's easy to set a global default (that's overridden by the per-origin limits).

You are right, we just need to code change to support different quota for different security origins.

[Lyon Chen]

Created attachment 193910 [details]
patch for 46648.

[Lyon Chen]

Created attachment 193922 [details]
patch fixing merge error in StorageNamespace.cpp.

[WebKit Review Bot]

Comment on attachment 193922 [details]
patch fixing merge error in StorageNamespace.cpp.

Attachment 193922 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17160695

[Build Bot]

Comment on attachment 193922 [details]
patch fixing merge error in StorageNamespace.cpp.

Attachment 193922 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17122744

[WebKit Review Bot]

Comment on attachment 193922 [details]
patch fixing merge error in StorageNamespace.cpp.

Attachment 193922 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17235164

[Peter Beverloo (cr-android ews)]

Comment on attachment 193922 [details]
patch fixing merge error in StorageNamespace.cpp.

Attachment 193922 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17209516

[Lyon Chen]

Created attachment 193969 [details]
patch fixing cr-xxx buildbot errors.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Lyon Chen]

Created attachment 193971 [details]
patch without changing chromium public API.

[Build Bot]

Comment on attachment 193971 [details]
patch without changing chromium public API.

Attachment 193971 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17205717

[WebKit Review Bot]

Comment on attachment 193971 [details]
patch without changing chromium public API.

Attachment 193971 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17187613

New failing tests:
inspector/storage-panel-dom-storage.html
inspector/storage-panel-dom-storage-update.html

[Adam Barth]

Comment on attachment 193971 [details]
patch without changing chromium public API.

View in context: https://bugs.webkit.org/attachment.cgi?id=193971&action=review

> Source/WebCore/page/ChromeClient.h:192
> +        enum StorageType {

bad indent

> Source/WebCore/page/ChromeClient.h:193
> +            StorageLocal = 0,

No need for = 0 here.  The compiler will handle that for you.

> Source/WebCore/page/ChromeClient.h:198
> +        virtual bool shouldAllowStorageRequest(StorageType, const SecurityOrigin&, unsigned quota) = 0;

const SecurityOrigin& -> SecurityOrigin*

We pass SecurityOrigin objects by pointer, not by reference.

Why is this function on the ChromeClient?  It has nothing to do with the browser's chrome.

> Source/WebCore/page/DOMWindow.cpp:771
> +    if (!storageArea.get()) {

There's no need to call get() here.

> Source/WebCore/page/DOMWindow.cpp:773
> +        if (!page->chrome() || !page->chrome()->client())
> +            return 0;

How can these ever be 0?

[Lyon Chen]

(In reply to comment #14)

Thanks for the review, will update accordingly.

For page->chrome() and page->chrome()->client(), I am not sure whether it is possible for any of them be null.

As for adding shouldAllowStorageRequest() to ChromeClient, the purpose is to give ChromeClient a chance to decide whether it wants to grant the storage request or not. And in portal specific ChromeClient implementation, different portal can impose the storage usage limit, based on per SecurityOrigin, and total storage usage limit, etc.

As for ChromeClient is not related to the browser chrome (I think page->chrome() is the browser chrome before we have multiple tab support in browser?), I am not sure why it matters here.

And more importantly this ChromeClient does provide the similar interface, like exceededDatabaseQuota(). So I feel this is the right place to put shouldAllowStorageRequest().

[Lyon Chen]

Created attachment 194110 [details]
patch fixing test regression and mac build failure.

[Lyon Chen]

Created attachment 194173 [details]
patch fixing mac-wk2 code issue that fails the tests.

[Build Bot]

Comment on attachment 194173 [details]
patch fixing mac-wk2 code issue that fails the tests.

Attachment 194173 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17203432

New failing tests:
fast/events/backspace-navigates-back.html
fast/history/history-back-forward-within-subframe-hash.html
fast/css/preserve-user-specified-zoom-level-on-reload.html
fast/dom/everything-to-string.html
http/tests/inspector/change-iframe-src.html
http/tests/cache/loaded-from-cache-after-reload.html
http/tests/inspector/console-resource-errors.html
fast/dom/Window/HTMLFrameSetElement-window-eventListener-attributes.html
fast/history/history-back-initial-vs-final-url.html
fast/dom/constructed-objects-prototypes.html
http/tests/history/replacestate-post-to-get.html
editing/pasteboard/smart-paste-001.html
fast/hidpi/image-set-as-background.html
fast/events/popup-blocked-from-history-reload.html
http/tests/cache/loaded-from-cache-after-reload-within-iframe.html
http/tests/inspector/compiler-script-mapping.html
fast/hidpi/image-set-background-repeat-without-size.html
fast/frames/layout-after-destruction.html
fast/dom/Window/HTMLBodyElement-window-eventListener-attributes.html
fast/harness/results.html
http/tests/inspector/console-xhr-logging-async.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
compositing/fixed-position-scroll-offset-history-restore.html
http/tests/history/replacestate-post-to-get-2.html
http/tests/history/back-during-onload-triggered-by-back.html
fast/events/constructors/storage-event-constructor.html
http/tests/cache/preload-cleared-after-parsing-canceled-by-js.html

[Lyon Chen]

Created attachment 194293 [details]
Another patch try to fix mac-wk2 test failure.

[Build Bot]

Comment on attachment 194293 [details]
Another patch try to fix mac-wk2 test failure.

Attachment 194293 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17223454

New failing tests:
fast/events/backspace-navigates-back.html
fast/history/history-back-forward-within-subframe-hash.html
fast/css/preserve-user-specified-zoom-level-on-reload.html
fast/dom/everything-to-string.html
http/tests/inspector/change-iframe-src.html
http/tests/cache/loaded-from-cache-after-reload.html
fast/loader/child-frame-add-after-back-forward.html
fast/dom/Window/HTMLFrameSetElement-window-eventListener-attributes.html
fast/hidpi/image-set-background-repeat.html
fast/events/autoscroll-in-textarea.html
fast/dom/constructed-objects-prototypes.html
http/tests/history/replacestate-post-to-get.html
editing/pasteboard/smart-paste-001.html
fast/hidpi/image-set-as-background.html
fast/loader/stateobjects/document-destroyed-navigate-back-with-fragment-scroll.html
fast/events/popup-blocked-from-history-reload.html
http/tests/cache/loaded-from-cache-after-reload-within-iframe.html
fast/hidpi/image-set-background-repeat-without-size.html
fast/frames/layout-after-destruction.html
fast/dom/Window/HTMLBodyElement-window-eventListener-attributes.html
fast/harness/results.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
compositing/fixed-position-scroll-offset-history-restore.html
http/tests/history/replacestate-post-to-get-2.html
http/tests/history/back-during-onload-triggered-by-back.html
fast/history/history-back-initial-vs-final-url.html
fast/events/constructors/storage-event-constructor.html
http/tests/cache/preload-cleared-after-parsing-canceled-by-js.html

[Build Bot]

Created attachment 194386 [details]
Archive of layout-test-results from webkit-ews-09

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: <class 'webkitpy.common.config.ports.MacWK2Port'>  Platform: Mac OS X 10.8.2

[Lyon Chen]

Created attachment 194429 [details]
patch fixed error causing mac-wk2 test failures.

[Lyon Chen]

+Darin, Jeremy, Beidson.

You are the experts on localStorage/sessionStorage. Do you have the time to review this patch to make it easier to fix filldisk.com exploitation?

[Carlos Garcia Campos]

Comment on attachment 194429 [details]
patch fixed error causing mac-wk2 test failures.

View in context: https://bugs.webkit.org/attachment.cgi?id=194429&action=review

My main concern of this approach of letting the client decide is that the call is synchronous, I'm not sure how that would fit in WebKit2. Maybe we could use an approach similar to the database, were there's a client notification and the user can set a new quota. I also wonder whether it would be enough to implement the quota per domain base in cross-platform code, I guess we would need to add platform implementations for TLD, maybe KURL::baseDomain() and KURL::isPublicSuffix() or as global functions, I don't know.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:251
> +    RefPtr<StorageArea> storageArea;
> +    if (isLocalStorage) {
> +        storageArea = page->group().localStorage()->storageArea(frame->document()->securityOrigin());
> +        if (!storageArea)
> +            storageArea = page->group().localStorage()->createStorageArea(frame->document()->securityOrigin(), page->group().groupSettings()->localStorageQuotaBytes());
> +    } else {
> +        storageArea = page->sessionStorage()->storageArea(frame->document()->securityOrigin());
> +        if (!storageArea)
> +            storageArea = page->sessionStorage()->createStorageArea(frame->document()->securityOrigin(), page->settings()->sessionStorageQuota());
> +    }
> +

Wouldn't it be possible to call the client method from storageArea() instead of splitting the method? That way you wouldn't need any change here.

> Source/WebCore/page/ChromeClient.h:195
> +        StorageIndexedDB

Is this actually used?

> Source/WebCore/page/ChromeClient.h:198
> +    virtual bool shouldAllowStorageRequest(StorageType, SecurityOrigin*, unsigned quota) = 0;

I wonder if this is enough information for the client to make a decision. For example, if I want to make a quota per base domain like firefox does, I would need to be able to iterate all areas to compare the base domain of their security origin.

> Source/WebCore/page/DOMWindow.cpp:777
> +    if (!storageArea) {
> +        if (!page->chrome()->client()->shouldAllowStorageRequest(ChromeClient::StorageSession, document->securityOrigin(), page->settings()->sessionStorageQuota())) {
> +            ec = QUOTA_EXCEEDED_ERR;
> +            return 0;
> +        }
> +
> +        storageArea = page->sessionStorage()->createStorageArea(document->securityOrigin(), page->settings()->sessionStorageQuota());

If storageArea calls the client we can assume that when it returns 0 is because of QUOTA_EXCEEDED_ERR.

> Source/WebCore/page/Page.cpp:1086
> -        m_sessionStorage = StorageNamespace::sessionStorageNamespace(this, m_settings->sessionStorageQuota());
> +        m_sessionStorage = StorageNamespace::sessionStorageNamespace(this);

You need to rebase the patch, this has changed recently.

[Carlos Garcia Campos]

I've just noticed there's already PublicSuffix.h in platform, only implemented by mac at the moment.

[Lyon Chen]

(In reply to comment #24)
> (From update of attachment 194429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194429&action=review
> 
> My main concern of this approach of letting the client decide is that the call is synchronous, I'm not sure how that would fit in WebKit2. Maybe we could use an approach similar to the database, were there's a client notification and the user can set a new quota.

But in current implementation DatabaseManager::openDatabaseBackend() also expect databaseExceededQuota() is synchronous, as it calls openDatabase(RetryOpenDatabase) immediately.

> I also wonder whether it would be enough to implement the quota per domain base in cross-platform code, I guess we would need to add platform implementations for TLD, maybe KURL::baseDomain() and KURL::isPublicSuffix() or as global functions, I don't know.

Right now in platform code, I am using our internal TLD checking. It was done this way so different ports can have flexibility to do things in their own way. Not sure it will be easy to do it in cross-platform way.

> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:251
> > +    RefPtr<StorageArea> storageArea;
> > +    if (isLocalStorage) {
> > +        storageArea = page->group().localStorage()->storageArea(frame->document()->securityOrigin());
> > +        if (!storageArea)
> > +            storageArea = page->group().localStorage()->createStorageArea(frame->document()->securityOrigin(), page->group().groupSettings()->localStorageQuotaBytes());
> > +    } else {
> > +        storageArea = page->sessionStorage()->storageArea(frame->document()->securityOrigin());
> > +        if (!storageArea)
> > +            storageArea = page->sessionStorage()->createStorageArea(frame->document()->securityOrigin(), page->settings()->sessionStorageQuota());
> > +    }
> > +
> 
> Wouldn't it be possible to call the client method from storageArea() instead of splitting the method? That way you wouldn't need any change here.

Then I will need Page info in StorageNameSpaceImpl, I am not sure whether StorageNameSpaceImpl should carry this info in it.

> 
> > Source/WebCore/page/ChromeClient.h:195
> > +        StorageIndexedDB
> 
> Is this actually used?

No, it is not yet. Just leave it as an indication it is intened to use for all storage, not just LocalStorage and SessionStorage. I can remove it.

> 
> > Source/WebCore/page/ChromeClient.h:198
> > +    virtual bool shouldAllowStorageRequest(StorageType, SecurityOrigin*, unsigned quota) = 0;
> 
> I wonder if this is enough information for the client to make a decision. For example, if I want to make a quota per base domain like firefox does, I would need to be able to iterate all areas to compare the base domain of their security origin.

Yes platform code could need to iterate all area to figure that out, but I supposed platform code will have kind of cache, like we do in BlackBerry port, we have database to remember every SecurityOrigin and every base domain (TLD) (can have multiple SecurityOrigin) storage usage. So it should not be a problem I hope.

> 
> > Source/WebCore/page/DOMWindow.cpp:777
> > +    if (!storageArea) {
> > +        if (!page->chrome()->client()->shouldAllowStorageRequest(ChromeClient::StorageSession, document->securityOrigin(), page->settings()->sessionStorageQuota())) {
> > +            ec = QUOTA_EXCEEDED_ERR;
> > +            return 0;
> > +        }
> > +
> > +        storageArea = page->sessionStorage()->createStorageArea(document->securityOrigin(), page->settings()->sessionStorageQuota());
> 
> If storageArea calls the client we can assume that when it returns 0 is because of QUOTA_EXCEEDED_ERR.

Sorry I didn't get your point, you mean there is no need to set ec (as QUOTA_EXCEEDED_ERR)?

> 
> > Source/WebCore/page/Page.cpp:1086
> > -        m_sessionStorage = StorageNamespace::sessionStorageNamespace(this, m_settings->sessionStorageQuota());
> > +        m_sessionStorage = StorageNamespace::sessionStorageNamespace(this);
> 
> You need to rebase the patch, this has changed recently.

[Andreas Kling]

Comment on attachment 194429 [details]
patch fixed error causing mac-wk2 test failures.

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.