Bug 37717 – Provide a way for clients to set session storage quota

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ada Chan]

Right now we have a way to set the quota for local storage but there's no way to specify a quota for session storage.  We should provide a way to set that quota for clients who care about limiting session storage size.

[Ada Chan]

Created attachment 53554 [details]
Patch to allow setting session storage quota

Patch.  I have suspicion that this may break chromium.  Hopefully the trybot will tell me exactly what to fix.

[Jeremy Orlow]

Comment on attachment 53554 [details]
Patch to allow setting session storage quota

Why should clients be able to set session storage quota?  If pages want to use up all the memory, they can.  And that memory goes away when the page goes away.  Are you scared about session storage being filled up and the user navigating to other origins but not closing the tab/page/window (and thus it stays used)?  I guess I could see that being a valid use case for this....but either way, the "why" should probably be explained better in the change log.  (The meat of the patch looks good tho.)

[Ada Chan]

Some WebKit devices may want to constrain session storage just like they do for local storage.  I'll mention that in the ChangeLog.

[Jeremy Orlow]

The reason why we constrain it for LocalStorage is because the effects last beyond just the browsing session.  With SessionStorage, all you have to do to kill any negative effects is close the browser window.  This is in line with the rest of the web.  My point is that if you're worried about memory consumption, limiting just SessionStorage doesn't do much for you.

The only thing that isn't in line with the rest of the web is that navigating to another page doesn't result in everything being cleaned up.

My point is that I'm not convinced this feature is actually useful and I'd (at very least) like these nuances to be clear in the ChangeLog.  Actually....maybe these should be comments in settings.h or something...

[Ada Chan]

Created attachment 53583 [details]
Revised patch with additional comments in Settings.h and ChangeLog.

[Ada Chan]

> My point is that I'm not convinced this feature is actually useful and I'd (at
> very least) like these nuances to be clear in the ChangeLog.  Actually....maybe
> these should be comments in settings.h or something...

I've added comments to Settings.h and ChangeLog.

Your comment is probably valid for the desktop environment.  However, a way to set a quota on session storage would be useful for devices that are more constrained in resources and therefore more concerned with memory consumption.

[Jeremy Orlow]

Comment on attachment 53583 [details]
Revised patch with additional comments in Settings.h and ChangeLog.

I don't think your comments adequately explain the situation.  This helps one element of memory consumption.  And the only reason why it's interesting is that the memory used stays until the Page dies.  If that weren't true, I'd r- and mark the bug invalid.

LocalStorage and SessionStorage are often grouped together in peoples mind.  So I want to make sure that if someone's using LocalStorage quota they don't automatically think using SessionStorage quota is important to them.

[Jeremy Orlow]

Comment on attachment 53583 [details]
Revised patch with additional comments in Settings.h and ChangeLog.

r=me

but i still think you can improve the comment a bit (per my other comment)

[Ada Chan]

(In reply to comment #8)
> (From update of attachment 53583 [details])
> r=me
> 
> but i still think you can improve the comment a bit (per my other comment)

I've changed comment in Settings.h to be:

        // Allow clients concerned with memory consumption to set a quota on session storage
        // since the memory used won't be released until the Page is destroyed.

[Jeremy Orlow]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 53583 [details] [details])
> > r=me
> > 
> > but i still think you can improve the comment a bit (per my other comment)
> 
> I've changed comment in Settings.h to be:
> 
>         // Allow clients concerned with memory consumption to set a quota on
> session storage
>         // since the memory used won't be released until the Page is destroyed.

Perfect.

[Ada Chan]

Thanks Jeremy for the review.

I really want to see the chromium trybot try this patch and make sure I don't cause any breakages.  Because there's a bug with trybots now not looking at r+ patches, I'll mark it back to ? for now.  When the trybot finishes, hopefully someone will r+ the patch again.

[Jeremy Orlow]

If anything http://trac.webkit.org/browser/trunk/WebKit/chromium/src/StorageNamespaceProxy.cpp will need some massaging.  Probably by explicitly calling the parent constructor.

[Jeremy Orlow]

You can also build locally with |webkit-build --chromium| too, which might be easier than iterating with the bot.

[WebKit Review Bot]

Attachment 53583 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1695208

[Ada Chan]

Created attachment 53596 [details]
Patch with chromium fix

[WebKit Review Bot]

Attachment 53596 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1614594

[Ada Chan]

Created attachment 53598 [details]
Patch with chromium fix take 2 - forgot WebKitTools last time.

[Jeremy Orlow]

Comment on attachment 53598 [details]
Patch with chromium fix take 2 - forgot WebKitTools last time.

This will still break Chromium since it consumes the API (even though it completely compiles now).  Could you please leave the default constructor and have it call the other one with noQuota?  In fact, you could even skip adding the constructor that allows a quota to be passed in since we don't currently have any plans to add it (and let someone working on Chromium change it if we change our minds).  Thanks!

