Closed Bug 1495904 Opened Last year Closed Last year

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

Categories

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

63 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
geckoview62 --- unaffected
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: jib, Assigned: jib)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
(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.
Right, I meant no promises are used in the JS example.
The test for this will be restored in bug 1493885.
(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
(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
Attachment #9013896 - Attachment description: Bug 1495904 - Have HTMLMediaElement::Seek() return synchronous errors correctly, so that mediaElement.currentPosition = value throws InvalidStateError if srcObject is a MediaStream. r?pehrsons → 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...
Blocks: 1493885
See Also: 1493885
Green try (test is in): bug 1493885 comment 17.
Pushed by jbruaroey@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/8ba29ee081ca
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1497149
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.
(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.
(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.
See Also: → 1507193
You need to log in before you can comment on or make changes to this bug.