Bug 174573 – Video errors should be instances of Error

• Component: Media
• Status: RESOLVED FIXED

[Andrew Morris]

jsbin: http://output.jsbin.com/jewolar

Output:
error instanceof Error: false

Expected output:
error instanceof Error: true

[Jon Lee]

It reports two different errors. On first load on Safari 11 on High Sierra the result is false, then true when I reload.

The error on load is "AbortError". Subsequently it is a "NotAllowedError".

Repro steps:
1. Set a break point on line 35 on the webpage source ahead of time.
2. Open a new tab.
3. Go to any website (google.com)
4. Open the web inspector.
5. Load the webpage and notice the AbortError.
6. Continue. Reload the page.
7. Notice the NotAllowedError.

[Radar WebKit Bug Importer]

<rdar://problem/34445494>

[Chris Dumez]

DOMException correctly inherits from EcmaScript's Error. So it could be that media code is not using DOMException in some places (maybe its own kind of exception)?

[Chris Dumez]

In particular, the following interface looks interesting:
interface MediaError;

Because it is an IDL interface and not an IDL exception, it likely does not inherit from Error.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> In particular, the following interface looks interesting:
> interface MediaError;
> 
> Because it is an IDL interface and not an IDL exception, it likely does not
> inherit from Error.

We match the spec though:
https://html.spec.whatwg.org/multipage/media.html#error-codes

So maybe it is not supposed to inherit Error?

[Chris Dumez]

Displaying more information about the error, we see that it is a:
DOMError {name: "AbortError", message: "The operation was aborted."}

not a DOMException, nor a MediaError.

[Chris Dumez]

DOMError does not exist in the specification anymore and we are supposed to use Error nowadays:
https://heycam.github.io/webidl/#es-Error

I think the right fix is to drop DOMError and use JS's Error directly instead.

[Chris Dumez]

Media code relies on DOMError here in HTMLMediaElement.h:
void rejectPendingPlayPromises(DOMError&);

[Chris Dumez]

Killing DOMError aside, the media code needs to be fixed to reject play promises with DOMException rather than DOMError. As per the spec, this is what we should do:
- https://html.spec.whatwg.org/multipage/media.html#reject-pending-play-promises
- https://html.spec.whatwg.org/multipage/media.html#loading-the-media-resource:reject-pending-play-promises-2
- https://html.spec.whatwg.org/multipage/media.html#loading-the-media-resource:reject-pending-play-promises-3
- https://html.spec.whatwg.org/multipage/media.html#playing-the-media-resource:reject-pending-play-promises-3
- https://html.spec.whatwg.org/multipage/media.html#playing-the-media-resource:reject-pending-play-promises

Eric / Jer, please take a look.

[Chris Dumez]

Created attachment 321266 [details]
WIP Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> Created attachment 321266 [details]
> WIP Patch

Patch should work but needs layout test coverage.

[Build Bot]

Comment on attachment 321266 [details]
WIP Patch

Attachment 321266 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4598896

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/paused_true_during_pause.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_play_noautoplay.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_pause_noautoplay.html
http/tests/security/video-cross-origin-caching.html

[Build Bot]

Created attachment 321270 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 321266 [details]
WIP Patch

Attachment 321266 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4598770

New failing tests:
http/tests/security/video-cross-origin-caching.html

[Build Bot]

Created attachment 321271 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Build Bot]

Comment on attachment 321266 [details]
WIP Patch

Attachment 321266 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4599114

New failing tests:
http/tests/security/video-cross-origin-caching.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/paused_true_during_pause.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_play_noautoplay.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_pause_noautoplay.html
fast/mediastream/MediaStream-MediaElement-setObject-null.html

[Build Bot]

Created attachment 321272 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 321266 [details]
WIP Patch

Attachment 321266 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4599109

New failing tests:
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/paused_true_during_pause.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_play_noautoplay.html
imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/event_pause_noautoplay.html
http/tests/security/video-cross-origin-caching.html

[Build Bot]

Created attachment 321275 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Chris Dumez]

Created attachment 321322 [details]
Patch

[youenn fablet]

Comment on attachment 321322 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321322&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:1045
> +        promise.rejectType<IDLInterface<DOMException>>(error);

If that is a pattern that will happen often, maybe we should be able to write it as promise.reject(error) or promise.rejectAsException(error)?

[Chris Dumez]

Created attachment 321327 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 321327 [details]
Patch

Clearing flags on attachment: 321327

Committed r222270: <http://trac.webkit.org/changeset/222270>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.