[Ada Chan]

> Could you please leave the default constructor and
> have it call the other one with noQuota?

I'd like to get clarification on your suggestion.  Are you saying instead of changing:

PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page*)

to take in a new parameter, I should just make another version that also takes in a quota

PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page*, unsigned quota)

and have the original version's implementation just call the new version with noQuota?  Then the only chromium change that I need to make is passing noQuota to StorageNamespaceImpl::sessionStorageNamespace here:

WebStorageNamespace* WebStorageNamespace::createSessionStorageNamespace()
{
    return new WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::sessionStorageNamespace(noQuota));
}

[Jeremy Orlow]

(In reply to comment #19)
> > Could you please leave the default constructor and
> > have it call the other one with noQuota?
> 
> I'd like to get clarification on your suggestion.  Are you saying instead of
> changing:
> 
> PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page*)
> 
> to take in a new parameter, I should just make another version that also takes
> in a quota
> 
> PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page*,
> unsigned quota)
> 
> and have the original version's implementation just call the new version with
> noQuota?  Then the only chromium change that I need to make is passing noQuota
> to StorageNamespaceImpl::sessionStorageNamespace here:
> 
> WebStorageNamespace* WebStorageNamespace::createSessionStorageNamespace()
> {
>     return new
> WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::sessionStorageNamespace(noQuota));
> }

I think what you've done in WebCore is great and shouldn't be changed.  I believe you can simply do this last part (passing in noQuota when creating the session storage StorageNamespaceImpl) and you're done.

If that doesn't make sense, it's probably because I'm missing something obvious.  If so, I'm happy to take a shot at the Chromium side of this and upload it.  Or you're free to land as is and we'll get our own build working again (since it really isn't your problem).

[Ada Chan]

> I think what you've done in WebCore is great and shouldn't be changed.  I
> believe you can simply do this last part (passing in noQuota when creating the
> session storage StorageNamespaceImpl) and you're done.

If I don't change what I have in WebCore, I'll also need to fix:
PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page* page)
to take in a quota argument since I've changed the signature of StorageNamespace:;sessionStorageNamespace in WebCore.

So the entire change for chromium will look like:

--- WebKit/chromium/src/StorageNamespaceProxy.cpp	(revision 57799)
+++ WebKit/chromium/src/StorageNamespaceProxy.cpp	(working copy)
@@ -47,7 +47,7 @@ PassRefPtr<StorageNamespace> StorageName
     return adoptRef(new StorageNamespaceProxy(WebKit::webKitClient()->createLocalStorageNamespace(path, quota), LocalStorage));
 }
 
-PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page* page)
+PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page* page, unsigned quota)
 {
     WebKit::ChromeClientImpl* chromeClientImpl = static_cast<WebKit::ChromeClientImpl*>(page->chrome()->client());
     WebKit::WebViewClient* webViewClient = chromeClientImpl->webView()->client();
Index: WebKit/chromium/src/WebStorageNamespaceImpl.cpp
===================================================================
--- WebKit/chromium/src/WebStorageNamespaceImpl.cpp	(revision 57799)
+++ WebKit/chromium/src/WebStorageNamespaceImpl.cpp	(working copy)
@@ -47,7 +47,7 @@ WebStorageNamespace* WebStorageNamespace
 
 WebStorageNamespace* WebStorageNamespace::createSessionStorageNamespace()
 {
-    return new WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::sessionStorageNamespace());
+    return new WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::sessionStorageNamespace(noQuota));
 }
 
 WebStorageNamespaceImpl::WebStorageNamespaceImpl(PassRefPtr<WebCore::StorageNamespace> storageNamespace)

[Ada Chan]

Checked in r57822.  Checked in the minimum changes needed to fix chromium build as described in the last comment.

http://trac.webkit.org/changeset/57822

[WebKit Review Bot]

http://trac.webkit.org/changeset/57822 might have broken Qt Linux Release minimal

[Eric Seidel (no email)]

Looks like this broke Qt Minimal and will likely need some #ifdef QSETTINGS guards.  :(

[Joseph Pecoraro]

(In reply to comment #24)
> Looks like this broke Qt Minimal and will likely need some #ifdef QSETTINGS
> guards.  :(

This could be that Qt on the build bot uses: -DENABLE_DOM_STORAGE=0
StorageMap::noQuota is inside a ENABLE(DOM_STORAGE) guard in StorageMap.h.

Eric, any way to tell if the "try" bot for Qt compiles with this flag on or off?

[Eric Seidel (no email)]

The "try" bot (EWS) just uses "build-webkit".  The code is here:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/ports.py#L152

Basically results in:
build-webkit --release --qt

We may want to remove the Qt MINIMAL builder from core, as personally I find trying to support an "everything off" configuration somewhat confusing. :)  I guess it's useful to know that our #ifdefs are clean or not.