Issue 917958  has been merged into this issue.

I can reproduce this on a local build from yesterday master.

Note, I actually get a corrupted file whether its being served from cache or not.  Its just truncated more when service from cache_storage.

If I disable service worker servicification in chrome://flags the problem appears to go away.

Matt, I hate to ask, but do you think we need to disable servicification in 71 again?

I want to echo that we are seeing this same issue in Chrome 71+.

It's not consistent but PDF access through a service worker seems to cause a gray screen / failed download.

Our service worker doesn't touch the cache api at all.

This could have the same root cause as  issue 916514 .

Unfortunately disabling servicification would reopen  issue 913220  which may be as serious.

cc: +mek (blobs), +qimin (downloads), +rockot (mojo) if you have ideas.

I could repro with the repro steps in the bug but I don't see the root cause yet. What's happening is:
1. The load goes through ServiceWorkerNavigationLoader which uses Mojo Blob to write 
   the data to the URLLoaderClient (which I think is ultimately DownloadResponseHandler). 
   SWNL calls blob->ReadAll() and passes the consumer end of the data pipe to
   URLLoaderClient::OnStartLoadingResponseBody.
2. The download manager reads chunks of 4096 bytes from the data pipe in StreamHandleInputStream::Read.
   Once it hits MOJO_RESULT_FAILED_PRECONDITION it stops reading.
3. Somehow StreamHandleInputStream is getting MOJO_RESULT_FAILED_PRECONDITION too early.
   The blob->ReadAll() call reports that it wrote the full contents to the data pipe (2056110 bytes).
   But the download code reads something like 1597440 bytes, always a multiple of 4096, before getting
   FAILED_PRECONDITION.

I don't know what triggers MOJO_RESULT_FAILED_PRECONDITION. I assumed the producer end of the data pipe could close and the consumer end could continue reading all the data that was written. It's not clear if there is some intermediary between ServiceWorkerNavigationLoader and DownloadResponseHandler. The ScopedDataPipeConsumerHandle SWNL passes to OnStartLoadingResponseBody has a different value() than the ScopedDataPipeConsumerHandle in DownloadResponseHandler::OnStartLoadingResponseBody().

I think it's possible for mojo to return precondition fail even if data is left for zero length reads.  You can use QuerySignalState instead of a zero length read like this:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/fetch/data_pipe_bytes_consumer.cc?sq=package:chromium&g=0&l=233

But if it's really using a read size of 4096 then it seems this should not be a problem.

Are we sure all the data was written on the producer side?

There is at least one bug in https://cs.chromium.org/chromium/src/components/download/internal/common/stream_handle_input_stream.cc?sq=package:chromium&dr=CSs&g=0&l=75, where we reinterpret_cast (disguised as a  c-style cast) a size_t* to a uint32_t*, which of course is never a valid thing to do. Not sure if/how that could explain the results we're seeing, since in practice it seems like we should be getting lucky since we don't support big endian systems anyway.

It's also not clear to me how the download code deals with the fact that OnStreamCompleted can be called before all bytes have been read from the datapipe. I.e it doesn't seem like it even sets completion_callback_ until the datapipe has finished reading, but it is very well possible for OnStreamCompleted to be called before hand (since it is send over an entirely separate mojo pipe). Again not sure if/how that would be related to this bug though, and I might be missing something since I haven't looked at any of the downloads code before today.

I believe the problem is MimeSniffingURLLoader.  It interposes itself in the stream.  It also calls OnComplete() before the body is completely proxied.  This can cause the loader to be destroyed before the body is fully sent.

https://chromium-review.googlesource.com/c/chromium/src/+/1396922

The CL still needs a test.

Thanks a ton for the investigation and fix! I'll try to push things forward by adding a test.

Just writing notes for my own reference: It made sense to me that the loader shouldn't send OnComplete() until it finished sending the data, as otherwise it could give an OK status when an ERR would be appropriate if a failure happened during sending. However it wasn't clear to me if it was a bug that the loader client was destroying the loader on OnComplete() before it finished reading all the data (i.e., waiting for both EOF on the data pipe and the OnComplete() message). But I guess it's valid, OnComplete() signals that the loader finished writing the data, so it's OK to destroy the loader and the client can still expect to read all the data that was written to the pipe. In this case, it looks like the loader is being destroyed via DownloadResponseHandler::OnComplete -> ResourceDownloader::OnResponseCompleted -> ResourceDownloader::Destroy which stops the download and destroys the loader, causing MOJO_RESULT_FAILED_PRECONDITION when reading the data pipe.

Revised comment #14's CL to https://chromium-review.googlesource.com/c/chromium/src/+/1395862

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

commit 577e198fdc76a66c5fede9425d4566b97ceed14b
Author: Matt Falkenhagen <falken@chromium.org>
Date: Sun Jan 06 23:02:43 2019

Fix when MimeSniffingURLLoader calls OnComplete().

MimeSniffingURLLoader was calling OnComplete() before it finished
writing all the data for the body. This caused truncated downloads since
the download handler was destroying the loader once it received
OnComplete(), so the loader couldn't finish writing all the data.

This CL is based on the original CL from wanderview@ (see bug).

