1121841 - InvokeAndRetry can falsely report EOS

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

Consider the following situation in InvokeAndRetry:

(1) We invoke the demuxer, it does a read.
(2) The read fails, and we record that on the MP4Stream.
(3) InvokeAndRetry notices the failed read, and retries as a blocking read.
(4) InvokeAndRetry calls back into the demuxer, which errors out for some unrelated reason.
(5) InvokeAndRetry inspects the MP4Stream again, sees the same failed read, and concludes the read failed twice.

This doesn't affect correctness, since we'll throw an error in either case. But it's confusing, because the NS_WARNINGs will describe the incorrect error condition.

Fixing this is trivial. I think we should also at least uplift it to aurora, because it's a non-risk and helpful for debugging.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Clear the failed read after checking it. v1

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8549412 [details] [diff] [review]
Clear the failed read after checking it. v1

Review of attachment 8549412 [details] [diff] [review]:
-----------------------------------------------------------------

I've would have done so in LastReadFailed instead, so no need for public function

Comment 3

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Comment on attachment 8549412 [details] [diff] [review]
> Clear the failed read after checking it. v1
> 
> Review of attachment 8549412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've would have done so in LastReadFailed instead, so no need for public
> function

Yeah, but then the API has a non-obvious side-effect.

Comment 4

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3abade3e4eb4

This is basically no risk, so we might as well uplift it to make debugging easier.

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8549412 [details] [diff] [review]
Clear the failed read after checking it. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]:
[Describe test coverage new/current, TBPL]: presuming green on inbound.
[Risks and why]: Low. Affects mp4 playback but small and self-contained cleanup in error handling.
[String/UUID change made/needed]: None.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3abade3e4eb4

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/55a3f5fc46e1
https://hg.mozilla.org/releases/mozilla-beta/rev/0b7d9ce1cdc7

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/55a3f5fc46e1