Bug 184447 – mp4 video element broken with service worker

• Component: Service Workers
• Status: RESOLVED FIXED

[Ben Kelly]

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

[Radar WebKit Bug Importer]

<rdar://problem/39313155>

[youenn fablet]

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.

[Ben Kelly]

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.

[Jun Kokatsu]

(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

[youenn fablet]

> 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.

[Jun Kokatsu]

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?

[youenn fablet]

(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.

[youenn fablet]

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.

[youenn fablet]

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.

[Jun Kokatsu]

Yeah, no worries. I will make sure to not to change the content :)

[youenn fablet]

*** Bug 186050 has been marked as a duplicate of this bug. ***

[youenn fablet]

Created attachment 381676 [details]
Patch

[youenn fablet]

Created attachment 381678 [details]
Patch

[youenn fablet]

Ping review?

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

win error is unrelated.

[Chris Dumez]

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.

[youenn fablet]

> 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.

[WebKit Commit Bot]

Comment on attachment 381678 [details]
Patch

Clearing flags on attachment: 381678

Committed r251594: <https://trac.webkit.org/changeset/251594>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

This test broke internal tests.

[Chris Dumez]

Committed r251710: <https://trac.webkit.org/changeset/251710>

[Matt Lewis]

It also broke API tests. Please see related bugs.

[Matt Lewis]

This also broke layout Tests

[WebKit Commit Bot]

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

[youenn fablet]

Created attachment 382808 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 382808 [details]
Patch

Clearing flags on attachment: 382808

Committed r252047: <https://trac.webkit.org/changeset/252047>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.