[ServiceWorker] Handle bytes_read=0 async result correctly in ServiceWorkerWriteToCacheJob.

[ServiceWorker] Handle bytes_read=0 async result correctly in ServiceWorkerWriteToCacheJob.

The async result of net_request_->Read() in ServiceWorkerWriteToCacheJob::ReadNetData() is passed to ServiceWorkerWriteToCacheJob::OnReadCompleted().
In current code the case when bytes_read is 0 is not handled correctly.
So when bytes_read is 0 the script loading will hang.

LayoutTest is here: https://codereview.chromium.org/650193003

I hope this patch will reduce the test flakiness :)

BUG=419999
TEST=http/tests/serviceworker/chromium/load-flushed-script.html

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

[horo, 2014-10-16 07:52:57]

horo@chromium.org changed reviewers:
+ michaeln@chromium.org

[horo, 2014-10-16 07:52:57]

michaeln@
Could you please review this?

[michaeln, 2014-10-16 23:37:12]

thanx for characterizing and fixing this bug!

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:374:
NotifyReadComplete(0);
I think we need to do the following to actually retain the resources in the disk
cache.

version_->script_cache_map()->NotifyFinishedCaching(url_, status);
did_notify_finished_ = true;

Given the call to  NotifyReadComplete(0) is needed here, is it also needed are
line 365 and 382?

[michaeln, 2014-10-17 00:07:31]

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:374:
NotifyReadComplete(0);
> I think we need to do the following to actually retain the resources in the
disk
> cache.
> 
> version_->script_cache_map()->NotifyFinishedCaching(url_, status);
> did_notify_finished_ = true;

Yup, those two lines need to go before the call to NotifyReadComplete() because
the job get's killed under that.

> Given the call to  NotifyReadComplete(0) is needed here, is it also needed are
> line 365 and 382?

Nope for 365, as long as 'status is not success NotifyDone will call
NotifyReadComplete internally.

I'm not sure about 382? I'm not sure that's even reachable anymore. Can there be
a negative bytes_read AND a successful status? I think probably not.

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:380:
"WriteHeadersToCache");
unrelated to this cl, but i don't get what this trace macro is doing here?

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:388:
DCHECK(!status.is_io_pending());
Can you also add DCHECK(!did_notify_finished_) here?

[horo, 2014-10-17 02:39:12]

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:374:
NotifyReadComplete(0);
On 2014/10/17 00:07:31, michaeln wrote:
> > I think we need to do the following to actually retain the resources in the
> disk
> > cache.
> > 
> > version_->script_cache_map()->NotifyFinishedCaching(url_, status);
> > did_notify_finished_ = true;
> 
> Yup, those two lines need to go before the call to NotifyReadComplete()
because
> the job get's killed under that.
> 
> > Given the call to  NotifyReadComplete(0) is needed here, is it also needed
are
> > line 365 and 382?
> 
> Nope for 365, as long as 'status is not success NotifyDone will call
> NotifyReadComplete internally.
> 
> I'm not sure about 382? I'm not sure that's even reachable anymore. Can there
be
> a negative bytes_read AND a successful status? I think probably not.

Added before 373.
I think 382 is not reachable. I changed to use bytes_read in if statement at
364.

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:380:
"WriteHeadersToCache");
On 2014/10/17 00:07:31, michaeln wrote:
> unrelated to this cl, but i don't get what this trace macro is doing here?

Removed.
We don't need it here.

https://codereview.chromium.org/640923003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:388:
DCHECK(!status.is_io_pending());
On 2014/10/17 00:07:31, michaeln wrote:
> Can you also add DCHECK(!did_notify_finished_) here?

Done.

[michaeln, 2014-10-17 19:23:10]

lgtm, thnx!

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

https://codereview.chromium.org/640923003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:373: // No
more data to process, the job is complete.
worth a DCHECK(request->status().is_success()) here?

https://codereview.chromium.org/640923003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:380: return;
delete the unneeded return statement too

[horo, 2014-10-20 02:06:50]

Thank you!

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

https://codereview.chromium.org/640923003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:373: // No
more data to process, the job is complete.
On 2014/10/17 19:23:10, michaeln wrote:
> worth a DCHECK(request->status().is_success()) here?

Done.

https://codereview.chromium.org/640923003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:380: return;
On 2014/10/17 19:23:10, michaeln wrote:
> delete the unneeded return statement too

Done.

[horo, 2014-10-20 02:06:53]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-20 02:08:24]

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

[commit-bot: I haz the power, 2014-10-20 09:33:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-20 09:34:18]

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