Closed Bug 1119803 Opened 5 years ago Closed 5 years ago

Uninitialised value use in StopPrerollingVideo

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jseward, Assigned: jseward)

Details

Attachments

(2 files)

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?
Attached file Valgrind complainery
Attached patch The obvious fixSplinter Review
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.
Attachment #8547456 - Flags: feedback?(bobbyholley)
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.
Attachment #8547456 - Flags: review+
Attachment #8547456 - Flags: feedback?(bobbyholley)
Attachment #8547456 - Flags: feedback+
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.
Flags: needinfo?(giles)
(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?
yes.
Yes, we should definitely uplift this. I can nominate after it lands on m-c.
Flags: needinfo?(giles)
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
Attachment #8547456 - Flags: approval-mozilla-beta?
Attachment #8547456 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/dff5dc1b281a
Assignee: nobody → jseward
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8547456 - Flags: approval-mozilla-beta?
Attachment #8547456 - Flags: approval-mozilla-beta+
Attachment #8547456 - Flags: approval-mozilla-aurora?
Attachment #8547456 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.