Bug:  918944 
Change-Id: I2f0013ca43aa740bdf54603b0228e1f7f2b7eed5
Reviewed-on: https://chromium-review.googlesource.com/c/1395862
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620233}
[modify] https://crrev.com/577e198fdc76a66c5fede9425d4566b97ceed14b/content/common/mime_sniffing_throttle_unittest.cc
[modify] https://crrev.com/577e198fdc76a66c5fede9425d4566b97ceed14b/content/common/mime_sniffing_url_loader.cc
[modify] https://crrev.com/577e198fdc76a66c5fede9425d4566b97ceed14b/content/common/mime_sniffing_url_loader.h

This bug also impacts Network Service, updating labels.

This is a bug with Chrome's mime sniffing mechanism. A potential workaround while the bug is being fixed is to disable mime sniffing by either:
1. Explicitly setting Content-Type to a non-generic type. E.g., "plain/text" and "application/octet-stream" still trigger mime sniffing in Chrome. So a type like "application/octet-stream-nosniff" or just "application/dummy-type" should disabling sniffing.
2. Setting X-Content-Type-Options: nosniff.

I'm not totally sure of the implications of doing these as opposed to using a generic content-type and allowing mime sniffing. 

I was asked to expand on the workaround. This would be a workaround at the server rather than in Chrome settings. The server would set Content-Type or X-Content-Type-Options HTTP headers on responses that are meant to be downloaded. This header adjustment could potentially be done by an http proxy between the site and the browser.

A workaround in Chrome settings for users is to go to chrome://flags and disable "#enable-service-worker-servicification".

Talking with landry@ it seems he has also found another work-around.  In your service worker if you copy the response like this for downloads:

  let r1 = await c.match(request);
  let r2 = new Response(r1.body, r1);

The frequency of corruption should be greatly reduced.

I found this a bit surprising so I investigated a bit and I think this is why this work-around works:

1) Copying the response this way forces the body to be proxied through a ReadableStream on the service worker thread.
2) The loading "completion" signal is not triggered until the ReadableStream has proxies all the data.
3) The pipe being filled by the ReadableStream and consumed by the MimeSniffingURLLoader is 64kb in size.
4) The pipe being filled by the BlobReader is 512kb in size.
5) So when the ReadableStream proxies the data there is at most 64kb of data left to consumer when it starts triggering the Complete() signal.  This is much less than the potential 512kb of buffered data when reading straight from the blob.
6) So there is still a race, but its much more likely that MimeSniffingURLLoader will consume the 64kb of buffered data before the completion signal is received.

There is still the potential for some corruption with this workaround, but the frequency should be greatly reduced.  Setting the "x-content-type-options: nosniff" header should avoid the corruption bug completely.

Reminder M72 Stable is coming VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure to land the fix and request a merge into the release branch ASAP. Thank you.

Request merge of c#18 to M72. The CL landed in 73.0.3664.0 and I verified it's working in the latest Canary. The crash dashboard looks fine too.[1]

[1] https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27+AND+expanded_custom_data.ChromeCrashProto.channel%3D%27canary%27&compProp=product.Version&v1=73.0.3664.0&v2=73.0.3663.0

This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Approving merge to M72 branch 3626 based on comment #24. Pls merge ASAP so we can pick it up for this week beta release. Thank you.

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

commit 7bed44b80bf44e37e0924eba102892246414d247
Author: Matt Falkenhagen <falken@chromium.org>
Date: Tue Jan 08 06:21:11 2019

M72: Fix when MimeSniffingURLLoader calls OnComplete().

MimeSniffingURLLoader was calling OnComplete() before it finished
writing all the data for the body. This caused truncated downloads since
the download handler was destroying the loader once it received
OnComplete(), so the loader couldn't finish writing all the data.

This CL is based on the original CL from wanderview@ (see bug).

Bug:  918944 
Change-Id: I2f0013ca43aa740bdf54603b0228e1f7f2b7eed5
Reviewed-on: https://chromium-review.googlesource.com/c/1395862
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#620233}(cherry picked from commit 577e198fdc76a66c5fede9425d4566b97ceed14b)
Reviewed-on: https://chromium-review.googlesource.com/c/1399689
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#604}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/7bed44b80bf44e37e0924eba102892246414d247/content/common/mime_sniffing_throttle_unittest.cc
[modify] https://crrev.com/7bed44b80bf44e37e0924eba102892246414d247/content/common/mime_sniffing_url_loader.cc
[modify] https://crrev.com/7bed44b80bf44e37e0924eba102892246414d247/content/common/mime_sniffing_url_loader.h

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

Commit: 7bed44b80bf44e37e0924eba102892246414d247
Author: falken@chromium.org
Commiter: falken@chromium.org
Date: 2019-01-08 06:21:11 +0000 UTC

M72: Fix when MimeSniffingURLLoader calls OnComplete().

MimeSniffingURLLoader was calling OnComplete() before it finished
writing all the data for the body. This caused truncated downloads since
the download handler was destroying the loader once it received
OnComplete(), so the loader couldn't finish writing all the data.

This CL is based on the original CL from wanderview@ (see bug).

Bug:  918944 
Change-Id: I2f0013ca43aa740bdf54603b0228e1f7f2b7eed5
Reviewed-on: https://chromium-review.googlesource.com/c/1395862
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#620233}(cherry picked from commit 577e198fdc76a66c5fede9425d4566b97ceed14b)
Reviewed-on: https://chromium-review.googlesource.com/c/1399689
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#604}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Can this be marked as fixed if nothing else is pending? It is unlikely we will respin M71.

Removing 71 target per c#29.

 Issue 922021  has been merged into this issue.