Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP #27272

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

WIP #27272

wants to merge 4 commits into from

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 21, 2021

@jakearchibald this is the playground I used to inspect various behaviors in browsers. To test scenarios such as how browsers validate 206 responses, how they deal with cross-origin redirects after the initial 206 response, etc. I need some way to describe these scenarios and get long-wav.py to handle them. Hardcoding it all would work, but that does not seem exactly elegant. Was wondering if you had ideas.

Also with respect to what to observe on the media element side for success. Or would success mean that it either continues or doesn't continue making requests?

@github-actions github-actions bot temporarily deployed to wpt-preview-27272 January 21, 2021 12:00 Inactive
@jakearchibald jakearchibald self-requested a review January 22, 2021 10:33
@jakearchibald
Copy link
Contributor

jakearchibald commented Jan 22, 2021

I need some way to describe these scenarios and get long-wav.py to handle them. Hardcoding it all would work, but that does not seem exactly elegant. Was wondering if you had ideas.

You mentioned putting JSON in the URL, which seems like a good idea:

{
  "responses": [
    { "redirect": "same-origin" },
    { "range": [0, 400] },
    { "range": [400, 500] },
    { "redirect": "same-origin" },
  ],
}

…where "responses" details what each response in the sequence should be.

If putting this in the URL becomes an issue, it could be sent to another endpoint, which stashes it, and returns a lookup token. The lookup token would go on the URL.

Also with respect to what to observe on the media element side for success. Or would success mean that it either continues or doesn't continue making requests?

It might be easier to make the wav much shorter, and use progress events and media.buffered to ensure it gets the whole thing, or at worst, let it play through and pick up the end event.

I was surprised to hear that the error event wasn't working. I wonder if that's because any data after the wav header is valid, so browsers don't see it as a problem if it ends abruptly. Maybe a more complex format would help?

@annevk
Copy link
Member Author

annevk commented Jan 29, 2021

I found out that Chrome seems to use the suspend event if a later range request redirects. This even happens if everything is same-origin. Same-origin seems like a plus of sorts, as it means we can have the same logic for all no-cors requests, but suspend is really weird.

@github-actions github-actions bot temporarily deployed to wpt-preview-27272 January 29, 2021 17:21 Inactive
@annevk
Copy link
Member Author

annevk commented Jan 29, 2021

@jakearchibald @padenot @dalecurtis @youennf I would really appreciate it if you could take a look at this work in progress locally and tell me what else I should be poking at. Context here is whatwg/fetch#145 and annevk/orb#16, and eventually whatwg/html#2814. I would like us to find out how much we can restrict no-cors range requests from media elements.

@dalecurtis
Copy link
Contributor

Is there a specific question you're looking for an answer too? I'm not really familiar with the tool or what you want to do here.

@annevk
Copy link
Member Author

annevk commented Feb 2, 2021

@dalecurtis I would really like to know the exact details of Chrome's range request fetches when CORS is not involved. A specific question might be, why do I get the suspend event and no further fetches if I don't redirect the first range request, but do redirect the second range request (and only that request). In total Chrome will make 3 fetches in that (first request, second request, second request redirected location) and then fire suspend and stop doing anything, as far as I can tell. (To be clear, that seems almost ideal behavior, but I don't understand why it would make the 3rd request (i.e., why not bail directly upon seeing a redirect) and why does it not fire error.)

@dalecurtis
Copy link
Contributor

@github-actions github-actions bot temporarily deployed to wpt-preview-27272 February 5, 2021 13:45 Inactive
@padenot padenot self-requested a review February 17, 2021 11:08
@dalecurtis
Copy link
Contributor

Anne, are you sure your modifications to long-wav.py are working correctly? Running the wpt server and using the following curl command w/o any modifications downloads quite a bit of data:

curl -v -L -r 0- 'http://127.0.0.1:8001/fetch/range/resources/long-wav.py?chunked&source-key=6acb51e9-7a21-4b24-9f3f-7420f31c6891&scenario=redirect-second-request' --output file.bin

However after making your modifications it only downloads 2048 bytes -- which is the same the Chrome loader gets.

(Make sure to update the source-key between runs)

@annevk
Copy link
Member Author

annevk commented Apr 12, 2021

@dalecurtis that's intentional, to encourage the browser to keep making range requests for subsequent blocks of data. If the redirect isn't there you'll see that Chrome ends up doing a lot of range requests, equivalent to other browsers. The discovery is that adding the redirect to the mix seems to prevent subsequent requests somehow (if the redirect was done in response to the second range request), though the redirect is first followed.

@jakearchibald could you double check the Python for me just in case?

@dalecurtis
Copy link
Contributor

@annevk I don't think that's working as intended, otherwise you'd expect curl to keep downloading? It's always possible curl and chrome have the same bug, but seems unlikely given the different code bases.

@annevk
Copy link
Member Author

annevk commented Apr 12, 2021

Well, Firefox and Safari do keep fetching. Does curl support 206 responses and keep making new requests asking for more bytes?

@dalecurtis
Copy link
Contributor

-r 0- specifies the range request, -L means follow links. It's possible I've done something wrong though.

@annevk
Copy link
Member Author

annevk commented Apr 13, 2021

I tried this locally and curl will make a range request and get a partial response, but it won't then do another fetch to get the remainder of the bytes. Browsers will as part of their media element implementation. For instance, if you remove the scenario parameter curl will still only fetch 2048 bytes.

@dalecurtis
Copy link
Contributor

Thanks, sorry for the confusion on my part. I found the issue in Chrome. We were assigning content length to the pre-redirect data structure instead of the post-redirect one. Fix here: https://chromium-review.googlesource.com/c/chromium/src/+/2823523

Thanks for digging into this and revealing this issue in Chrome!

@annevk
Copy link
Member Author

annevk commented Apr 14, 2021

@dalecurtis well, as discussed above and over email, perhaps this should not be fixed and instead be enshrined? That we don't follow redirects for subsequent media element range fetches?

@dalecurtis
Copy link
Contributor

Happy to consider that after the fact, but the way this was broken probably has undesirable side effects -- even if they've gone unnoticed. The fix has landed on canary where I'll watch metrics to see the outcome, if all looks unchanged we can consider breaking this properly with an error instead of a hang.

@dalecurtis
Copy link
Contributor

At least on canary there doesn't appear to be any changes from that fix, will check back after beta/dev. Then we can make a call on disabling it. https://bugs.chromium.org/p/chromium/issues/detail?id=1184798#c4

@annevk
Copy link
Member Author

annevk commented Apr 21, 2021

Appreciate the updates @dalecurtis!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants