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

Include a "message" field in MediaError object #2085

Closed
wolenetz opened this issue Nov 23, 2016 · 34 comments
Closed

Include a "message" field in MediaError object #2085

wolenetz opened this issue Nov 23, 2016 · 34 comments
Labels
addition/proposal New features or enhancements

Comments

@wolenetz
Copy link

wolenetz commented Nov 23, 2016

In addition to the current "code" field, this issue requests adding a DOMString "message" field to the the MediaError object, which the user agent MAY populate with an informative error message.

Currently, the MediaError object only allows standardized exposure of a very small enumeration of error codes (https://www.w3.org/TR/html52/semantics-embedded-content.html#error-codes).

Web authors have requested greater detail of errors, even if they are exposed in vendor-specific messages. Already, Edge has a field msExtendedCode. Chrome is working to add a similar, though more verbose, vendor-prefixed string field to MediaError (https://bugs.chromium.org/p/chromium/issues/detail?id=601086).

Why is this needed? A common example is that MSE (Media Source Extensions) emits MEDIA_ERR_DECODE and MEDIA_ERR_SRC_NOT_SUPPORTED from a large variety of places in the spec, and differentiating the actual reason for the error is difficult at best. Services that deliver content via HTMLMediaElement (with or without MSE) and that encounter MediaError errors in the user agent frequently need more detail than just MediaError.code from the user agent to diagnose content, web app or user agent problems, especially when those errors are hard-to-reproduce.

@foolip, @jdsmith3000, @jyavenard - FYI

-- edit: s/jdsmith/jdsmith3000/

@domenic
Copy link
Member

domenic commented Nov 23, 2016

MediaError is such a strange design. Ah well.

This seems reasonable to me, but could you explain why it's not better to expand the list of media error codes? Is the concern backward compatibility with code that's already expecting only that limited set?

@wolenetz
Copy link
Author

A larger enumeration of codes would be insufficient to cover all the various edge cases which can trip-up web authors; it would also hinder standardization since vendors have distinct edge cases.

For example, if we decide in Chrome to give a decode error when the media container (e.g., mp4) indicates that a particular video frame is a keyframe, but inspection of the coded frame indicates otherwise, we might want to emit a very specific message for this precise scenario. Having a string field gives us that flexibility to respond quickly to newly discovered edge cases.

Alternatively, is there any precedent for a common registry of easily-extendable error codes that is independent of particular version of HTML5.x?

I do believe we need to retain the large-granularity MediaError.code field for compatibility.

@domenic
Copy link
Member

domenic commented Nov 23, 2016

Alternatively, is there any precedent for a common registry of easily-extendable error codes that is independent of particular version of HTML5.x?

In the past we have used the wiki for this, with moderate success. E.g. https://wiki.whatwg.org/wiki/MetaExtensions. However, I think it's not much of a burden for implementers to file an issue asking for an update to the in-spec table. I'd prefer that model; the wiki is meant more for situations where the relevant code is not necessarily shipping in browsers, which are meant to use the spec to coordinate.

@jyavenard
Copy link
Member

Thanks for lodging this change.

We at Mozilla strongly support this. In fact we've already implemented it and is available in Firefox from version 50 (however it's only enabled on our nightly and aurora channel).

So we have a "message" attribute present in the MediaError that will provide extensive description about what caused "code".

The content of message itself shouldn't be subjected to a defined behaviour I believe.
For Firefox it contains an extended code error, followed by a human readable and meaningful error message, followed by the location in the code that produced the error)

@wolenetz
Copy link
Author

I think it's not much of a burden for implementers to file an issue asking for an update to the in-spec table.

The burden of adding to the spec on every potential error log message is relatively high versus just emitting an error message verbatim in a string field. Further, these messages are likely to not have much intersection across vendors, due to media specs leaving much to quality-of-implementation.

@domenic
Copy link
Member

domenic commented Nov 23, 2016

I see. Free-form message sounds OK to me then. It sounds like this has multi-implementer support, so let me just write up a PR for it.

@domenic domenic added the addition/proposal New features or enhancements label Nov 23, 2016
@domenic
Copy link
Member

