Bug 112713 – [chromium] Support Quota API in Worker in WebKit API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

We need worker-specific code in the platform layer.

[Alec Flett]

Created attachment 193983 [details]
Patch

[Alec Flett]

Here is a strawman patch. I have no idea if it works, only that it compiles but I wouldn't mind a quick look at it from kinuko. Most of this is cribbed from the FileSystem stuff. 

Tests coming up tomorrow.

There's one "XXX" in here about moving stuff into Platform/chromium - not sure what the best course of action is, but I'm CC'ing pilgrim@ just in case.

See also http://crbug.com/88490 for the chromium side.

[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.

[Adam Barth]

Comment on attachment 193983 [details]
Patch

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:37
> +// XXX need to move this to Platform?

We usually use FIXME rather than XXX

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:54
>  namespace WebCore {

Yeah, it looks like some of this code needs to move into WebCore.  That's not something we need to fix in this patch, however.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:58
> +namespace {

We usually use the "static" keyword rather than anonymous namespaces.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:74
> +        // m_bridge->didGrandStorageQuotaOnMainThread(grantedQuotaInBytes);

??

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:139
> +        delete observer;

:(

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.h:71
> +class WorkerStorageQuotaCallbacksBridge : public ThreadSafeRefCounted<WorkerStorageQuotaCallbacksBridge> {

Why is this all so complicated?

[Adam Barth]

> Here is a strawman patch. I have no idea if it works, only that it compiles but I wouldn't mind a quick look at it from kinuko.

Oh, sorry.  I thought this patch was further along.

The patch seems like it has way too much machinery for what it's trying to accomplish.  Maybe we should use some of our more modern threading primitives?

[Kinuko Yasuda]

Comment on attachment 193983 [details]
Patch

I'll take another look tomorrow but I think the code looks good. Thanks for doing this quickly!

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:39
> +#include "WebStorageQuotaType.h"

I think we're expected to move them, but I hope it can come later in a separate patch.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:65
> +    WebCore::WorkerLoaderProxy* workerLoaderProxy =  &workerThread->workerLoaderProxy();

nit: extra space before &workerThread

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:47
> +// FileSystemCallbacks that are to be dispatched on the main thread.

nit; FileSystemCallbacks -> WebStorageQuotaCallbacks

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:74
> +        // m_bridge->didGrandStorageQuotaOnMainThread(grantedQuotaInBytes);

Let's remove this commented-out line after we make sure it's not mandatory to support this in Workers.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:143
> +// FIXME: implement this. I think we can just follow the way how WorkerFileSystemCallbacksBridge.cpp is doing, though it's kinda complicated..

Oops, can you remove this line... thanks!

[Kinuko Yasuda]

(In reply to comment #5)
> > Here is a strawman patch. I have no idea if it works, only that it compiles but I wouldn't mind a quick look at it from kinuko.
> 
> Oh, sorry.  I thought this patch was further along.
> 
> The patch seems like it has way too much machinery for what it's trying to accomplish.  Maybe we should use some of our more modern threading primitives?

I agree this is too complex compared to what it's trying to achieve, while basically this one tries to follow the existing method so that we can be sure it works.  Because of the urgency we may not be able to re-implement this in the first patch but can you let us know about the modern threading primitives?

[Kinuko Yasuda]

Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.

Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).

Current patch is making the request to the browser by relaying the request to the main thread.  To do this we create proxy callback object on the main thread, perform usual IPC, wait callbacks on the main thread, and dispatch the callbacks on worker thread when they're fired on the proxy callback object on the main thread.  Other half of the complexity is added just to handle worker process shutdown gracefully.  Since we do the same in FileSystem Worker requests this patch is trying to follow the way precisely so that we can be sure it works. (But again I agree the current code looks too complicated)

abarth@: if you have a suggestion is it possible to guide us?  Meanwhile we don't have time to investigate (this patch is strongly requested for M27 cut) so if this looks acceptable we'd like to go with this direction... thanks!

[Adam Barth]

(In reply to comment #8)
> Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.
> 
> Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).

That's right, but we should look at the Chromium implementation of the function to see if we can remove the WebFrame parameter.

> Current patch is making the request to the browser by relaying the request to the main thread.  To do this we create proxy callback object on the main thread, perform usual IPC, wait callbacks on the main thread, and dispatch the callbacks on worker thread when they're fired on the proxy callback object on the main thread.  Other half of the complexity is added just to handle worker process shutdown gracefully.  Since we do the same in FileSystem Worker requests this patch is trying to follow the way precisely so that we can be sure it works. (But again I agree the current code looks too complicated)

I don't think we need to block this patch on finding a simpler design, but I suspect we can use WeakPtr and WTF::Functional to handle the shutdown case more gracefully.  It's likely we should apply those primitives to many of the threading operations in WebCore.

> abarth@: if you have a suggestion is it possible to guide us?  Meanwhile we don't have time to investigate (this patch is strongly requested for M27 cut) so if this looks acceptable we'd like to go with this direction... thanks!

I think it's fine to move forward with the existing tools and then think about how to improve this pattern across the codebase in follow-on work.

[Kinuko Yasuda]

(In reply to comment #9)
> (In reply to comment #8)
> > Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.
> > 
> > Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).
> 
> That's right, but we should look at the Chromium implementation of the function to see if we can remove the WebFrame parameter.
> 
> > Current patch is making the request to the browser by relaying the request to the main thread.  To do this we create proxy callback object on the main thread, perform usual IPC, wait callbacks on the main thread, and dispatch the callbacks on worker thread when they're fired on the proxy callback object on the main thread.  Other half of the complexity is added just to handle worker process shutdown gracefully.  Since we do the same in FileSystem Worker requests this patch is trying to follow the way precisely so that we can be sure it works. (But again I agree the current code looks too complicated)
> 
> I don't think we need to block this patch on finding a simpler design, but I suspect we can use WeakPtr and WTF::Functional to handle the shutdown case more gracefully.  It's likely we should apply those primitives to many of the threading operations in WebCore.
>
> > abarth@: if you have a suggestion is it possible to guide us?  Meanwhile we don't have time to investigate (this patch is strongly requested for M27 cut) so if this looks acceptable we'd like to go with this direction... thanks!
> 
> I think it's fine to move forward with the existing tools and then think about how to improve this pattern across the codebase in follow-on work.

Thanks!  Looks like today we have a lot more useful primitives in WTF/WebCore (man, I didn't know we have WeakPtr now!!), we'd love to cleanup the code in follow-up patches (unless Alec wants take a shot now).

[Mark Pilgrim (Google)]

(In reply to comment #9)
> (In reply to comment #8)
> > Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.
> > 
> > Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).
> 
> That's right, but we should look at the Chromium implementation of the function to see if we can remove the WebFrame parameter.

Implementation in render_view_impl.cc suggests that the WebFrame parameter is only used to get the security origin to check if it is unique, since unique origins can not store persistent state. This is a prevalent antipattern that I'm currently in the process of cleaning up in another (unrelated) bug. The fix is to change the WebFrame* parameter to a WebString that is the string serialization of the security origin of the document of the web frame, and change the implementation in Chromium to match.

[Kinuko Yasuda]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Let me summarize what we want to do, I need to check if we're going right (or at least ok) direction.
> > > 
> > > Basically we want to send the 'requestUsageAndQuota' IPC from worker to browser.  If it were a Platform function we'd directly send the request from the worker thread via thread-safe-sender, but the requestUsageAndQuota takes WebFrame and I don't think we can add it to Platform (in my understanding).
> > 
> > That's right, but we should look at the Chromium implementation of the function to see if we can remove the WebFrame parameter.
> 
> Implementation in render_view_impl.cc suggests that the WebFrame parameter is only used to get the security origin to check if it is unique, since unique origins can not store persistent state. This is a prevalent antipattern that I'm currently in the process of cleaning up in another (unrelated) bug. The fix is to change the WebFrame* parameter to a WebString that is the string serialization of the security origin of the document of the web frame, and change the implementation in Chromium to match.

Oh oh... yes now I remember I've seen your email about the cleanup.  So once we do that we can just move them to Platform. Let us see if we want to do the change now or just continue finishing this patch.  If we go the latter way we'd love to make the cleanup change later.

[Alec Flett]

Created attachment 194150 [details]
Patch

[Alec Flett]

And here's the patch with a working webcore, and a layout test.

The only issue left here is that shared workers are not working (the request is going in and the callback is firing, but not being dispatched to the JS callback) - dedicated workers work just fine. This may be something on the chromium side that I need to investigate.

[Build Bot]

Comment on attachment 194150 [details]
Patch

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

New failing tests:
fast/workers/worker-storagequota-query-usage.html
fast/workers/shared-worker-storagequota-query-usage.html

[WebKit Review Bot]

Comment on attachment 194150 [details]
Patch

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

New failing tests:
fast/workers/worker-storagequota-query-usage.html
svg/as-image/image-preserveAspectRatio-all.svg
svg/filters/feImage-preserveAspectRatio-all.svg
fast/workers/shared-worker-storagequota-query-usage.html

[Kinuko Yasuda]

Comment on attachment 194150 [details]
Patch

When you upload the final patch(es) I think we'd better file another bug for WebCore (e.g. "Expose StorageQuota interface on Workers") and split this into two: WebCore one and chromium-specific one.

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

> Source/WebCore/WebCore.gypi:178
> +            'Modules/quota/NavigatorStorageQuota.idl',

not needed?

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:154
> +

nit: extra empty line

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
> +// FIXME: This should be generalized with classes like MainThreadFileSystemCallbacks

Can we update this FIXME to:
// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota with WebString and move the method to Platform so that we can remove all these complexity for Worker.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:89
> +

nit: Extra empty lines

[Alec Flett]

Created attachment 194332 [details]
Patch

[Alec Flett]

here's the final unified patch, with working tests. I'm now splitting it up into WebKit vs WebCore.

[WebKit Review Bot]

Comment on attachment 194332 [details]
Patch

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

New failing tests:
fast/workers/worker-storagequota-query-usage.html
fast/workers/shared-worker-storagequota-query-usage.html

[WebKit Review Bot]

Created attachment 194353 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Alec Flett]

(those failure are mostly because the chrome side has to land first)

[Build Bot]

Comment on attachment 194332 [details]
Patch

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

New failing tests:
fast/workers/worker-storagequota-query-usage.html
fast/workers/shared-worker-storagequota-query-usage.html

[Build Bot]

Created attachment 194356 [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

[Kinuko Yasuda]

Please make sure add new tests to the skip list in each platform's TestExpectation (I'm sure you're doing so now).

> (those failure are mostly because the chrome side has to land first)

I don't think we support shared worker (and Quota) in test_shell/DumpRenderTree, so I think you need to skip them even for chromium at least for now.

[Alec Flett]

Created attachment 194357 [details]
Patch

[Alec Flett]

abarth@ - r?

The webcore side of things now shows up in bug 112972

[Adam Barth]

Comment on attachment 194357 [details]
Patch

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

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:58
> +static const char queryUsageAndQuotaMode[] = "queryUsageAndQuotaMode";

You should just use the string literal on line 68.  There's no advantage to having a separate declarations.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:65
> +    WorkerScriptController* controller = WorkerScriptController::controllerForContext();
> +    WorkerContext* workerContext = controller->workerContext();
> +    WebCore::WorkerThread* workerThread = workerContext->thread();
> +    WorkerRunLoop& runLoop = workerThread->runLoop();

I would combine all of this into one line.  We shouldn't need to name all these temporaries.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:69
> +    mode.append(String::number(runLoop.createUniqueId()));

You should use operator+ rather than calling append() on String.

> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:72
> +    bridge->postQueryUsageAndQuotaToMainThread(commonClient, storageType, mode);

Is the bridge just dropped on the floor here?

[Adam Barth]

> abarth@ - r?

I don't think I'm the best reviewer for this patch.

[Adam Barth]

Comment on attachment 194357 [details]
Patch

API change LGTM

[Kinuko Yasuda]

Comment on attachment 194357 [details]
Patch

The code looks good (in a sense that it looks mostly same to the existing working code).
Can you please update the FIXME comment so that it becomes clear there're possible cleanups we can do later?

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

>> Source/WebKit/chromium/src/StorageQuotaChromium.cpp:72
>> +    bridge->postQueryUsageAndQuotaToMainThread(commonClient, storageType, mode);
> 
> Is the bridge just dropped on the floor here?

It's reference is held in a task that is posted to the main thread in the method.

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:154
> +

nit: extra empty space

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
> +// FIXME: This should be generalized with classes like MainThreadFileSystemCallbacks

Can we update this FIXME to:

// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:89
> +

nit: extra empty lines

[Kinuko Yasuda]

(In reply to comment #29)
> > abarth@ - r?
> 
> I don't think I'm the best reviewer for this patch.

(I pinged levin@ to review this. -- thanks Dave!)

[David Levin]

Comment on attachment 194357 [details]
Patch

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

>> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
>> +// FIXME: This should be generalized with classes like MainThreadFileSystemCallbacks
> 
> Can we update this FIXME to:
> 
> // FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker.

+1

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:50
> +    // Callbacks are self-destructed and we always return leaked pointer here.

It might be better to explain that the class is destructed in the callbacks "did*. See the other comment below.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:63
> +        m_bridge->didQueryStorageUsageAndQuotaOnMainThread(usageInBytes, quotaInBytes, m_mode);

delete this;

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:68
> +        m_bridge->didFailOnMainThread(error, m_mode);

delete this;

[David Levin]

Please address all the feedback given so far and then I think this will be ready for r+

[Alec Flett]

Created attachment 194414 [details]
Patch

All review comments addressed... one more time, levin@ - r?

[David Levin]

Comment on attachment 194414 [details]
Patch

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

Where are the tests?

Plus the other comment.

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:91
> +// that it only gets deleted on the worker context thread which is verified by ~Observer.

Kinuko's previous comment about this:
"Can we update this FIXME to:

// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker."

[Kinuko Yasuda]

(In reply to comment #36)
> (From update of attachment 194414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194414&action=review
> 
> Where are the tests?

We have tests in WebCore-side patch in bug 112972

(Alec, it'd be nicer to note that the test is coming in the other patch in ChangeLog)

[David Levin]

Comment on attachment 194414 [details]
Patch

ok. Please fix the comment per Kinuko's suggestion before submitting.

[Alec Flett]

Created attachment 194589 [details]
Patch for landing

[Alec Flett]

(Sorry there were two FIXME's - so now I've replaced them both with the new comment)

[WebKit Review Bot]

Comment on attachment 194589 [details]
Patch for landing

Clearing flags on attachment: 194589

Committed r146633: <http://trac.webkit.org/changeset/146633>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

Comment on attachment 194589 [details]
Patch for landing

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

> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
> +// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker.

Would it be good to handle this more like we handle IDB api calls and do the thread hopping (back and forth) on the other side of the WebKit API? The amount of and complexity of this stuff in webkit/webcore feels really disproportionate to the task.

[Kinuko Yasuda]

Comment on attachment 194589 [details]
Patch for landing

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

>> Source/WebKit/chromium/src/WorkerStorageQuotaCallbacksBridge.cpp:46
>> +// FIXME: Replace WebFrame parameter in queryStorageUsageAndQuota() with WebString and move the method to Platform so that we can remove all these complexity for Worker.
> 
> Would it be good to handle this more like we handle IDB api calls and do the thread hopping (back and forth) on the other side of the WebKit API? The amount of and complexity of this stuff in webkit/webcore feels really disproportionate to the task.

I'm thinking about moving this to Platform and just send an IPC from there using thread-safe-sender with thread-safe callback map on the other side.
And (as I keep saying in this issue) I agree that this change is way too complex and should be gone asap.

For follow-up cleanup I filed a bug (on crbug for now)-- http://crbug.com/223203

[Michael Nordman]

Comment on attachment 194589 [details]
Patch for landing

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

> Source/WebKit/chromium/public/WebCommonWorkerClient.h:76
> +    virtual void queryUsageAndQuota(WebStorageQuotaType, WebStorageQuotaCallbacks*)

Yup, define this method to be called on the worker thread directly and to have the callback invoked directly on the worker thread in response.

[Alec Flett]

and to follow up with michaeln@ - I asked dgrogan "how does IDB do this because I see none of this glue in any of our code" and he pointed out the change on the chrome side in the IndexedDBMessageFilter or something like that.