Uninitialised value uses in FormatParser::FormatChunk::IsValid()

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jseward, Assigned: lchristie)

Tracking

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

This might be fallout from bug 1231793.

The test
toolkit/content/tests/widgets/test_audiocontrols_dimensions.html 
causes all 4 tested values in FormatParser::FormatChunk::IsValid()
to be undefined:

   FrameSize()
   SampleRate()
   Channels()
   mPos

STR:
Do an --enable-valgrind --disable-jemalloc build, and then:

  ./mach mochitest --keep-open=no -f plain --valgrind=vMOZHX \
     toolkit/content/tests/widgets/test_audiocontrols_dimensions.html
Posted file Valgrind complaints
Posted patch bug-1250497-fix.patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc9081adf938
Attachment #8722674 - Attachment is obsolete: true
Attachment #8722674 - Flags: review?(cpearce)
Attachment #8722699 - Flags: review?(cpearce)
Comment on attachment 8722699 [details] [diff] [review]
bug-1250497-fix.patch

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

::: dom/media/wave/WaveDemuxer.cpp
@@ +807,5 @@
>  void
>  FormatParser::FormatChunk::Reset()
>  {
>    mPos = 0;
> +  for (int i = 0, i < FMT_CHUNK_MIN_SIZE, i++) {

You can instead go:

memset(mRaw, 0, FMT_CHUNK_MIN_SIZE);

OR you may even be able to initialize it in your constructor like so:


RIFFParser::RIFFHeader::RIFFHeader()
  : mRaw(0)
  , mPos(0)
{
  Reset();
}

This also initializes mPos to 0, which is good practice, otherwise people like Julian will keep bugging you about your sloppy code. :P
Attachment #8722699 - Flags: review?(cpearce) → review-
Comment on attachment 8722796 [details] [diff] [review]
bug-1250497-fix.patch

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

::: dom/media/wave/WaveDemuxer.cpp
@@ +649,5 @@
>  
>  // RIFFParser::RIFFHeader
>  
>  RIFFParser::RIFFHeader::RIFFHeader()
> +  : mPos(0)

Seems a bit redundant to include both the mPos(0) and the mPos=0 from Reset in the constructor. Maybe just don't bother with the mPos(0) then. Ditto below.
Attachment #8722796 - Flags: review?(cpearce) → review+
Attachment #8722796 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to Louis Christie [:lchristie] from comment #7)
> bug-1250497-fix.patch

Looks good to me -- it fixes the complaints.
https://hg.mozilla.org/mozilla-central/rev/b18eacf1d5c9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.