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

Specify how media elements make and validate range requests#2814

Closed
wants to merge 1 commit into from

Conversation

jakearchibald
Copy link
Collaborator

@jakearchibald jakearchibald commented Jul 5, 2017

I'm looking for early feedback on this. I'm going to add some comments to highlight parts I'm not sure about.


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 21, 2021, 12:09 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

Parsing MDN data...
Parsing...
Generating HTML variant...
Error: missing <dfn> for topic "concept-request-type" explicitly from <span> element containing "type"; previous heading contents are "4.8.12.5 Loading the media resource"
Error count: 1
Saving index-html
Splitting...
Saving index.html
Saving introduction.html
Saving infrastructure.html
Saving common-microsyntaxes.html
Saving urls-and-fetching.html
Saving common-dom-interfaces.html
Saving structured-data.html
Saving dom.html
Saving semantics.html
Saving sections.html
Saving grouping-content.html
Saving text-level-semantics.html
Saving links.html
Saving edits.html
Saving embedded-content.html
Saving images.html
Saving iframe-embed-object.html
Saving media.html
Saving image-maps.html
Saving embedded-content-other.html
Saving tables.html
Saving forms.html
Saving input.html
Saving form-elements.html
Saving form-control-infrastructure.html
Saving interactive-elements.html
Saving scripting.html
Saving canvas.html
Saving custom-elements.html
Saving semantics-other.html
Saving microdata.html
Saving interaction.html
Saving dnd.html
Saving browsers.html
Saving window-object.html
Saving origin.html
Saving history.html
Saving browsing-the-web.html
Saving webappapis.html
Saving dynamic-markup-insertion.html
Saving timers-and-user-prompts.html
Saving system-state.html
Saving imagebitmap-and-animations.html
Saving comms.html
Saving server-sent-events.html
Saving web-sockets.html
Saving web-messaging.html
Saving workers.html
Saving worklets.html
Saving webstorage.html
Saving syntax.html
Saving parsing.html
Saving named-characters.html
Saving xhtml.html
Saving rendering.html
Saving obsolete.html
Saving iana.html
Saving indices.html
Saving references.html
Saving acknowledgements.html
Generating DEV variant...
Splitting...
Saving index.html
Saving introduction.html
Saving infrastructure.html
Saving common-microsyntaxes.html
Saving urls-and-fetching.html
Saving common-dom-interfaces.html
Saving dom.html
Saving semantics.html
Saving sections.html
Saving grouping-content.html
Saving text-level-semantics.html
Saving links.html
Saving edits.html
Saving embedded-content.html
Saving images.html
Saving iframe-embed-object.html
Saving media.html
Saving image-maps.html
Saving embedded-content-other.html
Saving tables.html
Saving forms.html
Saving input.html
Saving form-elements.html
Saving form-control-infrastructure.html
Saving interactive-elements.html
Saving scripting.html
Saving canvas.html
Saving custom-elements.html
Saving semantics-other.html
Saving microdata.html
Saving interaction.html
Saving dnd.html
Saving browsers.html
Saving window-object.html
Saving origin.html
Saving history.html
Saving browsing-the-web.html
Saving webappapis.html
Saving dynamic-markup-insertion.html
Saving timers-and-user-prompts.html
Saving system-state.html
Saving imagebitmap-and-animations.html
Saving comms.html
Saving server-sent-events.html
Saving web-sockets.html
Saving web-messaging.html
Saving workers.html
Saving worklets.html
Saving webstorage.html
Saving syntax.html
Saving named-characters.html
Saving xhtml.html
Saving obsolete.html
Saving indices.html
Saving references.html
Saving acknowledgements.html
Generating SNAP variant...
Error: missing <dfn> for topic "concept-request-type" explicitly from <span> element containing "type"; previous heading contents are "4.8.12.5 Loading the media resource"
Error count: 1
Saving index-snap



If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.


💥 Error: Wattsi server error 💥

