Bug 184447 - mp4 video element broken with service worker
Summary: mp4 video element broken with service worker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 186050 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-04-10 06:49 PDT by Ben Kelly
Modified: 2019-11-05 01:33 PST (History)
15 users (show)

See Also:


Attachments
Patch (24.40 KB, patch)
2019-10-23 07:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (25.47 KB, patch)
2019-10-23 08:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.99 MB, application/zip)
2019-10-24 06:55 PDT, Build Bot
no flags Details
Patch (25.54 KB, patch)
2019-11-04 23:15 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Kelly 2018-04-10 06:49:06 PDT
STR:

1. Open safari or STP.
2. Load with https://blog.wanderview.com (installs service worker)
3. Click the link to navigate to https://blog.wanderview.com/blog/2017/03/13/firefox-52-settimeout-changes/
4. Possibly close the tab and open it again to get the page loaded from offline Cache API
5. Observe that the <video> elements show a broken video frame.
6. Disable service workers from the developer menu
7. Open site again and observe that the <video> elements now render
Comment 1 Radar WebKit Bug Importer 2018-04-10 06:52:03 PDT
<rdar://problem/39313155>
Comment 2 youenn fablet 2018-04-10 13:42:21 PDT
Thanks for the report.
Here is some info that currently explains the behavior:

Video loading usually happens as follow in Mac/iOS:
- WebKit will do a 0-1 byte range request and expect a 206 with 2 bytes as a response
- Based on the response, it will do loading or not with additional range requests.

When going through the service worker, the service worker needs to handle range request properly. The sw.js script is not doing that so cache API will probably not provide the expected results.

When service worker intercepts video loads, we have the following behavior:
- MediaResourceLoader sends a request with various headers, include byte-range and several other headers
- Service Worker receives a fetch event
- self.fetch() is used on the request of the fetch event
At that point, the fetch event request has all headers and self.fetch will create a new request based on it.
Since the load is in no-cors mode, we use RequestNoCors header filtering mode, which then filters out all MediaResourceLoader specific headers.
We then do the load, get a 200 response and not a 206 due to the filtered out headers.
We send it to MediaResourceLoader which actually decides it will not play the video.

In the short term, the best approach would be to make the script not intercept cross-origin byte-range/video requests.
Or handle all range requests in scripts and not rely on fetch/servers to do it.

Even if we would somehow all fetch to use all headers (might be ok since the script did not touch any of them),
we would probably need to beef up the Cache API to cache range requests, or the script would need to do the handling by itself, probably using synthetic responses, which is basically back to let the script handle every range request.

Another approach would be to change the way we handle response checking in our media resource loading code path.
Comment 3 Ben Kelly 2018-04-10 14:01:42 PDT
Handling range requests in fetch and cache specs is an open issue:

https://github.com/whatwg/fetch/issues/144