domenic commented Nov 23, 2016

Questions for you all that I encounter while speccing this:

  • How would you describe this field to web developers, briefly? So far I have "Returns a specific informative diagnostic message about the error condition encountered"
  • How would you describe this field in more detail to implementers? What, if any, normative requirements would you put on it? (Even vague non-testable ones are OK.)
  • The original message emphasized "MAY". Should this be nullable? null by default? Or empty string? Or should we always require this to include a message of some sort?

@wolenetz
Copy link
Author

wolenetz commented Nov 23, 2016

How would you describe this field to web developers, briefly? So far I have "Returns a specific informative diagnostic message about the error condition encountered"

SGTM. Might want to mention to web developers that the message is not standardized nor expected to be uniform across various user agent implementations.

How would you describe this field in more detail to implementers? What, if any, normative requirements would you put on it? (Even vague non-testable ones are OK.)

  • The message field should not have a setter exposed in IDL. It's read-only.
  • For the same MediaError object, the message should remain the same exact value. Forgive my relative and pedantic noob-ness here, what I'm getting at is the current message must === any previously read message from the same MediaError object. IOW, if the JS type for message is a String object, then it must remain the same String object across multiple reads of the message field from the same MediaError object.
  • It is recommended (but not required) that any time a new MediaError object is created, that the implementation also populate the message field if there is any further available detail in the implementation beyond just MediaError.code.
  • There might be some privacy concerns here, but I'm less familiar with what they might be. @domenic, can you help here?

The original message emphasized "MAY". Should this be nullable? null by default? Or empty string? Or should we always require this to include a message of some sort?

  • I'm not clear on pros/cons for either here. IIUC, they both would support the "feature detection" of the message field, right? (IOW, a web app could interrogate the runtime to see if there is a message attribute on the MediaError object, regardless of whether or not that attribute is nullable, right?)
  • See also my "It is recommended..." bullet in this response, above.

@domenic
Copy link
Member

domenic commented Nov 23, 2016

Thanks, those are really helpful!

There might be some privacy concerns here, but I'm less familiar with what they might be. domenic@, can you help here?

Hmm. Is it possible these error messages will reveal more details about the user than just their user agent? E.g., could they reveal the presence or absence of certain graphics card hardware, or driver versions, or similar? Should we disallow or discourage that?

I'm not clear on pros/cons for either here. IIUC, they both would support the "feature detection" of the message field, right? (IOW, a web app could interrogate the runtime to see if there is a message attribute on the MediaError object, regardless of whether or not that attribute is nullable, right?)

Yeah, there is no impact on feature detection. I guess the question is really whether we want to require this always to contain a useful string or not. Probably we should just recommend that it do so but not require, and in that case using null as the signifier for "no message available" makes sense to me.

@wolenetz
Copy link
Author

E.g., could they reveal the presence or absence of certain graphics card hardware, or driver versions, or similar? Should we disallow or discourage that?

I suppose (real) decode failure on a particular piece of hardware/driver combo, but lack of such failure on other combos is already leaking information. The problem is the balance between enabling web authors to fix broken media/apps while preserving privacy. Do we have precedence here?

@wolenetz
Copy link
Author

Probably we should just recommend that it do so but not require, and in that case using null as the signifier for "no message available" makes sense to me.

SGTM

@jyavenard
Copy link
Member

Here is Mozilla definition:
https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MediaError.webidl

It's a plain read-only, constant DOM string.

We have tests, to check on the message feature, which is simply to check if the attribute exists or not.
We do like so:
https://dxr.mozilla.org/mozilla-central/source/dom/media/test/test_decode_error.html#34

If there's no message, the string is simply empty, just like code attribute is always set to a value.
The error attribute on the HTML media element is the one that is nullable, it either exists or it doesn't.
Making the message attribute itself nullable sounds redundant to me, and unnecessary complication.

Hmm. Is it possible these error messages will reveal more details about the user than just their user agent? E.g., could they reveal the presence or absence of certain graphics card hardware, or driver versions, or similar? Should we disallow or discourage that?

From Mozilla approach, our changes went through our privacy and security team. They do not reveal anything about the user details, nor user agent.