PR Preview failed to build. (Last tried on Jan 12, 2022, 10:06 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

Parsing MDN data...
Parsing...
Generating HTML variant...
Error: missing <dfn> for topic "concept-request-type" explicitly from <span> element containing "type"; previous heading contents are "4.8.12.5 Loading the media resource"
Error count: 1
Saving index-html
Splitting...
Saving index.html
Saving introduction.html
Saving infrastructure.html
Saving common-microsyntaxes.html
Saving urls-and-fetching.html
Saving common-dom-interfaces.html
Saving structured-data.html
Saving dom.html
Saving semantics.html
Saving sections.html
Saving grouping-content.html
Saving text-level-semantics.html
Saving links.html
Saving edits.html
Saving embedded-content.html
Saving images.html
Saving iframe-embed-object.html
Saving media.html
Saving image-maps.html
Saving embedded-content-other.html
Saving tables.html
Saving forms.html
Saving input.html
Saving form-elements.html
Saving form-control-infrastructure.html
Saving interactive-elements.html
Saving scripting.html
Saving canvas.html
Saving custom-elements.html
Saving semantics-other.html
Saving microdata.html
Saving interaction.html
Saving dnd.html
Saving browsers.html
Saving window-object.html
Saving origin.html
Saving history.html
Saving browsing-the-web.html
Saving webappapis.html
Saving dynamic-markup-insertion.html
Saving timers-and-user-prompts.html
Saving system-state.html
Saving imagebitmap-and-animations.html
Saving comms.html
Saving server-sent-events.html
Saving web-sockets.html
Saving web-messaging.html
Saving workers.html
Saving worklets.html
Saving webstorage.html
Saving syntax.html
Saving parsing.html
Saving named-characters.html
Saving xhtml.html
Saving rendering.html
Saving obsolete.html
Saving iana.html
Saving indices.html
Saving references.html
Saving acknowledgements.html
Generating DEV variant...
Splitting...
Saving index.html
Saving introduction.html
Saving infrastructure.html
Saving common-microsyntaxes.html
Saving urls-and-fetching.html
Saving common-dom-interfaces.html
Saving dom.html
Saving semantics.html
Saving sections.html
Saving grouping-content.html
Saving text-level-semantics.html
Saving links.html
Saving edits.html
Saving embedded-content.html
Saving images.html
Saving iframe-embed-object.html
Saving media.html
Saving image-maps.html
Saving embedded-content-other.html
Saving tables.html
Saving forms.html
Saving input.html
Saving form-elements.html
Saving form-control-infrastructure.html
Saving interactive-elements.html
Saving scripting.html
Saving canvas.html
Saving custom-elements.html
Saving semantics-other.html
Saving microdata.html
Saving interaction.html
Saving dnd.html
Saving browsers.html
Saving window-object.html
Saving origin.html
Saving history.html
Saving browsing-the-web.html
Saving webappapis.html
Saving dynamic-markup-insertion.html
Saving timers-and-user-prompts.html
Saving system-state.html
Saving imagebitmap-and-animations.html
Saving comms.html
Saving server-sent-events.html
Saving web-sockets.html
Saving web-messaging.html
Saving workers.html
Saving worklets.html
Saving webstorage.html
Saving syntax.html
Saving named-characters.html
Saving xhtml.html
Saving obsolete.html
Saving indices.html
Saving references.html
Saving acknowledgements.html
Generating SNAP variant...
Error: missing <dfn> for topic "concept-request-type" explicitly from <span> element containing "type"; previous heading contents are "4.8.12.5 Loading the media resource"
Error count: 1
Saving index-snap



If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

source Outdated
<span>CORS-cross-origin</span> responses aren't mixed with responses from other URLs.</p>

<p>A <span>media resource</span> has a
<dfn data-x="concept-media-resource-uses-opaque-data-flag">uses opaque data flag</dfn>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #2813 - we may need to change the name of this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like "uses opaque response flag". We don't go more granular thus far than a Response for that kind of data and I doubt we'll ever will so it seems reasonable?

source Outdated
@@ -31982,6 +31992,18 @@ interface <dfn>HTMLMediaElement</dfn> : <span>HTMLElement</span> {

</p>

<p>A <span>media resource</span> has a
<dfn data-x="concept-media-resource-response-urls">list of response urls</dfn>, a
<span>list</span>, which is initially empty.</p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be an ordered set? It doesn't really matter in terms of behaviour, but an implementation using a set would use less memory.

Copy link
Member

@annevk annevk Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Say you have: URL -> URL2 -> URL where the first URL redirects but the last one doesn't due the a cookie. Would it matter if that ends up as (URL, URL2)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The order isn't important here either.

I want to ensure that all chunks of a particular media resource were requested from the same endpoint if opaque responses are involved. If the server redirects, that's fine. If all the responses are not-opaque, anything goes.

The attack I'm wanting to avoid is:

  1. Video src is https://personal.example.com, which contains personal data and is not a valid media element.
  2. Media element issues request for 0-.
  3. Service worker responds with 0-100 from a resource which is a known to be a video header, where the 101st byte represents the media length.
  4. Media element issues request for 101-.
  5. Service worker responds with fetch(event.request).

Since we've already given the media element a valid video header, the additional bytes from https://personal.example.com may leak as the video length etc etc.

However, this PR would see that the initial response url of null doesn't match https://personal.example.com, and return a network error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why would you store a list of URLs? Wouldn't you just stay opaque the moment you encounter it once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"opaque" isn't enough information. All the responses could be opaque but from different places. As a result, a load of non-video data could become valid video data and leak information.

I guess I could replace this with "initial response url list", which is the url list of the first response used, and a "mixed response sources flag".

Then, if the first entry of an additional response's url list doesn't match the first entry in the "initial response url list", set the "mixed response sources flag".

If the "uses opaque data flag" is set, or the response is opaque, and "mixed response sources flag" is set, return a network error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm breaking HLS by doing this. Will rethink my approach. I think "If opaque data is used, every response's (including ones previously used by this resource) first entry in the url list must match its request url" works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine now

source Outdated
data-x="concept-media-resource-uses-opaque-data-flag">uses opaque data flag</span> affects
whether subtitles referenced in the <span>media data</span> are exposed in the API and, for
<code>video</code> elements, whether a <code>canvas</code> gets tainted when the video is
drawn on it.</p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those last few lines are adapted from some text that already existed. I'm a little concerned about how hand-wavey they are, especially as the canvas spec seems to contradict it (#2813).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last few lines are allowed to be hand-wavey since they are statements of fact, not normative requirements. However they are not allowed to be wrong, so #2813 may indeed be a problem. I think it's OK for now though, pending getting #2813 figured out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the text here matches what browsers do. The canvas section is wrong.

source Outdated
data-x="concept-media-ranged-fetch-steps">ranged fetch steps</span> to
perform HTTP range retrieval requests for a given start and end range, or switching to a
streaming protocol. The user agent must consider a resource erroneous only if it has given
up trying to fetch it.</p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I'm worried about the hand-waving here, but I'm not sure how to improve it without defining how each codec works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does fetching behavior relate to the codec?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Video starts fetching.
  2. Container/codec decoding happens.
  3. Through knowledge of the codec, the browser knows the metadata sits in the last 1000 bytes of the resource.
  4. Fetch Range: 123456-

Or

  1. User seeks resource to 10 minutes in.
  2. Through knowledge of the codec, the browser knows the content at 10:00 sits 9000000 bytes in.
  3. Fetch Range: 9000000-.

Without defining "Through knowledge of the codec", I'm just providing the steps for making a range request, but never really defining when it's called.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to call out that codec contract a little bit. So it's more clear what we're not defining. That will also make it easier for folks to tell us we're making the wrong assumptions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

source Outdated
@@ -33015,6 +33053,115 @@ interface <dfn>HTMLMediaElement</dfn> : <span>HTMLElement</span> {
will end up firing a <code data-x="event-media-suspend">suspend</code> event, as described
earlier.</p>

<p>The <dfn data-x="concept-media-ranged-fetch-steps">ranged fetch steps</dfn> with a
<var>start</var> and optional <var>end</var> are the following steps:</p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've written this like an inner function that can access variables defined in the parent steps. Is this in any way ok?

Copy link
Member

@annevk annevk Jul 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a question for the Infra Standard, but I think generally we try to avoid that and pass parameters explicitly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I placed it within the resource fetch algorithm because that runs for the entire load of the media element. As in, it only reaches the final step if the whole media finishes loading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenic what do you think? I think personally I'd rater avoid nested functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the way in this this is referenced, it seems totally fine to me. It's barely a function; it's more annotating some steps by saying /* begin ranged fetch steps */ ... /* end range fetch steps */.

source Outdated
list</span>'s <span data-x="concept-header-list-allow-privileged-headers">allow privilaged
headers flag</span>.</p></li>

<li><p>Let <var>rangeValue</var> be `<code data-x="">bytes </code>`.</p></li>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm going to move these "creating a range header" steps into the fetch spec, to avoid these lines being duplicated in every spec that wants to make a range request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's at least two callers (or expected to be) that seems reasonable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be needed for downloads, and probably background fetch.

source Outdated
<li><p>Let <var>rangeValue</var> be `<code data-x="">bytes </code>`.</p></li>

<li><p><span data-x="UTF-8 encode">Encode</span> <var>start</var> and append the result to
<var>rangeValue</var></p></li>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't really sure how to build this header. Is "append" correct here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For building a byte sequence that seems fine. There's some debate still over whether they should maybe be immutable, but this will probably end up working.

source Outdated

<ol>
<li><p>If <var>firstResponseUrl</var> is null, and <var>url</var> is not null, return a
<span>network error</span>.</p></li>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, this is wrong. brb

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you remember why you said this was wrong? I am seeing the same in Chrome. Firefox is more lenient (though does require 2xx). Not sure exactly what Safari wants here, I cannot get 200 to work there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember why this is wrong. Safari doesn't like 200 here, but I think it should work (as described in the steps below).

There are other issues with this PR though, which I'll detail in a follow-up comment.

source Outdated
<span data-x="concept-request-header-list">header list</span>.</p></li>

<li><p>If <var>range</var>'s first-byte-pos doesn't equal <var>start</var>, return a
<span>network error</span>.</p></li>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I refer to first-byte-pos like this? It's from https://tools.ietf.org/html/rfc7233#section-4.2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to cross-reference that term. Given the tight integration with HTTP uplifting to Fetch makes more sense to me now, even if this is the only caller.

source Outdated
response even if it requested a range. It <!--non-normative-->may terminate fetches
that are (or become) redundant, e.g. if the returned range is already covered by a
previous or in-flight response, or the media is seeked as such that active fetches are
unlikely to be useful.</p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this too much detail for a note? It's difficult since the process of aborting fetches isn't really covered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it needs to not use should and may in a note. You can e.g. substitute "encouraged" and "could". Otherwise it does seem helpful.

@jakearchibald
Copy link
Collaborator Author

@foolip Could you take a look at this?

@foolip
Copy link
Member

foolip commented Jul 6, 2017

Going on vacation soon and won't have time to finish all things, like this. I'll check back early in August to see if this is still open.

source Outdated
associated:
<ul class="brief">
<li><dfn data-x="concept-header-list-allow-privileged-headers" data-x-href="https://fetch.spec.whatwg.org/#concept-header-list-allow-privileged-headers">allow privileged headers flag</dfn></li>
<li><dfn data-x="concept-header-list-append" data-x-href="https://fetch.spec.whatwg.org/#concept-header-list-append">append</dfn> method</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: s/method/algorithm, I think.

source Outdated
<span>CORS-cross-origin</span> responses aren't mixed with responses from other URLs.</p>

<p>A <span>media resource</span> has a
<dfn data-x="concept-media-resource-uses-opaque-data-flag">uses opaque data flag</dfn>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: <dfn goes on previous line. The Great Re-Wrapper can help by turning unwrapped lines into wrapped ones.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of that tool, cheers!

(fwiw I'd rather we just relied on editors for this kind of wrapping. Breaking mid-line makes it hard to search for terms across the document)

source Outdated
@@ -31982,6 +31992,18 @@ interface <dfn>HTMLMediaElement</dfn> : <span>HTMLElement</span> {

</p>

<p>A <span>media resource</span> has a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think media resource is the right place to store these things? So far in the spec it seems less like a spec data structure with fields and more like a concept. Maybe putting them on the media element might be more appropriate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use media resource as it changes when the src changes. If that happens, we want the properties to reset.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

source Outdated
<span>CORS-cross-origin</span> responses aren't mixed with responses from other URLs.</p>

<p>A <span>media resource</span> has a
<dfn data-x="concept-media-resource-uses-opaque-data-flag">uses opaque data flag</dfn>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're trying to move away from flags and to booleans, maybe this should be a boolean? OK either way though; maybe we should only do an en-masse conversion instead of mixing up both into the spec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware of the flag->boolean change. Happy to change it here.

source Outdated
@@ -32933,11 +32955,25 @@ interface <dfn>HTMLMediaElement</dfn> : <span>HTMLElement</span> {

<!--FETCH--><p><span data-x="concept-fetch">Fetch</span> <var>request</var>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the relation between this request and the one possibly created by the range request steps? It seems like we're normatively requiring this one, and then if the browser wants to do a range request, it needs to do a second request. Is that correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I intended. It feels like browsers should make a non-range request first to get the start of the resource, but also to determine if ranges are accepted.

source Outdated
<p>If <var>responseUrlList</var>[0] <span data-x="list contains">exists</span>, <span
data-x="list append">append</span> <var>responseUrlList</var>[0] to the <var>current media
resource</var>'s <span data-x="concept-media-resource-response-urls">list of response
urls</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit surprising we append the first, not the last (i.e. not the response's url). A note explaining that we are intentionally doing this for the purpose of tracking redirects would be good, IMO.

source Outdated
data-x="concept-media-resource-uses-opaque-data-flag">uses opaque data flag</span> affects
whether subtitles referenced in the <span>media data</span> are exposed in the API and, for
<code>video</code> elements, whether a <code>canvas</code> gets tainted when the video is
drawn on it.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last few lines are allowed to be hand-wavey since they are statements of fact, not normative requirements. However they are not allowed to be wrong, so #2813 may indeed be a problem. I think it's OK for now though, pending getting #2813 figured out?

source Outdated
data-x="concept-media-ranged-fetch-steps">ranged fetch steps</span> to
perform HTTP range retrieval requests for a given start and end range, or switching to a
streaming protocol. The user agent must consider a resource erroneous only if it has given
up trying to fetch it.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a separate worry about the hand-waving here, which is that it gives the user agent license to do literally anything. For example a user agent could issue range requests but not use the ranged fetch steps. Maybe the intent is clear enough though that we don't need to change it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about the "streaming protocol" bit, in that it might not follow the rules in terms of mixing opaque & non-opaque responses.

source Outdated
<span>environment settings object</span> and
<span data-x="concept-request-type">type</span> to "<code data-x="">audio</code>" if the
<span>media element</span> is an <code>audio</code> element and to "<code
data-x="">video</code>" otherwise.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be clearer as two steps IMO.

source Outdated
response even if it requested a range. It <!--non-normative-->may terminate fetches
that are (or become) redundant, e.g. if the returned range is already covered by a
previous or in-flight response, or the media is seeked as such that active fetches are
unlikely to be useful.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it needs to not use should and may in a note. You can e.g. substitute "encouraged" and "could". Otherwise it does seem helpful.

@jakearchibald
Copy link
Collaborator Author

I'm going to move on to range requests & downloads, but in the mean time… any ideas how I could write tests?

I think all browsers will issue ranges requests for a simple wav file that's seeked, which I can automate in browsers that support autoplay. However, the spec doesn't require browsers to make range requests, so it feels like a bad approach. Is there any kind of "unable to test" warning I could create in this situation.

Or, should I make it a manual test, where you have to seek a media element to generate range requests?

source Outdated
data-x="concept-request-header-list">header list</span>.</p></li>

<li><p>If <var>range</var>'s <a
href="https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.16">first-byte-pos</a>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annevk Is there a better way to link to this? The HTTP spec doesn't allow linking to the term directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it should be abstracted out into a <span> with the appropriate <dfn data-x-href=""> in the dependencies section, for one, but that's just editorial...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, other than that I was thinking that maybe this should be in Fetch given the tight integration with HTTP?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that mean duplicating HTTP's definition?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd still reference HTTP directly in Fetch as you did here, but for everyone else in the platform we'd have it abstracted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's be nice to have a header list algorithm that took a header name, and returned the parsed parts as defined by HTTP. It might be nice to expose this on the headers object too.

Eg: Let parsedRange be the result of parsing a header given headerList and Range.

Where parsedRange would be a structure like:

[
  {
    "ranges-specifier": "bytes=0-",
    "byte-ranges-specifier": "bytes=0-",
    "bytes-unit": "bytes",
    "byte-range-set": "0-",
    "suffix-byte-range-spec": null,
    "byte-range-spec": "0-",
    "first-byte-pos": "0",
    "last-byte-pos": null
  }
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great, but that would only work for headers the browser supports (and might be tricky given where that code lives).

@domenic
Copy link
Member

domenic commented Jul 11, 2017

I think all browsers will issue ranges requests for a simple wav file that's seeked, which I can automate in browsers that support autoplay. However, the spec doesn't require browsers to make range requests, so it feels like a bad approach. Is there any kind of "unable to test" warning I could create in this situation.

In general it's OK to write tests that aren't fully required by the spec. This is especially true if all browsers pass them. (Or intend to pass them, e.g. all use range requests.)

@jakearchibald jakearchibald changed the title WIP: Specify how media elements make and validate range requests Specify how media elements make and validate range requests Jul 12, 2017
@jakearchibald
Copy link
Collaborator Author

jakearchibald commented Jul 12, 2017

No longer a WIP as such, but I need to fix the first-byte-pos reference.

Will pick this up again soon.

@jakearchibald
Copy link
Collaborator Author

There are security issues with this PR, especially the way it checks only the first response URL.

The final response URL is the important one, although there might need to be further restrictions if the redirect goes cross-origin then comes back to the same final url, unless we want that to be observable.

whatwg/fetch#144 (comment) is a better summary of the issues and solutions.

Background fetch has some algorithms for validating partial responses, which might be useful here, although those are CORS requests https://wicg.github.io/background-fetch/#validate-partial-response-algorithm, the validation for opaque responses will need to be stricter.

I'm happy for this PR to close. It's probably easier to start again.

@noamr
Copy link
Contributor

noamr commented Feb 22, 2022

@jakearchibald are you still on this? If not I can take over from where you left off.

@jakearchibald
Copy link
Collaborator Author

Please do! Yeah I don't have time for this in the near future

@jakearchibald
Copy link
Collaborator Author

The details in #2814 (comment) are probably more useful than the content of the PR

@noamr
Copy link
Contributor

noamr commented Feb 22, 2022

The details in #2814 (comment) are probably more useful than the content of the PR

Yea I went through that of course and using it as a guide.

@noamr
Copy link
Contributor

noamr commented Feb 23, 2022

New PR: #7655

@noamr noamr mentioned this pull request Feb 23, 2022
3 tasks
noamr added a commit to web-platform-tests/wpt that referenced this pull request Mar 24, 2022
See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.
noamr added a commit to web-platform-tests/wpt that referenced this pull request Mar 24, 2022
See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.
noamr added a commit to web-platform-tests/wpt that referenced this pull request Mar 29, 2022
* Add two cases to preload SRI

* Add test for stronger/weaker digest

* Fix expected results for video loading from multiple origins

See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.

* Add another case
domenic pushed a commit that referenced this pull request Mar 31, 2022
See #2814, especially #2814 (comment), for some background.

The strategy taken here is that multiple opaque range responses are OK as long as they're from the same origin. (They don't have to be the same URL.) Programmatic service worker responses are considered a null origin for these purposes.

This matches implemented behaviors.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 2, 2022
Automatic update from web-platform-tests
Add a few cases to preload SRI (#33326)

* Add two cases to preload SRI

* Add test for stronger/weaker digest

* Fix expected results for video loading from multiple origins

See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.

* Add another case
--

wpt-commits: 6a214e8155265b0c4e13471302d35f27386ef550
wpt-pr: 33326
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Apr 5, 2022
Automatic update from web-platform-tests
Add a few cases to preload SRI (#33326)

* Add two cases to preload SRI

* Add test for stronger/weaker digest

* Fix expected results for video loading from multiple origins

See whatwg/html#7655

When loading video from multiple opaque origins (by a middleman service-worker),
video loading should fail rather than be alllowed and taint the canvas.

That's because some of the video responses may contain metadata such as duration that
would leak to the subsequent requests.

See whatwg/html#2814 (comment) for further details.

This change makes the test case pass in all browsers.

* Add another case
--

wpt-commits: 6a214e8155265b0c4e13471302d35f27386ef550
wpt-pr: 33326
mfreed7 pushed a commit to mfreed7/html that referenced this pull request Jun 3, 2022
See whatwg#2814, especially whatwg#2814 (comment), for some background.

The strategy taken here is that multiple opaque range responses are OK as long as they're from the same origin. (They don't have to be the same URL.) Programmatic service worker responses are considered a null origin for these purposes.

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

Successfully merging this pull request may close these issues.

None yet

5 participants