Other browsers basically accept a 200 with full body for a range request in their media code right now.
Comment 4 Jun Kokatsu 2018-04-16 14:48:00 PDT
(In reply to youenn fablet from comment #2)
> Thanks for the report.
> Here is some info that currently explains the behavior:
> 
> Video loading usually happens as follow in Mac/iOS:
> - WebKit will do a 0-1 byte range request and expect a 206 with 2 bytes as a
> response
> - Based on the response, it will do loading or not with additional range
> requests.
> 
> When going through the service worker, the service worker needs to handle
> range request properly. The sw.js script is not doing that so cache API will
> probably not provide the expected results.
> 
> When service worker intercepts video loads, we have the following behavior:
> - MediaResourceLoader sends a request with various headers, include
> byte-range and several other headers
> - Service Worker receives a fetch event
> - self.fetch() is used on the request of the fetch event
> At that point, the fetch event request has all headers and self.fetch will
> create a new request based on it.
> Since the load is in no-cors mode, we use RequestNoCors header filtering
> mode, which then filters out all MediaResourceLoader specific headers.
> We then do the load, get a 200 response and not a 206 due to the filtered
> out headers.
> We send it to MediaResourceLoader which actually decides it will not play
> the video.
> 
> In the short term, the best approach would be to make the script not
> intercept cross-origin byte-range/video requests.
> Or handle all range requests in scripts and not rely on fetch/servers to do
> it.
> 
> Even if we would somehow all fetch to use all headers (might be ok since the
> script did not touch any of them),
> we would probably need to beef up the Cache API to cache range requests, or
> the script would need to do the handling by itself, probably using synthetic
> responses, which is basically back to let the script handle every range
> request.
> 
> Another approach would be to change the way we handle response checking in
> our media resource loading code path.

This doesn't work with Same-Origin audio/video file.

PoC
https://test.shhnjk.com/media_test.html
Comment 5 youenn fablet 2018-05-02 11:02:50 PDT
> This doesn't work with Same-Origin audio/video file.
> 
> PoC
> https://test.shhnjk.com/media_test.html

You might need to activate crossOrigin on the video element, otherwise no-cors will be used and headers expurged no matter whether the URL is cross origin or not.

Second thing is that your service worker should check whether the response is 206, as caching a 206 will be rejected and the fetch event might end up being errored.

Note that fetch event destination should now be working for audio and video loads so that service workers could use that to make sure to not intercept these loads.
Comment 6 Jun Kokatsu 2018-05-12 14:40:50 PDT
Adding crossorigin attribute didn't help. I can also confirm that Service worker is not responding with 206 response.
So the solution here is to not to intercept any audio/video resources in service worker?
Comment 7 youenn fablet 2018-05-13 14:56:25 PDT
(In reply to Jun Kokatsu from comment #6)
> Adding crossorigin attribute didn't help. I can also confirm that Service
> worker is not responding with 206 response.

Interesting, I'll try to reproduce the issue.

> So the solution here is to not to intercept any audio/video resources in
> service worker?

Request.destination in recent WebKit builds can be used for that purpose.
Comment 8 youenn fablet 2018-05-14 02:30:12 PDT
I had a look and it seems that adding 'Accept-Encoding : identity' header makes it work.
It is true that Accept-Encoding is currently filtered out when being exposed to the service worker as it is a forbidden request header name.

I am not sure though why the server is requiring this header to serve range requests.
Comment 9 youenn fablet 2018-05-29 14:49:44 PDT
Jun,

I filed https://github.com/whatwg/fetch/issues/747 for the 'Accept-Encoding: Identity' bug.
I do not know whether your website is stable but I made direct reference to it from GitHub. Let me know whether that is fine.
Comment 10 Jun Kokatsu 2018-05-29 18:41:34 PDT
Yeah, no worries. I will make sure to not to change the content :)
Comment 11 youenn fablet 2019-10-23 07:16:45 PDT
*** Bug 186050 has been marked as a duplicate of this bug. ***
Comment 12 youenn fablet 2019-10-23 07:44:00 PDT
Created attachment 381676 [details]
Patch
Comment 13 youenn fablet 2019-10-23 08:35:06 PDT
Created attachment 381678 [details]
Patch
Comment 14 youenn fablet 2019-10-24 05:50:18 PDT
Ping review?
Comment 15 Build Bot 2019-10-24 06:55:45 PDT
Comment on attachment 381678 [details]
Patch

Attachment 381678 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13173674

New failing tests:
fast/repaint/backgroundSizeRepaint.html
Comment 16 Build Bot 2019-10-24 06:55:48 PDT
Created attachment 381804 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 17 youenn fablet 2019-10-24 07:05:49 PDT
win error is unrelated.
Comment 18 Chris Dumez 2019-10-24 08:28:19 PDT
Comment on attachment 381678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381678&action=review

> Source/WebCore/ChangeLog:10
> +        In particular, remove thre range header from a Request/Headers object whenever modified.

thre ?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:972
> +            if (auto error = validateRangeRequestedFlag(request.resourceRequest(), resource->response()))

Why cannot we check if the resource->resourceRequest() has a range header or not? Why do we need a flag on the response?

> Source/WebCore/platform/network/ResourceResponseBase.h:245
> +    bool m_isRangeRequested { false };

I have concerns about adding yet another flag to ResourceResponse. Especially, I am not convinced it is actually required.
Comment 19 youenn fablet 2019-10-24 08:47:22 PDT
> I have concerns about adding yet another flag to ResourceResponse.
> Especially, I am not convinced it is actually required.

I initially thought we could do without.
I went with this approach to more closely match the spec.

We need to store whether a response was generated from a range request.
We do not always have the request associated to a response.
This is in particular the case when serving a response from service worker.

We could probably add a flag to FetchResponse.
The fetch spec has https://fetch.spec.whatwg.org/#concept-response-range-requested-flag which maps to ResourceResponse, not FetchResponse.
The validation check is done in https://fetch.spec.whatwg.org/#main-fetch, at which point we have no concept of a FetchResponse.
We would need to move that check to code implementing https://fetch.spec.whatwg.org/#http-network-or-cache-fetch.
That does not sound appealing.

Is the issue the size of the ResourceResponse?
If so, we can probably optimise this.
For instance Tainting is currently taking a byte, while it could take 2 bits only.
Comment 20 WebKit Commit Bot 2019-10-25 09:20:29 PDT
Comment on attachment 381678 [details]
Patch

Clearing flags on attachment: 381678

Committed r251594: <https://trac.webkit.org/changeset/251594>
Comment 21 WebKit Commit Bot 2019-10-25 09:20:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Matt Lewis 2019-10-29 09:59:49 PDT
This test broke internal tests.
Comment 23 Chris Dumez 2019-10-29 10:04:17 PDT
Committed r251710: <https://trac.webkit.org/changeset/251710>
Comment 24 Matt Lewis 2019-10-29 10:06:30 PDT
It also broke API tests. Please see related bugs.
Comment 25 Matt Lewis 2019-10-29 10:09:23 PDT
This also broke layout Tests
Comment 26 WebKit Commit Bot 2019-11-04 22:26:03 PST
Comment on attachment 381678 [details]
Patch

Rejecting attachment 381678 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 381678, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13213699
Comment 27 youenn fablet 2019-11-04 23:15:50 PST
Created attachment 382808 [details]
Patch
Comment 28 WebKit Commit Bot 2019-11-05 01:33:11 PST
Comment on attachment 382808 [details]
Patch

Clearing flags on attachment: 382808

Committed r252047: <https://trac.webkit.org/changeset/252047>
Comment 29 WebKit Commit Bot 2019-11-05 01:33:13 PST
All reviewed patches have been landed.  Closing bug.