https://static-misc.glitch.me/sw-audio-bug/ - here's another bug caused by the same issue. This one's simpler.

In stable, the audio is playable and seekable.

In Canary, the audio isn't seekable, and the service worker gets spammed with requests. You can see the ERR_REQUEST_RANGE_NOT_SATISFIABLE errors in the console.

Passing --disable-features="ServiceWorkerServicification" on the command line seems to fix the problem for me in a local build.

I think we need to fix this TODO.
https://cs.chromium.org/chromium/src/content/common/service_worker/service_worker_loader_helpers.cc?l=191&rcl=89d25d512786f1fd90ba18ada5b49078d8793cac

That's strange because I thought the non-s13nsw path had the same limitation. I'm not sure how it gets handled in that path. I'll take a look.

Confirmed on local build that it works when both S13nSW and NetworkService are disabled, and breaks when either of them are enabled.

The request range is [851968,-1] which ExtractSinglePartHttpRange() considers invalid. It only allows [0,-1] if the last pos is -1. Looks overly restrictive.

But a problem is that Blob's interface has only ReadRange(offset, length) or ReadAll() and it might be possible length isn't known here.

The non-s13n path reads the Blob using GetBlobDataFromUUID(response->blob->uuid) and then BlobProtocolHandler::CreateBlobRequest(). It doesn't seem to do parsing of the request headers on its own so I don't know how those are handled. I wonder if in the non-s13n path we just give the full response and leave it up to the client to pick out the range it wanted?

I should probably document this, but it is perfectly valid/supported to pass -1/uint64max (or any other arbitrarily large value) as length to ReadRange, and it will just cap it at the size of the blob.

Update: I changed Blob reading to accept -1 but Jake's site is still not working. Trying to seek doesn't result in a request to the service worker.

In S13n, MultibufferDataSource has streaming_ == true which prevents seeking.

In non-S13n, ResourceMultiBufferDataProvider::DidReceiveResponse gets a response with a content-length. In S13n the content-length is -1.

It looks like we need to set ResourceResponseHead.content_length from the HTTP header value like we do for mime type and charset here:
https://cs.chromium.org/chromium/src/content/common/service_worker/service_worker_loader_helpers.cc?l=101&rcl=27d5bb460978eff41f46e0f59d7ea12520a06516

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

commit 56dd83c9005f68d55ddbf85c551759526b4e37cd
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri Oct 05 09:32:53 2018

service worker: s13n: Support partial responses from blobs better.

This CL allows media to play and be seekable when service worker
provides the response from cache storage.
Live demo from Jake Archibald: https://static-misc.glitch.me/sw-audio-bug/

This CL fixes two things:
- Supports blob responses from service worker for partial requests with
  a range of [x, -1] where x is not 0 (the zero case was already
  supported). This requires changes to
  ServiceWorkerLoaderHelpers::ReadBlobResponseBody and
  mojom.Blob.ReadRange().
- Sets ResourceResponseInfo.content_length based on the HTTP header
  Content-Length for service worker loaders. This allows Chromium's
  media code to allow seeking over the media source.

Bug:  892227 
Change-Id: I9e6f6faaae7b83ad3aee9c0f217834d62c214b85
Reviewed-on: https://chromium-review.googlesource.com/c/1263739
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597057}
[modify] https://crrev.com/56dd83c9005f68d55ddbf85c551759526b4e37cd/content/browser/service_worker/service_worker_navigation_loader_unittest.cc
[modify] https://crrev.com/56dd83c9005f68d55ddbf85c551759526b4e37cd/content/common/service_worker/service_worker_loader_helpers.cc
[modify] https://crrev.com/56dd83c9005f68d55ddbf85c551759526b4e37cd/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc
[modify] https://crrev.com/56dd83c9005f68d55ddbf85c551759526b4e37cd/storage/browser/blob/blob_impl.cc
[modify] https://crrev.com/56dd83c9005f68d55ddbf85c551759526b4e37cd/storage/browser/blob/blob_impl_unittest.cc
[modify] https://crrev.com/56dd83c9005f68d55ddbf85c551759526b4e37cd/third_party/blink/public/mojom/blob/blob.mojom

Thanks for jumping on this so quickly!

I wanted to merge this to 70 to fix the bug which I agree shouldn't hit stable and therefore could block ServiceWorkerServicification or NetworkService launch.

