Add storage code to appcache update process.

Add storage code to appcache update process.
Add storage API for simulating storage errors.

TEST=verify appcache update wrote to storage correctly
BUG=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30612

[michaeln, 2009-10-23 00:58:16]

http://codereview.chromium.org/326002/diff/1/3
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/326002/diff/1/3#newcode55
Line 55: bytes_to_write_ = 0;
This data member access is iffy... see comments below.

http://codereview.chromium.org/326002/diff/1/3#newcode224
Line 224: OnResponseCompleted(request);
here's another delete request (and info) code path

http://codereview.chromium.org/326002/diff/1/3#newcode270
Line 270: OnResponseCompleted(request);
Since this can delete the 'request', which will delete the 'info', the
UpdateJobInfo::OnWriteComplete has to be very careful to not access it's data
members upon return from OnWriteResposneComplete.

http://codereview.chromium.org/326002/diff/1/3#newcode281
Line 281: ReadResponseData(request);
This too can result in 'delete request'.

[jennb, 2009-10-28 01:45:33]

Completed the work for this CL.  I have code in here that cleans up responses. 
Should I take it out?

[michaeln, 2009-10-28 21:19:52]

not a full review yet... but here is a round of comments

http://codereview.chromium.org/326002/diff/3001/4002
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/326002/diff/3001/4002#newcode40
Line 40: void SetUpResponseWriter(AppCacheResponseWriter* writer,
nit: Setup... but i see both SetUp and Setup in use in the code base

http://codereview.chromium.org/326002/diff/3001/4002#newcode49
Line 49: write_callback_.reset(new net::CompletionCallbackImpl<UpdateJobInfo>(
its much more common to define storage for CallbackImpls inline rather than heap
allocate them...

class UpdateJobInfo {
  UpdateJobInfo() 
      : bar_(0),
        ALLOW_THIS_IN_INITIALIZER_LIST(
            write_callback_(this, &UpdateJobInfo::OnWriteComplete)) {
  }

  int bar_;
  net::CompletionCallbackImpl<UpdateJobInfo> write_callback_;
};

http://codereview.chromium.org/326002/diff/3001/4002#newcode68
Line 68: int bytes_to_write_;  // remember data bytes while writing http
response info
is this really specific to 'response info' or is this for the 'data'... oh i see
(after looking at the callsite)... after 'info' is written then you write this
amount of 'data'.

that's a little awkward?

http://codereview.chromium.org/326002/diff/3001/4002#newcode232
Line 232: int bytes_read) {
Can you add a DCHECK(bytes_read > 0) here so readers know w/o examining the
callsite?

http://codereview.chromium.org/326002/diff/3001/4002#newcode239
Line 239: return false;  // wait for async storage callback
nit: maybe "wait for async completion to continue reading"... if it fits on the
line?

http://codereview.chromium.org/326002/diff/3001/4002#newcode467
Line 467: entry.set_response_id(info->response_writer_.get()->response_id());
response_writer_-> should work here

http://codereview.chromium.org/326002/diff/3001/4002#newcode482
Line 482: // might be smart enough to discard uncommitted writes?)
i think we can leave this out for now, and revisit if needed when we get to
persistent storage... wdyt?

http://codereview.chromium.org/326002/diff/3001/4002#newcode549
Line 549: manifest_response_writer_.get()->WriteData(
don't need the .get() on the smart ptr

http://codereview.chromium.org/326002/diff/3001/4002#newcode551
Line 551: new net::CompletionCallbackImpl<AppCacheUpdateJob>(
ditto... more common to define inline

[michaeln, 2009-10-28 21:57:25]

some more comments

http://codereview.chromium.org/326002/diff/3001/4002
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/326002/diff/3001/4002#newcode68
Line 68: int bytes_to_write_;  // remember data bytes while writing http
response info
maybe call this 'initial_bytes_to_write_', and instead of delivering the value
back to the UpdateJob via an argument in each call to OnWriteReponseComplete,
have that method check for a non-zero value in the info class... or maybe have a
seperate UpdateJob method for OnWriteResponseInfoComplete? Something like that
may be less awkward... wdyt?

Info::OnWriteComplete(int result) {
 if (initial_bytes_to_write_)
    update_job_->OnWriteResponseInfoComplete(result, info);
 else
    update_job_->OnWriteResponseComplete(result, info);
}

http://codereview.chromium.org/326002/diff/3001/4002#newcode256
Line 256: info->response_writer_.get()->WriteInfo(
How you're handling the refcounted ioBuffer parameter is susceptible to leaks.
If the callee doesn't take then later release a ref to the ioBuffer, the object
will be leaked. There are code paths where the callee may skip the taking a ref
of its own. A safer pattern is to load the newly created refcounted object into
a scoped_refptr in the current scope as follows...

scoped_refptr<HttpResponseInfoIOBuffer> info_buffer(new ...);
info->response_writer_->WriteInfo(info_buffer, callback);

... on exit from the current scope, Release will get called and if the callee
didn't take a ref of its own, the object will be deleted.

Ditto other refcounted info ioBuffer arg usages for the writing of the manifest
entry.


Also, with the smart ptr, you don't need to the .get() to invoke the method.

http://codereview.chromium.org/326002/diff/3001/4002#newcode536
Line 536: new net::CompletionCallbackImpl<AppCacheUpdateJob>(
ditto... more common to define inline

also, where is this heap allocated block deleted?

http://codereview.chromium.org/326002/diff/3001/4002#newcode577
Line 577: service_->storage()->StoreGroupAndNewestCache(group_, this);  // async
I think we may have some racey'ness here, and I missed this in the design of the
storage APIs :(

Our data structures can get into an inconsistent state if the the store call
fails. The in-memory group will reflect the new cache, but that's not how it is
on disk, and thats not how it will look after the group falls out of the
working-set and gets reloaded into memory.

Thoughts?

http://codereview.chromium.org/326002/diff/3001/4002#newcode868
Line 868: // has not been committed by actually writing the cache to storage?)
ditto

[jennb, 2009-10-29 21:25:40]

New patch uploaded.  Ignore patch 3 and go straight to patch 4  - I thought of a
couple more tests right after uploading patch 3 and made 2 back-to-back uploads.

http://codereview.chromium.org/326002/diff/3001/4002
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/326002/diff/3001/4002#newcode40
Line 40: void SetUpResponseWriter(AppCacheResponseWriter* writer,
On 2009/10/28 21:19:52, michaeln wrote:
> nit: Setup... but i see both SetUp and Setup in use in the code base

I was once lectured: "set up" is a verb, "setup" is a noun.

http://codereview.chromium.org/326002/diff/3001/4002#newcode49
Line 49: write_callback_.reset(new net::CompletionCallbackImpl<UpdateJobInfo>(
On 2009/10/28 21:19:52, michaeln wrote:
> its much more common to define storage for CallbackImpls inline rather than
heap
> allocate them...
> 
> class UpdateJobInfo {
>   UpdateJobInfo() 
>       : bar_(0),
>         ALLOW_THIS_IN_INITIALIZER_LIST(
>             write_callback_(this, &UpdateJobInfo::OnWriteComplete)) {
>   }
> 
>   int bar_;
>   net::CompletionCallbackImpl<UpdateJobInfo> write_callback_;
> };
> 
> 

Done.

http://codereview.chromium.org/326002/diff/3001/4002#newcode68
Line 68: int bytes_to_write_;  // remember data bytes while writing http
response info
On 2009/10/28 21:19:52, michaeln wrote:
> is this really specific to 'response info' or is this for the 'data'... oh i
see
> (after looking at the callsite)... after 'info' is written then you write this
> amount of 'data'.
> 
> that's a little awkward?

Reworked flow to write all data first, then write response info.

http://codereview.chromium.org/326002/diff/3001/4002#newcode232
Line 232: int bytes_read) {
On 2009/10/28 21:19:52, michaeln wrote:
> Can you add a DCHECK(bytes_read > 0) here so readers know w/o examining the
> callsite?

Done.

http://codereview.chromium.org/326002/diff/3001/4002#newcode239
Line 239: return false;  // wait for async storage callback
On 2009/10/28 21:19:52, michaeln wrote:
> nit: maybe "wait for async completion to continue reading"... if it fits on
the
> line?

Done.

http://codereview.chromium.org/326002/diff/3001/4002#newcode256
Line 256: info->response_writer_.get()->WriteInfo(
On 2009/10/28 21:57:25, michaeln wrote:
> How you're handling the refcounted ioBuffer parameter is susceptible to leaks.
> If the callee doesn't take then later release a ref to the ioBuffer, the
object
> will be leaked. There are code paths where the callee may skip the taking a
ref
> of its own. A safer pattern is to load the newly created refcounted object
into
> a scoped_refptr in the current scope as follows...
> 
> scoped_refptr<HttpResponseInfoIOBuffer> info_buffer(new ...);
> info->response_writer_->WriteInfo(info_buffer, callback);
> 
> ... on exit from the current scope, Release will get called and if the callee
> didn't take a ref of its own, the object will be deleted.
> 
> Ditto other refcounted info ioBuffer arg usages for the writing of the
manifest
> entry.
> 
> 
> Also, with the smart ptr, you don't need to the .get() to invoke the method.

Done.

http://codereview.chromium.org/326002/diff/3001/4002#newcode467
Line 467: entry.set_response_id(info->response_writer_.get()->response_id());
On 2009/10/28 21:19:52, michaeln wrote:
> response_writer_-> should work here

Done.

http://codereview.chromium.org/326002/diff/3001/4002#newcode482
Line 482: // might be smart enough to discard uncommitted writes?)
On 2009/10/28 21:19:52, michaeln wrote:
> i think we can leave this out for now, and revisit if needed when we get to
> persistent storage... wdyt?

Done.

http://codereview.chromium.org/326002/diff/3001/4002#newcode536
Line 536: new net::CompletionCallbackImpl<AppCacheUpdateJob>(
On 2009/10/28 21:57:25, michaeln wrote:
> ditto... more common to define inline
> 
> also, where is this heap allocated block deleted?

Done.

http://codereview.chromium.org/326002/diff/3001/4002#newcode549
Line 549: manifest_response_writer_.get()->WriteData(
On 2009/10/28 21:19:52, michaeln wrote:
> don't need the .get() on the smart ptr

Done.

http://codereview.chromium.org/326002/diff/3001/4002#newcode551
Line 551: new net::CompletionCallbackImpl<AppCacheUpdateJob>(
On 2009/10/28 21:19:52, michaeln wrote:
> ditto... more common to define inline

Done.

http://codereview.chromium.org/326002/diff/3001/4002#newcode577
Line 577: service_->storage()->StoreGroupAndNewestCache(group_, this);  // async
On 2009/10/28 21:57:25, michaeln wrote:
> I think we may have some racey'ness here, and I missed this in the design of
the
> storage APIs :(
> 
> Our data structures can get into an inconsistent state if the the store call
> fails. The in-memory group will reflect the new cache, but that's not how it
is
> on disk, and thats not how it will look after the group falls out of the
> working-set and gets reloaded into memory.
> 
> Thoughts?

Added code to restore in-memory state of group upon storage failure.

http://codereview.chromium.org/326002/diff/3001/4002#newcode868
Line 868: // has not been committed by actually writing the cache to storage?)
On 2009/10/28 21:57:25, michaeln wrote:
> ditto

Done.

[michaeln, 2009-10-30 00:25:30]

http://codereview.chromium.org/326002/diff/3001/4002
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/326002/diff/3001/4002#newcode577
Line 577: service_->storage()->StoreGroupAndNewestCache(group_, this);  // async

> Added code to restore in-memory state of group upon storage failure.

I think this is OK for now, but its still racey. Can you add a TODO here to
revisit at a later date?

http://codereview.chromium.org/326002/diff/12001/13002#newcode281
Line 281: return;
Can the request be complete prior to the caller having read everything out of
it? I'm not sure. Should we read until we hit EOF instead?

http://codereview.chromium.org/326002/diff/12001/13005
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/326002/diff/12001/13005#newcode162
Line 162: delegate_ref->delegate) {
you could go down this code path even if the delegate_ref has been dropped...
so...

if (simulate_failure) {
  if (delegate_ref->delegate()
    call it
}

So long as simulate_failure was on... the ops would fail regardless of
delegate_ref status.

[jennb, 2009-10-30 17:31:17]

New patch uploaded.

http://codereview.chromium.org/326002/diff/3001/4002
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/326002/diff/3001/4002#newcode577
Line 577: service_->storage()->StoreGroupAndNewestCache(group_, this);  // async
On 2009/10/30 00:25:30, michaeln wrote:
> 
> > Added code to restore in-memory state of group upon storage failure.
> 
> I think this is OK for now, but its still racey. Can you add a TODO here to
> revisit at a later date?
> 

Done.

http://codereview.chromium.org/326002/diff/12001/13002#newcode281
Line 281: return;
On 2009/10/30 00:25:31, michaeln wrote:
> Can the request be complete prior to the caller having read everything out of
> it? I'm not sure. Should we read until we hit EOF instead?

Reworked. Now info is written immediately after first data write completes and
we read to EOF. Trying to do all of one and then the other seemed logically
simpler, but turned out messier in code.

http://codereview.chromium.org/326002/diff/12001/13005
File webkit/appcache/mock_appcache_storage.cc (right):

http://codereview.chromium.org/326002/diff/12001/13005#newcode162
Line 162: delegate_ref->delegate) {
On 2009/10/30 00:25:31, michaeln wrote:
> you could go down this code path even if the delegate_ref has been dropped...
> so...
> 
> if (simulate_failure) {
>   if (delegate_ref->delegate()
>     call it
> }
> 
> So long as simulate_failure was on... the ops would fail regardless of
> delegate_ref status.

Done.

[michaeln, 2009-10-30 18:48:16]

LGTM (one minor style thing)

http://codereview.chromium.org/326002/diff/9002/9004
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/326002/diff/9002/9004#newcode240
Line 240: this, request);
indents are off here

[jennb, 2009-10-30 18:50:33]

http://codereview.chromium.org/326002/diff/9002/9004
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/326002/diff/9002/9004#newcode240
Line 240: this, request);
On 2009/10/30 18:48:16, michaeln wrote:
> indents are off here

Done.