+falken@: will you be able to take a look at this?


ccing mmenke@ who was looking at this with me last week.

Yep, thanks very much for filing this, I'll take a look. Marking it as a blocker for launching "S13nSW" (SW for Network Service).

By the way, it turns out that this problem is being masked by a bug in mojo ( Issue 821030 ). 

If you apply this patch:
https://chromium-review.googlesource.com/c/chromium/src/+/957918, some of ServiceWorkerNewScriptLoader will fail as expected.

ServiceWorkerCacheWriter doesn't do partial write, so the problem is that it doesn't release the IOBuffer after writes. I guess that's why CompleteRead() and ReleaseHandle() are explicitly used.
Btw, MojoToNetIOBuffer doesn't understand if the handle has been released when it's destructed. 
Created a CL for possible fix: crrev.com/c/963158

> ServiceWorkerCacheWriter doesn't do partial write,

That's not the problem no. Before SWCacheWriter gets a go at the data you first pass the data to another DataPipe, and it is that WriteData call that is likely to do a partial write. (not to mention the explicit kReadBufferSize capping ServiceWorkerNewScriptLoader::WriteData does that could result in a partial read as well).

> Btw, MojoToNetIOBuffer doesn't understand if the handle has been released when it's destructed. 

I'd argue that it is part of the contract of MojoToNetIOBuffer that you're not supposed to call ReleaseHandle before first calling CompleteRead...

Thanks for your comments!
And sorry for lack of my explanation...

Re c#7:
Thanks, I understand the point.
That partial write (to the |client_producer_|, in this case) is a problem and we need to fix it.

Re c#8:
Just thinking of the crashes by crrev.com/c/957918, it indicates that SWCacheWriter owns IOBuffer created for the previous write (even after CompleteRead() and ReleaseHandle()) until it's destructed or the next MaybeWriteData() happens. When SWCacheWriter gets destructed, the IOBuffer is also destructed, and it crashes since ReleaseHandle() has been called before.
However, I'm not sure if the lazy release of IOBuffer should crash. Otherwise, all ReleaseHandle()s before desctruction of MojoToNetIOBuffer can cause crashes.
I don't know it's intentional behavior. WDYT?

The detailed flow of code path would be like as follows:
1: SWNewScriptLoader reads data from the network (SWNewScriptLoader::OnNetworkDataAvailable()) and calls SWNewScriptLoader::WriteData()
2: SWNewScriptLoader::WriteData() writes the data to the |client_producer_| pipe, and also calls SWCacheWriter::MaybeWriteData()
3: SWCacheWriter writes the data to the cache with having the ownership of the IOBuffer
4: SWNewScriptLoader::OnWriteDataComplete() is called after a write to the cache has completed. CompleteRead() and ReleaseHandle() is called and the owner of the handle gets back to the SWNewScriptLoader.
5: SWNewScriptLoader::OnNetworkDataAvailable() is called because pipe has been closed.
6: SWNewScriptLoader::CommitCompleted() is called. SWCacheWriter is destructed.
7: SWCacheWriter still owns the IOBuffer, so the destruction happens. ~MojoToNetIOBuffer() internally calls CompleteRead again.
8: Crash happens since the handle has already been moved to the SWNewScriptLoader.

I think the simplest fix for SWNewScriptLoader is to stop using MojoToNetIOBuffer and to use net::WrappedIOBuffer(pending_buffer->buffer()) directly.

That way, when you tell mojo handle that bytes are written through " pending_buffer->CompleteRead(bytes_written)", mojo will perform the correct accounting. You need not worry about MojoToNetIOBuffer's destructor calling CompleteRead() when you are in fact not done with consuming that data.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c75747afd32c21a843fc9762a088056cc09294c

commit 1c75747afd32c21a843fc9762a088056cc09294c
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Fri Apr 06 06:59:25 2018

ServiceWorker: Use WrappedIOBuffer instead of MojoToNetIOBuffer

Previously we use network::MojoToNetIOBuffer, and it ensures CompleteRead() is
called when its destruction. However, CompleteRead() may be called again when a
SWCacheWriter gets destructed after a write has done and ReleaseHandle() has
been called. This CL avoids that by using WrappedIOBuffer instead of
MojoToNetIOBuffer.

TEST=content_unittests '--gtest_filter=ServiceWorkerNewScript*' with patching crrev.com/c/957918

Bug:  820540 
Change-Id: I3419feaca83aca277a4216970104bceeb423506a
Reviewed-on: https://chromium-review.googlesource.com/991626
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548700}
[modify] https://crrev.com/1c75747afd32c21a843fc9762a088056cc09294c/content/browser/service_worker/service_worker_new_script_loader.cc