1232326 - Uninitialised value use in AudioBufferInPlaceScale

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Julian Seward [:jseward]]

dom/media/webaudio/AudioNodeEngine.cpp: AudioBufferInPlaceScale() is
passed an undefined |aScale| value from
PeriodicWave::createBandLimitedTables().

This seems to be because PeriodicWave::PeriodicWave does not
initialise m_normalizationScale.  Setting it to 1.0f in the
constructor makes the Valgrind complaints go away.

On closer inspection, m_normalizationScale seems almost completely
unused.  It only has liveness within a single call to
PeriodicWave::createBandLimitedTables and so it would be equally
possible and maybe better, to remove it as a class field and replace
the single use with an auto variable in
PeriodicWave::createBandLimitedTables.

Comment 1

[Julian Seward [:jseward]]

(In reply to Julian Seward [:jseward] from comment #0)
> On closer inspection, m_normalizationScale seems almost completely
> unused.  It only has liveness within a single call to
> PeriodicWave::createBandLimitedTables [..]

Ah, no, I am wrong.  It could be that there are two calls to 
PeriodicWave::createBandLimitedTables on the same object.  The first
can set m_normalizationScale and the second might use it.

At least, removing m_normalizationScale and using an auto scalar
as suggested in comment 0, causes the test to fail.

Comment 2

[Julian Seward [:jseward]]

STR: 
DISPLAY=:1.0 MOZ_DISABLE_NONLOCAL_CONNECTIONS=0 \
  ./mach mochitest -f plain --valgrind=vMOZHX \
  "--valgrind-args=--fullpath-after=CURR/ --track-origins=no" \
  dom/media/webaudio/test/test_periodicWaveBandLimiting.html

Comment 3

[Julian Seward [:jseward]]

Attachment: Some valgrind complains (two examples of many)

Comment 4

[Julian Seward [:jseward]]

Attachment: bug1232326-1.diff

Sets m_normalizationScale to 1.0f in PeriodicWave::PeriodicWave.
I don't know if 1.0 is a suitable initial value, but it seems
plausible and it doesn't cause the test 
dom/media/webaudio/test/test_periodicWaveBandLimiting.html
to fail.

Comment 5

[Dan Minor [:dminor]]

Comment on attachment 8698056 [details] [diff] [review]
bug1232326-1.diff

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

This is fine. The value will be set to a proper value when we detect that it's necessary to rebuild the band limited tables in waveDataForFundamentalFrequency.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbcfa05140d

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1bbcfa05140d