DB quota

Use the QuotaManager to determine the space available to WebSQLDatabases. Since activity outside of the database system now affect how much space is available, our strategy for informing the client side of the new limit needed to change. It's not enough to just send the new limit when a change within the DB system has occurred.

This change depends on a webkit/webcore change.
https://bugs.webkit.org/show_bug.cgi?id=60985

In this CL the renderer will ask the browser for the limit when needed. But also in this CL are ipc plumbing additions to support notifying the renderer as changes occur. That plumbing will be utilized in a later CL.

* [Chrome] DatabaseMessageFilter uses the QuotaManager respond to the SpaceAvailable query.

* [DRT] SimpleDatabaseSystem uses quota_per_origin_ data member to respond to the SpaceAvailable query.

* Mostly mind numbing plumbing for WebKit API additions.

// Sync getter for use on webcore's background db threads to
// call out to chrome to get the value.
long long WebKitClient::databaseGetSpaceAvailableForOrigin(origin_identifier);

// Split the existing updateDatabaseSize method into three methods. Chrome calls into these.
static void WebDatabase::updateDatabaseSize(originIdentifier, dbname, spaceAvailable);
static void WebDatabase::updateSpaceAvailable(originIdentifier, spaceAvailable);
static void WebDatabase::resetSpaceAvailable(originIdentifier);
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86537

[michaeln, 2011-05-19 06:47:04]

I think this is ready for looking at now, mostly plumbing but
SimpleDatabaseSystem and DatabaseMessageFilter have some real changes.

[michaeln, 2011-05-19 07:03:34]

http://codereview.chromium.org/7037018/diff/5009/content/browser/renderer_hos...
File content/browser/renderer_host/database_message_filter.cc (right):

