Bug 174573

Summary: Video errors should be instances of Error
Product: WebKit Reporter: Andrew Morris <andrew>
Component: MediaAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, eric.carlson, jer.noble, jonlee, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
URL: http://output.jsbin.com/jewolar
See Also: https://bugs.webkit.org/show_bug.cgi?id=177201
Bug Depends on:    
Bug Blocks: 177207    
Attachments:
Description Flags
WIP Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Patch
none
Patch none

Description Andrew Morris 2017-07-16 18:14:43 PDT
jsbin: http://output.jsbin.com/jewolar

Output:
error instanceof Error: false

Expected output:
error instanceof Error: true
Comment 1 Jon Lee 2017-07-31 11:32:02 PDT
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.
Comment 2 Radar WebKit Bug Importer 2017-09-14 15:53:32 PDT
<rdar://problem/34445494>
Comment 3 Chris Dumez 2017-09-19 10:18:53 PDT
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)?
Comment 4 Chris Dumez 2017-09-19 10:20:54 PDT
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.
Comment 5 Chris Dumez 2017-09-19 10:22:05 PDT
(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?
Comment 6 Chris Dumez 2017-09-19 10:24:20 PDT
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.
Comment 7 Chris Dumez 2017-09-19 10:27:25 PDT
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.
Comment 8 Chris Dumez 2017-09-19 10:29:37 PDT
Media code relies on DOMError here in HTMLMediaElement.h:
void rejectPendingPlayPromises(DOMError&);
Comment 10 Chris Dumez 2017-09-19 16:49:28 PDT
Created attachment 321266 [details]
WIP Patch
Comment 11 Chris Dumez 2017-09-19 16:49:55 PDT
(In reply to Chris Dumez from comment #10)
> Created attachment 321266 [details]
> WIP Patch

Patch should work but needs layout test coverage.
Comment 12 Build Bot 2017-09-19 17:57:26 PDT
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
Comment 13 Build Bot 2017-09-19 17:57:27 PDT
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
Comment 14 Build Bot 2017-09-19 18:09:18 PDT
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
Comment 15 Build Bot 2017-09-19 18:09:20 PDT
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
Comment 16 Build Bot 2017-09-19 18:36:37 PDT
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
Comment 17 Build Bot 2017-09-19 18:36:38 PDT
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
Comment 18 Build Bot 2017-09-19 18:43:37 PDT
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
Comment 19 Build Bot 2017-09-19 18:43:38 PDT
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
Comment 20 Chris Dumez 2017-09-20 09:09:34 PDT
Created attachment 321322 [details]
Patch
Comment 21 youenn fablet 2017-09-20 09:59:55 PDT
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)?
Comment 22 Chris Dumez 2017-09-20 10:08:23 PDT
Created attachment 321327 [details]
Patch
Comment 23 WebKit Commit Bot 2017-09-20 10:38:31 PDT
Comment on attachment 321327 [details]
Patch

Clearing flags on attachment: 321327

Committed r222270: <http://trac.webkit.org/changeset/222270>
Comment 24 WebKit Commit Bot 2017-09-20 10:38:33 PDT
All reviewed patches have been landed.  Closing bug.