1119803 - Uninitialised value use in StopPrerollingVideo

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Julian Seward [:jseward]]

test case = dom/media/test/test_playback.html

Looks like MediaDecoderStateMachine::mIsVideoPrerolling is used
without being initialised.  I had a bit of a look around but the
class is vast and I can't guess how the logic works.  I can say
that mIsVideoPrerolling -- and, for that matter, mIsAudioPrerolling --
are not initialised in MediaDecoderStateMachine::MediaDecoderStateMachine.
Maybe they should be?

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complainery

Comment 2

[Julian Seward [:jseward]]

Attachment: The obvious fix

This stops V complaining.  Does it seem the right approach?

Note that I did not also initialise mIsAudioPrerolling since
V didn't complain about that.  But not doing so rather bugs me
since it seems as if the logic for those two fields is the same
and so either both of them or neither of them should be
initialised in MediaDecoderStateMachine::MediaDecoderStateMachine.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8547456 [details] [diff] [review]
The obvious fix

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

Yeah please initialize both this and mIsVideoPrerolling to false. r=me with that.

Comment 4

[Bobby Holley (:bholley)]

This is basically a harmless bug I think - the worst impact would be one non-deterministic invocation of ScheduleStateMachine. But we should probably get it on the uplift radar anyway.

Comment 5

[Julian Seward [:jseward]]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #3)
> Yeah please initialize both this and mIsVideoPrerolling to false. r=me with

IIUC "this" already refers to mIsVideoPrerolling.  Do you mean mIsAudioPrerolling?

Comment 6

[Bobby Holley (:bholley)]

yes.

Comment 7

[Ralph Giles (:rillian)]

Yes, we should definitely uplift this. I can nominate after it lands on m-c.

Comment 8

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dff5dc1b281a

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8547456 [details] [diff] [review]
The obvious fix

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, hidden bugs from uninitialized variable use.
[Describe test coverage new/current, TBPL]: Landed on m-i.
[Risks and why]: Risk is minimal. Small and obvious change.
[String/UUID change made/needed]: None

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/dff5dc1b281a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/799189768df9
https://hg.mozilla.org/releases/mozilla-beta/rev/0a648dfd0459

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/799189768df9