http://codereview.chromium.org/7037018/diff/5009/content/browser/renderer_hos...
content/browser/renderer_host/database_message_filter.cc:285:
quota_manager->GetUsageAndQuota(
Hi Kinuko, my local build got into an odd state where GetUsageAndQuota() calls
for a particular origin never completed. I'm not sure how it entered this state,
but a 'task' was pending in the QM and not making progress. Subsequent calls to
this method just added the callback to the wedged task. Haven't been able to
repro it yet.

[kinuko, 2011-05-19 07:18:29]

Geez that's bad.  GetUsageAndQuota in turn calls client's GetUsageForOrigin,
could there be some re-entrancy issue?  Anyway I need to look into the code
again.
Right now I haven't added unittest that actually calls into QM from fileapi
or some 'real' client but we might need a test for that.

On Thu, May 19, 2011 at 4:03 PM, <michaeln@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/7037018/diff/5009/content/browser/renderer_hos...
> File content/browser/renderer_host/database_message_filter.cc (right):
>
>
>
http://codereview.chromium.org/7037018/diff/5009/content/browser/renderer_hos...
> content/browser/renderer_host/database_message_filter.cc:285:
> quota_manager->GetUsageAndQuota(
> Hi Kinuko, my local build got into an odd state where GetUsageAndQuota()
> calls for a particular origin never completed. I'm not sure how it
> entered this state, but a 'task' was pending in the QM and not making
> progress. Subsequent calls to this method just added the callback to the
> wedged task. Haven't been able to repro it yet.
>
>
> http://codereview.chromium.org/7037018/
>

[kinuko, 2011-05-19 16:33:38]

LGTM

http://codereview.chromium.org/7037018/diff/5009/content/common/database_util.h
File content/common/database_util.h (right):

http://codereview.chromium.org/7037018/diff/5009/content/common/database_util...
content/common/database_util.h:23: static long long databaseGetSpaceAvaialble(
typo: Avaialble -> Available

(throughout this patch)

[michaeln, 2011-05-19 22:19:47]

http://codereview.chromium.org/7037018/diff/5009/content/common/database_util.h
File content/common/database_util.h (right):

http://codereview.chromium.org/7037018/diff/5009/content/common/database_util...
content/common/database_util.h:23: static long long databaseGetSpaceAvaialble(
On 2011/05/19 16:33:38, kinuko wrote:
> typo: Avaialble -> Available
> 
> (throughout this patch)

Done.

[michaeln, 2011-05-20 05:49:31]

On 2011/05/19 07:18:29, kinuko wrote:
> Geez that's bad.  GetUsageAndQuota in turn calls client's GetUsageForOrigin,
> could there be some re-entrancy issue?  Anyway I need to look into the code
> again.

I figured out where it gets wedged, in a call to IsCompleted in the usage
tracking stuff, there are tests for if (usage_ >= 0) and such to determine
completion... but those numbers went negative in the DB case. Definite problems
with the DBClient (or maybe the client tracker) to end up with negative numbers
like that... but also the QM probably shouldn't get wedged despite the
confounding inputs.

[kinuko, 2011-05-20 05:56:58]

On 2011/05/20 05:49:31, michaeln wrote:
> On 2011/05/19 07:18:29, kinuko wrote:
> > Geez that's bad.  GetUsageAndQuota in turn calls client's GetUsageForOrigin,
> > could there be some re-entrancy issue?  Anyway I need to look into the code
> > again.
> 
> I figured out where it gets wedged, in a call to IsCompleted in the usage
> tracking stuff, there are tests for if (usage_ >= 0) and such to determine
> completion... but those numbers went negative in the DB case. Definite
problems
> with the DBClient (or maybe the client tracker) to end up with negative
numbers
> like that... but also the QM probably shouldn't get wedged despite the
> confounding inputs.

Great, ...I see that could happen.  Seems like we should use a separate flag for
sure.  I'll make the fix.

[michaeln, 2011-05-20 23:56:48]

> Great, ...I see that could happen.  Seems like we should use a separate flag
for
> sure.  I'll make the fix.

I've got a fix in the works for the db client getting out of sync.

[michaeln, 2011-05-22 21:24:13]

i ran the perf tests with this vs M12... no significance difference (some
slighly faster, some slightly slower)

[michaeln, 2011-05-22 21:49:01]

@darin for content owners

This is the chrome-side of the webkit patch you reviewed last week.

http://codereview.chromium.org/7037018/diff/26001/content/browser/renderer_ho...
File content/browser/renderer_host/database_message_filter.cc (right):

http://codereview.chromium.org/7037018/diff/26001/content/browser/renderer_ho...
content/browser/renderer_host/database_message_filter.cc:55:
scoped_ptr<IPC::Message> reply_msg_;
MyGetUsageAndQuotaCallback can be deleted w/o having been Run during shutdown,
we delete the reply msg in this case.

http://codereview.chromium.org/7037018/diff/26001/content/browser/renderer_ho...
content/browser/renderer_host/database_message_filter.cc:286: new
MyGetUsageAndQuotaCallback(this, reply_msg));
the callback is deleted by the QuotaMgr

[darin (slow to review), 2011-05-24 04:56:10]

http://codereview.chromium.org/7037018/diff/25024/content/browser/renderer_ho...
File content/browser/renderer_host/database_message_filter.cc (right):

http://codereview.chromium.org/7037018/diff/25024/content/browser/renderer_ho...
content/browser/renderer_host/database_message_filter.cc:44: // Params are
<status, usage, quota>
nit: create local variables with these names?  it will help make the code below
more readable.

http://codereview.chromium.org/7037018/diff/25024/content/common/database_util.h
File content/common/database_util.h (right):

http://codereview.chromium.org/7037018/diff/25024/content/common/database_uti...
content/common/database_util.h:15: static WebKit::WebKitClient::FileHandle
databaseOpenFile(
nit: these method names should start with an uppercase letter.

http://codereview.chromium.org/7037018/diff/25024/webkit/support/simple_datab...
File webkit/support/simple_database_system.cc (right):

http://codereview.chromium.org/7037018/diff/25024/webkit/support/simple_datab...
webkit/support/simple_database_system.cc:268: done_event->Signal();
are you sure it is safe to delete done_event before Signal returns?  i'm pretty
sure this'll be ok on windows, but i'm not so sure about the posix
implementation of waitable_event_posix.cc.  i recall talking to agl about how we
should consider changing it to support this use case.

[michaeln, 2011-05-24 05:10:54]

http://codereview.chromium.org/7037018/diff/25024/webkit/support/simple_datab...
File webkit/support/simple_database_system.cc (right):

http://codereview.chromium.org/7037018/diff/25024/webkit/support/simple_datab...
webkit/support/simple_database_system.cc:268: done_event->Signal();
On 2011/05/24 04:56:11, darin wrote:
> are you sure it is safe to delete done_event before Signal returns?  i'm
pretty
> sure this'll be ok on windows, but i'm not so sure about the posix
> implementation of waitable_event_posix.cc.  i recall talking to agl about how
we
> should consider changing it to support this use case.

right... i remember that too... i'll check, the other uses like this haven't
cropped up as problems so maybe he posix impl was fixed (wishful thinking).

[michaeln, 2011-05-24 07:34:04]

> right... i remember that too... i'll check, the other uses like this haven't
> cropped up as problems so maybe he posix impl was fixed (wishful thinking).

http://codesearch.google.com/codesearch/p?hl=en#hfE6470xZHk/base/waitable_eve...

This usage looks valid now given the kernel_ data member and it's embedded
lock_. The lock on the kernel is held until Signal() returns, and
TimedWait(infinite) can't return w/o being in the signaled state AND having
acquired/released the kernel's lock after having determined it's in the signaled
state. Also, there are other usages of this form in test_shell/DRT that aren't
exhibiting any problems, so to the best of my knowlege... this is fair usage.

[darin (slow to review), 2011-05-24 16:30:05]

On Tue, May 24, 2011 at 12:34 AM, <michaeln@chromium.org> wrote:

> right... i remember that too... i'll check, the other uses like this
>> haven't
>> cropped up as problems so maybe he posix impl was fixed (wishful
>> thinking).
>>
>
>
>
http://codesearch.google.com/codesearch/p?hl=en#hfE6470xZHk/base/waitable_eve...
>
> This usage looks valid now given the kernel_ data member and it's embedded
> lock_. The lock on the kernel is held until Signal() returns, and
> TimedWait(infinite) can't return w/o being in the signaled state AND having
> acquired/released the kernel's lock after having determined it's in the
> signaled
> state. Also, there are other usages of this form in test_shell/DRT that
> aren't
> exhibiting any problems, so to the best of my knowlege... this is fair
> usage.
>
>
Ah, yes.  That was implemented in r12576 (
http://codereview.chromium.org/53026).

-Darin



>
> http://codereview.chromium.org/7037018/
>

[michaeln, 2011-05-24 19:43:10]

@darin, ptal

http://codereview.chromium.org/7037018/diff/25024/content/browser/renderer_ho...
File content/browser/renderer_host/database_message_filter.cc (right):

http://codereview.chromium.org/7037018/diff/25024/content/browser/renderer_ho...
content/browser/renderer_host/database_message_filter.cc:44: // Params are
<status, usage, quota>
On 2011/05/24 04:56:11, darin wrote:
> nit: create local variables with these names?  it will help make the code
below
> more readable.

Done.

http://codereview.chromium.org/7037018/diff/25024/content/common/database_util.h
File content/common/database_util.h (right):

http://codereview.chromium.org/7037018/diff/25024/content/common/database_uti...
content/common/database_util.h:15: static WebKit::WebKitClient::FileHandle
databaseOpenFile(
On 2011/05/24 04:56:11, darin wrote:
> nit: these method names should start with an uppercase letter.

Done.

[darin (slow to review), 2011-05-24 20:56:17]

LGTM

On Tue, May 24, 2011 at 12:43 PM, <michaeln@chromium.org> wrote:

> @darin, ptal
>
>
>
>
>
http://codereview.chromium.org/7037018/diff/25024/content/browser/renderer_ho...
> File content/browser/renderer_host/database_message_filter.cc (right):
>
>
>
http://codereview.chromium.org/7037018/diff/25024/content/browser/renderer_ho...
> content/browser/renderer_host/database_message_filter.cc:44: // Params
> are <status, usage, quota>
> On 2011/05/24 04:56:11, darin wrote:
>
>> nit: create local variables with these names?  it will help make the
>>
> code below
>
>> more readable.
>>
>
> Done.
>
>
>
>
http://codereview.chromium.org/7037018/diff/25024/content/common/database_util.h
> File content/common/database_util.h (right):
>
>
>
http://codereview.chromium.org/7037018/diff/25024/content/common/database_uti...
> content/common/database_util.h:15: static
> WebKit::WebKitClient::FileHandle databaseOpenFile(
> On 2011/05/24 04:56:11, darin wrote:
>
>> nit: these method names should start with an uppercase letter.
>>
>
> Done.
>
>
> http://codereview.chromium.org/7037018/
>