1495904 - mediaelement.currentTime = 0 causes uncatchable (!) InvalidStateError when srcObject is a MediaStream.

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

STRs:
1. Open https://jsfiddle.net/jib1/dyj8k4mf/

Expected result:
  Spec [1] says this should throw InvalidStateError (in a way JS can catch).

Actual result:
  In 62: Call succeeds without error.
  In 63: Call succeeds, with an error in web console (thanks to bug 1486130 [3]):

    InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable

There's no way to catch this error! It's an unhandled promise rejection, even though no promises are used. Culprit [2]:

  void
  HTMLMediaElement::SetCurrentTime(double aCurrentTime, ErrorResult& aRv)
  {
    LOG(LogLevel::Debug,
        ("%p SetCurrentTime(%f) called by JS", this, aCurrentTime));
--> RefPtr<Promise> tobeDropped = Seek(aCurrentTime, SeekTarget::Accurate, aRv);
  }

A quick search for this pattern reveals element.fastSeek() suffers the same bug.


[1] "On any attempt to set this attribute, the User Agent must throw an InvalidStateError exception." -
    https://w3c.github.io/mediacapture-main/getusermedia.html#mediastreams-in-media-elements
[2] https://searchfox.org/mozilla-central/rev/6ddb5fb144993fb5de044e2e8d900d7643b98a4d/dom/html/HTMLMediaElement.cpp#2727

[3] 20:03.09 INFO: Last good revision: 7eb7606a2b12990e8ef5e470db27838d5f5af612
    20:03.09 INFO: First bad revision: 7ccc1c8a7abeefd54bc4cf9a5d0eef23681c924a
    20:03.09 INFO: Pushlog:
    https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7eb7606a2b12990e8ef5e470db27838d5f5af612&tochange=7ccc1c8a7abeefd54bc4cf9a5d0eef23681c924a

Comment 1

[Kris Maglione [:kmag]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #0)
> There's no way to catch this error! It's an unhandled promise rejection,
> even though no promises are used. Culprit [2]:

A Promise is used though. Or, rather, it's created but unused.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Right, I meant no promises are used in the JS example.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1495904 - Have HTMLMediaElement::Seek() return synchronous errors synchronously instead of causing uncatchable promise rejections in web console. Enforce existing fastSeek() and mediaElement.currentPosition = value behavior (which is to ignore...

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The test for this will be restored in bug 1493885.

Comment 5

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #0)
> STRs:
> 1. Open https://jsfiddle.net/jib1/dyj8k4mf/
> 
> Expected result:
>   Spec [1] says this should throw InvalidStateError (in a way JS can catch).

> [1] "On any attempt to set this attribute, the User Agent must throw an
> InvalidStateError exception." -
>    
> https://w3c.github.io/mediacapture-main/getusermedia.html#mediastreams-in-
> media-elements


This is completely monkey patched and the media element spec doesn't mention any throwing [4]. It doesn't mention any handling (such as throwing, raising errors, etc.) of when you can't seek. All I can see is [5] (which is done in parallel, so `seeking` observably flops to true and back):
> 8. (...) If there are no ranges given in the seekable attribute then set the seeking IDL attribute to false and abort these steps.

IMO we should fix this so we don't regress our previous behavior, but then follow up by filing spec bugs to avoid the monkey patching.


[4] https://www.w3.org/TR/html52/semantics-embedded-content.html#dom-htmlmediaelement-currenttime
[5] https://www.w3.org/TR/html52/semantics-embedded-content.html#seek

Comment 6

[Andreas Pehrson [:pehrsons]]

I meant to use these links:

[4] https://html.spec.whatwg.org/multipage/media.html#dom-media-currenttime
[5] https://html.spec.whatwg.org/multipage/media.html#dom-media-seek

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] from comment #5)
> IMO we should fix this so we don't regress our previous behavior, but then
> follow up by filing spec bugs to avoid the monkey patching.

Agree. Spec issue filed: https://github.com/w3c/mediacapture-main/issues/543

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Green try (test is in): bug 1493885 comment 17.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ba29ee081ca
Have HTMLMediaElement::Seek() return synchronous errors synchronously instead of causing uncatchable promise rejections in web console. Enforce existing fastSeek() and mediaElement.currentPosition = value behavior (which is to ignore... r=pehrsons

Comment 10

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/8ba29ee081ca

Comment 11

[Jean-Yves Avenard [:jya]]

About [1]: https://w3c.github.io/mediacapture-main/getusermedia.html#mediastreams-in-media-elements

This change appears invalid to me, for both the normal playback code, and with mediastream.

https://searchfox.org/mozilla-central/rev/29aea2a2a3bd0f5e25ce0b60a76053fb25ba5149/dom/html/HTMLMediaElement.cpp#2772:
Why test the value of aRv at the start?

Additionally, I see no reference as mentioned in the description of having to throw should a mediastream be set:

https://w3c.github.io/mediacapture-main/getusermedia.html#mediastreams-in-media-elements

"Any calls to the fastSeek method on a HTMLMediaElement must be ignored"

for setting currentTime:

"The value is the official playback position, in seconds. Any attempt to alter it MUST be ignored. "

While yes, an exception was displayed in the console, it wasn't one JS could intercept, as such, the previous behaviour was correct as far as JS was concerned.

Comment 12

[Andreas Pehrson [:pehrsons]]

(In reply to Jean-Yves Avenard [:jya] from comment #11)
> Additionally, I see no reference as mentioned in the description of having
> to throw should a mediastream be set:

See comment 7.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jean-Yves Avenard [:jya] from comment #11)
> https://searchfox.org/mozilla-central/rev/
> 29aea2a2a3bd0f5e25ce0b60a76053fb25ba5149/dom/html/HTMLMediaElement.cpp#2772:
> Why test the value of aRv at the start?

Yes, that was left over from the creation of the promise I removed, a mistake. We've since restored the creation of the promise there in bug 1497149, so we're good again: https://searchfox.org/mozilla-central/diff/32f97763a0304e222e7848381f63d981386d38ac/dom/html/HTMLMediaElement.cpp#2772

> While yes, an exception was displayed in the console, it wasn't one JS could
> intercept, as such, the previous behaviour was correct as far as JS was
> concerned.

Should be addressed by bug 1497149. It's a good point that if we were to consider uplifting this to 63, we would definitely need patches from bug 1497149 as well, but I think we just decided WONTFIX for 63, since the web console noise is mostly non-JS observable, hence noise.