The file name that could be indicated in the message, is generic to mozilla build bot and do not include the full path.

It is of course possible in the future that should we encountered a decoding failure due to a particular video card, that this information is showing in the message attribute.
But that can only be beneficial to the end user, as it would allow the service provider to properly explain on why things didn't play as expected.

@domenic
Copy link
Member

domenic commented Nov 23, 2016

I think null is going to be more useful to developers than the empty string.

I'm also considering the following provision:

This message string must not contain only the information already
available via the supplied error code; for example, it must not simply be a translation of the
code into a string format. If no additional information is available beyond that provided by the
error code, the message must be set to null.

What do you think of that?

@wolenetz
Copy link
Author

@ #2085 (comment), SGTM modulo: null vs "": I don't favor one over the other.

@domenic
Copy link
Member

domenic commented Nov 23, 2016

I've put up a preliminary pull request at #2086, but I realize this is all kind of fast so I want to give this discussion a few days to settle and make sure we have participation from all relevant parties, and that we're all agreed on issues like null vs. empty string or whether we need to include any privacy guidance. We can continue discussing here; no need to move the discussion to the PR.

@jyavenard
Copy link
Member

Thanks for doing this.

We can make message a nullable attribute if needs be. I personally don't see much of a difference between testing if an object is null or if it's empty. From the JS perspective it's really equivalent

@bzbarsky
Copy link
Contributor

For what it's worth, making this nullable seems like a slightly more annoying API for authors to work with. Instead of just being able to work with the string (and check whether it's "" when they want to detect the "no useful message" case specifically), they now have to check it for null before doing any string operations on it, on the off chance that it's null.

Worse yet, often enough they will forget, and only test in a UA that does have a message available, and then their code will end up throwing exceptions in another UA that uses a different media pippeline that does not provide a useful message in that situation. Then that other UA will switch to returning "" instead of null to avoid the web compat problem, and the resulting equilibrium is that no one ever returns null anyway... Might as well just spec it that way to start with. ;)

@zcorpan
Copy link
Member

zcorpan commented Nov 24, 2016

When the error is because of a network error, the message should be the empty string/null. Exposing detailed error information about network errors to JS is generally a security problem. c.f. https://html.spec.whatwg.org/#feedback-from-the-protocol:concept-websocket-closed

@jyavenard
Copy link
Member

When the error is because of a network error, the message should be the empty string/null. Exposing detailed error information about network errors to JS is generally a security problem. c.f. https://html.spec.whatwg.org/#feedback-from-the-protocol:concept-websocket-closed

That doesn't have to be. MSE allows to throw a network error at the element (via endOfStream).

@zcorpan
Copy link
Member

zcorpan commented Nov 24, 2016

MSE itself doesn't have access to detailed network error information, right? Or do you mean that MSE should be able to populate the message via a new argument to endOfStream or so? That case seems fine. That's like a clean WebSocket close where the server gives a reason in the close frame, which is exposed.

@annevk
Copy link
Member

annevk commented Nov 24, 2016

I agree that we should just use the empty string and not allow null as a value. That just results in type errors for no good reason and goes against patterns we've established elsewhere. (E.g., request.referrer returning the empty string when there's no referrer.)

@wolenetz
Copy link
Author

wolenetz commented Nov 29, 2016

MSE endOfStream() API doesn't currently have a message parameter. It only has an optional error parameter, which if specified, can be only "network" or "decode". In any case of endOfStream(), the application knows why it called it, so doesn't need to round-trip through the API to MediaError some app-specific reason for ending the stream. If such round-tripping is necessary, it would necessarily be dependent on having this message MediaError attribute added, as well as someone requesting some message parameter be added to the MSE endOfStream() API + getting it incubated.

From feedback, it seems best that MediaError message be a non-nullable string, which is empty unless there's more information about the error that the user agent wishes to provide to the app.

-edited to clarify 'some message parameter be' instead of just 'it'
-edited to clarify the last paragraph is about MediaError's message.

@domenic
Copy link
Member

domenic commented Nov 29, 2016

I still think null is a better representation of there being no message than the empty string, but I don't care strongly, so I can switch it given everyone seems to be in favor of the empty string instead.

