ServiceWorkerWriteToCacheJob: refactor

ServiceWorkerWriteToCacheJob: refactor

This CL is a re-land of a previously reviewed and approved CL that was reverted: https://codereview.chromium.org/1315443003

ServiceWorkerWriteToCacheJob is a subclass of URLRequestJob that is responsible for filling and updating a write-through cache for ServiceWorker data. Specifically, ServiceWorkerWriteToCacheJob creates an underlying URLRequestJob which it uses to do the actual network request for the script, then returns the script body to the creator of the ServiceWorkerWriteToCacheJob while also storing it in the ServiceWorker cache if it has been updated.

In order to avoid spurious cache rewrites, ServiceWorkerWriteToCacheJob defers writing an entry for as long as possible: if there is an existing entry, it compares incoming network data to the cache entry's data, and only starts writing once there is a mismatch. Since the mismatch may occur after much data has already been handled, once this happens ServiceWorkerWriteToCacheJob must transparently copy all the existing data up to that point from the cache, then resume streaming network data into the cache and back to its consumer. This is the "compare-and-copy" process. If there is no existing entry, network data is streamed directly to the cache; this is the "passthrough" process.

This CL removes the code implementing the compare-and-copy and passthrough processes from ServiceWorkerWriteToCacheJob and moves it into a separate class, ServiceWorkerCacheWriter. ServiceWorkerCacheWriter exposes an asynchronous API for overwriting a single cache entry that is now used by ServiceWorkerWriteToCacheJob.

BUG=474859
TBR=jochen,falken

Committed: https://crrev.com/d778a5a82cdccab2b3cbf4940749753c98e59dcc
Cr-Commit-Position: refs/heads/master@{#349669}

patch from issue 1315443003 at patchset 260001 (http://crrev.com/1315443003#ps260001)

Committed: https://crrev.com/015f4a69ad39e4625648e160dde9b75f8aaad522
Cr-Commit-Position: refs/heads/master@{#350824}

[Elly Fong-Jones, 2015-09-21 19:03:47]

ellyjones@chromium.org changed reviewers:
+ falken@chromium.org, mmenke@chromium.org

[Elly Fong-Jones, 2015-09-21 19:04:20]

Hi,

Please take a look. This is a reupload of
https://codereview.chromium.org/1315443003/ (as the first patchset) with a fix
applied in the second patchset.

[mmenke, 2015-09-23 15:23:55]

Sorry for slowness - wanted to get my AUTH fix out for review (Which I consider
an M47 blocker) before getting to this, but looks like that CL's going to take a
bit more work than I expected.

Not going to do a full re-review of this, but want to understand the class well
enough to understand the bug, and a regression test for the bug, which we really
should have.

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_cache_writer.h (right):

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_cache_writer.h:59: virtual
~ServiceWorkerCacheWriter();
virtual not needed.

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:394:
NotifyReadComplete(result);
Should we add a unittest to catch whatever broke?

[mmenke, 2015-09-23 15:24:30]

I'm still looking at this, just wanted to request a test sooner rather than
later, so not to block you quite so much on this.

[mmenke, 2015-09-23 16:39:58]

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:164:
HandleNetData(*bytes_read);
If bytes_read is 0 here, and HandleNetData completes synchronously, don't you
need to NotifyDone / NotifyDoneHelper here, too, like in
ServiceWorkerWriteToCacheJob::OnReadCompleted?

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:393:
NotifyDoneHelper(GetStatus(), std::string());
We're calling NotifyFinishedCaching twice in this case, one in HandleNetData,
and once here.  Is that a problem?

[Elly Fong-Jones, 2015-09-24 17:09:15]

ptal?

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_cache_writer.h (right):

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_cache_writer.h:59: virtual
~ServiceWorkerCacheWriter();
On 2015/09/23 15:23:55, mmenke wrote:
> virtual not needed.

Done.

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:164:
HandleNetData(*bytes_read);
On 2015/09/23 16:39:58, mmenke wrote:
> If bytes_read is 0 here, and HandleNetData completes synchronously, don't you
> need to NotifyDone / NotifyDoneHelper here, too, like in
> ServiceWorkerWriteToCacheJob::OnReadCompleted?

Ugh. ReadRawData is supposed to call NotifyDone unless it succeeds
synchronously, so I do need to call NotifyDoneHelper in the error case only in
case HandleNetData signalled an error. :( Done

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:393:
NotifyDoneHelper(GetStatus(), std::string());
On 2015/09/23 16:39:58, mmenke wrote:
> We're calling NotifyFinishedCaching twice in this case, one in HandleNetData,
> and once here.  Is that a problem?

Nope, it checks for that.

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:394:
NotifyReadComplete(result);
On 2015/09/23 15:23:55, mmenke wrote:
> Should we add a unittest to catch whatever broke?

It is fiddly to unit-test specific sync/async behaviors in this code. The
existing unit tests work at the URLRequest layer of abstraction and don't have
fine-grained control over the URLRequestJob subclass's state machine. I can hack
a test for this in but the rest of the code isn't tested at this granularity and
the test will be quite ugly implementation-wise because of it. This case is
covered by the layout test that originally caught the bug, though.

[mmenke, 2015-09-24 17:52:07]

LGTM, and sorry I didn't get to this faster.  That's becoming a common refrain
from me.  :(

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:164:
HandleNetData(*bytes_read);
On 2015/09/24 17:09:15, Elly Jones wrote:
> On 2015/09/23 16:39:58, mmenke wrote:
> > If bytes_read is 0 here, and HandleNetData completes synchronously, don't
you
> > need to NotifyDone / NotifyDoneHelper here, too, like in
> > ServiceWorkerWriteToCacheJob::OnReadCompleted?
> 
> Ugh. ReadRawData is supposed to call NotifyDone unless it succeeds
> synchronously, so I do need to call NotifyDoneHelper in the error case only in
> case HandleNetData signalled an error. :( Done

That's bizarre, but looks like you're right.

https://codereview.chromium.org/1356153002/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_write_to_cache_job.cc:394:
NotifyReadComplete(result);
On 2015/09/24 17:09:15, Elly Jones wrote:
> On 2015/09/23 15:23:55, mmenke wrote:
> > Should we add a unittest to catch whatever broke?
> 
> It is fiddly to unit-test specific sync/async behaviors in this code. The
> existing unit tests work at the URLRequest layer of abstraction and don't have
> fine-grained control over the URLRequestJob subclass's state machine. I can
hack
> a test for this in but the rest of the code isn't tested at this granularity
and
> the test will be quite ugly implementation-wise because of it. This case is
> covered by the layout test that originally caught the bug, though.

I'll defer to you on that (Particularly as the filter refactor should vastly
improve the state of things here).

[Elly Fong-Jones, 2015-09-25 14:37:06]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2015-09-25 14:37:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356153002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356153002/40001

[commit-bot: I haz the power, 2015-09-25 14:51:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-25 14:52:33]

Patchset 3 (id:??) landed as
https://crrev.com/015f4a69ad39e4625648e160dde9b75f8aaad522
Cr-Commit-Position: refs/heads/master@{#350824}

[horo, 2015-09-29 07:30:37]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1376673002/ by horo@chromium.org.

The reason for reverting is: This change has broken the Service Worker update
logic.
See crbug.com/537098
.