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
<rdar://problem/39313155>
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.
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.
(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
> 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.
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?
(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.
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.
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.
Yeah, no worries. I will make sure to not to change the content :)
*** Bug 186050 has been marked as a duplicate of this bug. ***
Created attachment 381676 [details] Patch
Created attachment 381678 [details] Patch
Ping review?
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
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
win error is unrelated.
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.
> 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 on attachment 381678 [details] Patch Clearing flags on attachment: 381678 Committed r251594: <https://trac.webkit.org/changeset/251594>
All reviewed patches have been landed. Closing bug.
This test broke internal tests.
Committed r251710: <https://trac.webkit.org/changeset/251710>
It also broke API tests. Please see related bugs.
This also broke layout Tests
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
Created attachment 382808 [details] Patch
Comment on attachment 382808 [details] Patch Clearing flags on attachment: 382808 Committed r252047: <https://trac.webkit.org/changeset/252047>