@domenic
Copy link
Member

domenic commented Nov 29, 2016

Per discussion, I've switched the pull request #2086 to the empty string instead of null. I've also removed "do not merge yet" since this has seen enough discussion. This still needs web platform tests though before it is ready to go. @jyavenard or @wolenetz would you be willing to write some tests for this? Is it testable without depending on UA-dependent behavior---i.e. is there a way to deterministically trigger a MediaError?

@foolip
Copy link
Member

foolip commented Nov 30, 2016

Trying to play a 404 resource will get you a MediaError instance, but you couldn't assume anything about the message I suppose, just assert_equals(typeof mediaElement.error.message, "string").

@domenic
Copy link
Member

domenic commented Nov 30, 2016

Yeah, that sounds perfect. If someone is willing to write some web platform tests for that that's all that's standing in the way of getting this merged to the spec.

@wolenetz
Copy link
Author

I've written w3c wpt before (MSE), but am curious - is there a distinct set of tests for whatwg?

@domenic
Copy link
Member

domenic commented Nov 30, 2016

Nope; as stated at the top of https://github.com/w3c/web-platform-tests, "Test Suites for Web Platform specifications—including WHATWG, W3C and others".

@wolenetz
Copy link
Author

Excellent. I'll put something together soon. I've also sent a request for feedback [1] on this to the w3c archive and a couple MSFT HTML and MSE w3c spec editors whom I've collaborated with previously, since they might want to provide some advance input, too, but I don't believe they participate in WHATWG.

[1] https://lists.w3.org/Archives/Public/www-archive/2016Nov/0004.html

@domenic
Copy link
Member

domenic commented Jan 13, 2017

@wolenetz any progress on the web platform tests for this? :)

@wolenetz
Copy link
Author

Coming next week. Holidays and ramp-up are now complete :)

@foolip
Copy link
Member

foolip commented Jan 27, 2017

Tests are looking good. Do you want to make a spec PR as well, @wolenetz?

@annevk
Copy link
Member

annevk commented Jan 27, 2017

We already have a PR at #2086, right? (Seems you found it already.)

domenic added a commit that referenced this issue Jan 27, 2017
MXEBot pushed a commit to mirror/chromium that referenced this issue Apr 22, 2017
This improves the level of information available for web authors to use
when debugging media playback errors.

* Adds support for the MediaError.message field [1] [2]

* Changes MediaLog::PipelineStatusToString() to return just a
  stringified version of the enum identifier, suitable for use as a
  UA-specific-code when included in MediaError.message.

* Changes MediaLog::GetErrorMessage() to produce messages in the form:
  [[Stringified pipeline error code, if any, without any colon][: The
  first MEDIA_ERROR_LOG_ENTRY from the log, if any, since that is most
  likely to contain the most precise error information]]
  The produced message does not contain any newlines.
  See the updated media_browsertest.cc for basic examples.

* When the message would otherwise be empty for various cases in
  HTMLMediaElement, and more meaningful information beyond the basic
  standard MediaError codes and HTMLMediaElement networkStates is
  available, reports basic additional information (prefixed by error
  code "MEDIA_ELEMENT_ERROR: ".) This should assist edge case
  differentiation. See the updated media_browsertest.cc for basic
  examples.

* Test coverage:
  * Adds basic layout test coverage test similar to that added in [3].
  * Adds basic content browser test coverage for a small variety of
    errors, since these messages' format and content are specific to
    Chrom*.

The format of the message string may change in future (for example, see
discussion in [4]). Also, the variety of error codes in the message
prior to the first ':' will likely change in future, and the descriptive
error message detail following the ':' will likely change to include
more cases or more consistent format in future.

[1] - whatwg/html#2085
[2] - whatwg/html#2086
[3] - web-platform-tests/wpt#4638
[4] - whatwg/html#2531
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk

BUG=601086, 710617