But it seems we are still doing something wrong here.

The scenario is:
1. The page makes a range request
2. SW responds with 200 and a blob (from cache storage).
3. Our loading code (ServiceWorkerLoaderHelpers) is trying to respond to the page with 200 and send it only the requested bytes from that blob.

But it seems we should be either returning 200 with the entire blob, or 206 with the requested bytes. Since the SW responded with 200 + entire blob, we should probably just respond with that, i.e., the loading code shouldn't do any intermediate range handling. In fact the legacy non-S13N implementation appears to behave this way.

So it seems to me the right thing to do is just delete the special range-handling code in ServiceWorkerLoaderHelpers::ReadBlobResponseBody.

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

commit 7d1d4a4719a085b946a29f7dc0937d6894ccdc5c
Author: Matt Falkenhagen <falken@chromium.org>
Date: Mon Oct 08 18:04:33 2018

service worker: s13n: remove special range request handling

Consider the scenario:
1. Page makes a range request
2. Service worker responds with 200 status and a big blob.

Previously, S13nServiceWorker loading code returned to the page a 200
status and attempted to return only the requested bytes from the blob.
This was incorrect: if a 200 status response is returned, all the
bytes should be present. Furthermore, since the service worker itself
didn't return a 206 status response, it is probably not
correct for the loading code to attempt to convert it to one.

This CL changes the code to return exactly what the service worker
returned.

Bug:  892227 
Change-Id: Id032ba73ae0a61b300f2d45c1cbf2ba041481788
Reviewed-on: https://chromium-review.googlesource.com/c/1267057
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597608}
[modify] https://crrev.com/7d1d4a4719a085b946a29f7dc0937d6894ccdc5c/content/browser/service_worker/service_worker_navigation_loader.cc
[modify] https://crrev.com/7d1d4a4719a085b946a29f7dc0937d6894ccdc5c/content/common/service_worker/service_worker_loader_helpers.cc
[modify] https://crrev.com/7d1d4a4719a085b946a29f7dc0937d6894ccdc5c/content/common/service_worker/service_worker_loader_helpers.h
[modify] https://crrev.com/7d1d4a4719a085b946a29f7dc0937d6894ccdc5c/content/renderer/service_worker/service_worker_subresource_loader.cc
[modify] https://crrev.com/7d1d4a4719a085b946a29f7dc0937d6894ccdc5c/content/renderer/service_worker/service_worker_subresource_loader_unittest.cc

I'd like to merge c#16 and c#19 to M70 to avoid potentially blocking the ServiceWorkerServicification and/or NetworkService from experimenting in M70 Stable. The changes fix a regression that would happen when either of those features are on. The regression could prevent Chrome from playing and seeking media when served via service workers.

The changes have almost no effect when the features are disabled. The exception is c#16 makes a minor improvement to the internal Blob implementation (used whether those features are on or off) that should be safe.

This bug requires manual review: We are only 7 days from stable.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

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

Has this been tested in any local channels yet? I'm uncomfortable taking this change in, since it seems somewhat significant (code deletion/addition). 

Can you please add all impacted OS's also?

Understood. c#16 went through some canaries and looks fine. c#19 hasn't gone into canary yet.

With the exception of the blob_impl.cc change, the code changes are for the NetworkService/ServiceWorkerServicification-enabled path only. It also looks more than it is because c#16 changed code that c#19 removes outright. I'll prepare a merge patch for reference and to have ready in case of a merge.

Are you fully confident this won't introduce any new regressions and this change is fully safe for merge? We are less than 1 week away from M70 stable. 

I've reconsidered and I think this can wait for 71. We have been at 50% in Beta/Dev/Canary for a 2-3 months with good results, which hopefully means the impact of this bug in the wild isn't high. The change looks OK <https://chromium-review.googlesource.com/c/chromium/src/+/1270437> but could use more bake time for confidence it doesn't cause breakage of the feature. If it turns out to block the launch in 70 we can revisit merging.

I'll mark this as Fixed in 71 and we can remove the Merge labels.

As per c#28, removing the merge request.

For the fossil record,  issue 909996  shows this bug did impact one site in the wild on 70, but it wasn't noticed until SWS ramped up to 50% on Stable, despite being at 50% on Beta and 100% on Dev/Canary for several months.