Review-Url: https://codereview.chromium.org/2660003003
Cr-Commit-Position: refs/heads/master@{#466359}
GnorTech pushed a commit to nwjs/chromium.src that referenced this issue Apr 26, 2017
This improves the level of information available for web authors to use
when debugging media playback errors.

* Adds support for the MediaError.message field [1] [2]

* Changes MediaLog::PipelineStatusToString() to return just a
  stringified version of the enum identifier, suitable for use as a
  UA-specific-code when included in MediaError.message.

* Changes MediaLog::GetErrorMessage() to produce messages in the form:
  [[Stringified pipeline error code, if any, without any colon][: The
  first MEDIA_ERROR_LOG_ENTRY from the log, if any, since that is most
  likely to contain the most precise error information]]
  The produced message does not contain any newlines.
  See the updated media_browsertest.cc for basic examples.

* When the message would otherwise be empty for various cases in
  HTMLMediaElement, and more meaningful information beyond the basic
  standard MediaError codes and HTMLMediaElement networkStates is
  available, reports basic additional information (prefixed by error
  code "MEDIA_ELEMENT_ERROR: ".) This should assist edge case
  differentiation. See the updated media_browsertest.cc for basic
  examples.

* Test coverage:
  * Adds basic layout test coverage test similar to that added in [3].
  * Adds basic content browser test coverage for a small variety of
    errors, since these messages' format and content are specific to
    Chrom*.

The format of the message string may change in future (for example, see
discussion in [4]). Also, the variety of error codes in the message
prior to the first ':' will likely change in future, and the descriptive
error message detail following the ':' will likely change to include
more cases or more consistent format in future.

[1] - whatwg/html#2085
[2] - whatwg/html#2086
[3] - web-platform-tests/wpt#4638
[4] - whatwg/html#2531
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk

BUG=601086, 710617

Review-Url: https://codereview.chromium.org/2660003003
Cr-Commit-Position: refs/heads/master@{#466359}
(cherry picked from commit ed8e709)

TBR=jochen@chromium.org,sandersd@chromium.org,mlamouri@chromium.org,mcasas@chromium.org,dalecurtis@chromium.org,foolip@chromium.org

Review-Url: https://codereview.chromium.org/2837133002 .
Cr-Commit-Position: refs/branch-heads/3071@{#173}
Cr-Branched-From: a106f0a-refs/heads/master@{#464641}
xqq pushed a commit to xqq/chromium-media that referenced this issue May 17, 2017
This improves the level of information available for web authors to use
when debugging media playback errors.

* Adds support for the MediaError.message field [1] [2]

* Changes MediaLog::PipelineStatusToString() to return just a
  stringified version of the enum identifier, suitable for use as a
  UA-specific-code when included in MediaError.message.

* Changes MediaLog::GetErrorMessage() to produce messages in the form:
  [[Stringified pipeline error code, if any, without any colon][: The
  first MEDIA_ERROR_LOG_ENTRY from the log, if any, since that is most
  likely to contain the most precise error information]]
  The produced message does not contain any newlines.
  See the updated media_browsertest.cc for basic examples.

* When the message would otherwise be empty for various cases in
  HTMLMediaElement, and more meaningful information beyond the basic
  standard MediaError codes and HTMLMediaElement networkStates is
  available, reports basic additional information (prefixed by error
  code "MEDIA_ELEMENT_ERROR: ".) This should assist edge case
  differentiation. See the updated media_browsertest.cc for basic
  examples.

* Test coverage:
  * Adds basic layout test coverage test similar to that added in [3].
  * Adds basic content browser test coverage for a small variety of
    errors, since these messages' format and content are specific to
    Chrom*.

The format of the message string may change in future (for example, see
discussion in [4]). Also, the variety of error codes in the message
prior to the first ':' will likely change in future, and the descriptive
error message detail following the ':' will likely change to include
more cases or more consistent format in future.

[1] - whatwg/html#2085
[2] - whatwg/html#2086
[3] - web-platform-tests/wpt#4638
[4] - whatwg/html#2531
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk

BUG=601086, 710617

Review-Url: https://codereview.chromium.org/2660003003
Cr-Original-Commit-Position: refs/heads/master@{#466359}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: ed8e7091b5b0a51a8c0781f95813a5929c1f8110
alice pushed a commit to alice/html that referenced this issue Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

No branches or pull requests

7 participants