905070 - Uninitialised value use in mozilla::dom::HTMLMediaElement::MetadataLoaded

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Julian Seward [:jseward]]

I think this is new -- I didn't see it a couple of days ago.

TEST_PATH=content/media/test/test_chaining.html

Conditional jump or move depends on uninitialised value(s)
   at 0x5D32892: mozilla::dom::HTMLMediaElement::MetadataLoaded(int, int, bool, bool, nsDataHashtable<nsCStringHashKey, nsCString
> const*) (HTMLMediaElement.cpp:2793)
   by 0x5DA5E5D: mozilla::MediaDecoder::MetadataLoaded(int, int, bool, bool, nsDataHashtable<nsCStringHashKey, nsCString>*) (Medi
aDecoder.cpp:743)
   by 0x5DA9957: mozilla::AudioMetadataEventRunner::Run() (AbstractMediaDecoder.h:148)
   by 0x6A69A43: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:622)
   by 0x6A2F394: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:238)
   by 0x6501EA6: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:81)
   by 0x6A9D435: MessageLoop::RunInternal() (message_loop.cc:220)
   by 0x6A9D444: MessageLoop::RunHandler() (message_loop.cc:213)
   by 0x6A9D6BC: MessageLoop::Run() (message_loop.cc:187)
   by 0x6461837: nsBaseAppShell::Run() (nsBaseAppShell.cpp:163)
   by 0x62C01EE: nsAppStartup::Run() (nsAppStartup.cpp:269)
   by 0x568A403: XREMain::XRE_mainRun() (nsAppRunner.cpp:3855)
 Uninitialised value was created by a heap allocation
   at 0x4808904: malloc (vg_replace_malloc.c:291)
   by 0x481B108: moz_xmalloc (mozalloc.cpp:54)
   by 0x5DAD478: mozilla::MediaDecoderStateMachine::QueueMetadata(long, int, int, bool, bool, nsDataHashtable<nsCStringHashKey, n
sCString>*) (mozalloc.h:201)
   by 0x5DA48B1: mozilla::MediaDecoder::QueueMetadata(long, int, int, bool, bool, nsDataHashtable<nsCStringHashKey, nsCString>*) 
(MediaDecoder.cpp:705)
   by 0x60A169F: mozilla::OggReader::ReadOggChain() (OggReader.cpp:749)
   by 0x60A1815: mozilla::OggReader::DecodeAudioData() (OggReader.cpp:640)
   by 0x5DB0B30: mozilla::MediaDecoderStateMachine::DecodeLoop() (MediaDecoderStateMachine.cpp:933)
   by 0x5DB0E7E: mozilla::MediaDecoderStateMachine::DecodeThreadRun() (MediaDecoderStateMachine.cpp:510)
   by 0x56940BD: nsRunnableMethodImpl<void (nsPACMan::*)(), true>::Run() (nsThreadUtils.h:356)
   by 0x6A69A43: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:622)
   by 0x6A2F394: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:238)
   by 0x6A6AB98: nsThread::ThreadFunc(void*) (nsThread.cpp:250)


Conditional jump or move depends on uninitialised value(s)
   at 0x5D31D82: mozilla::dom::HTMLMediaElement::UpdateReadyStateForData(mozilla::MediaDecoderOwner::NextFrameStatus) (HTMLMediaE
lement.cpp:3013)
   by 0x5D31F9D: mozilla::dom::HTMLMediaElement::FirstFrameLoaded(bool) (HTMLMediaElement.cpp:2801)
   by 0x5DA5EE6: mozilla::MediaDecoder::MetadataLoaded(int, int, bool, bool, nsDataHashtable<nsCStringHashKey, nsCString>*) (Medi
aDecoder.cpp:760)
   by 0x5DA9957: mozilla::AudioMetadataEventRunner::Run() (AbstractMediaDecoder.h:148)
   by 0x6A69A43: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:622)
   by 0x6A2F394: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:238)
   by 0x6501EA6: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:81)
   by 0x6A9D435: MessageLoop::RunInternal() (message_loop.cc:220)
   by 0x6A9D444: MessageLoop::RunHandler() (message_loop.cc:213)
   by 0x6A9D6BC: MessageLoop::Run() (message_loop.cc:187)
   by 0x6461837: nsBaseAppShell::Run() (nsBaseAppShell.cpp:163)
   by 0x62C01EE: nsAppStartup::Run() (nsAppStartup.cpp:269)
 Uninitialised value was created by a heap allocation
   at 0x4808904: malloc (vg_replace_malloc.c:291)
   by 0x481B108: moz_xmalloc (mozalloc.cpp:54)
   by 0x5DAD478: mozilla::MediaDecoderStateMachine::QueueMetadata(long, int, int, bool, bool, nsDataHashtable<nsCStringHashKey, nsCString>*) (mozalloc.h:201)
   by 0x5DA48B1: mozilla::MediaDecoder::QueueMetadata(long, int, int, bool, bool, nsDataHashtable<nsCStringHashKey, nsCString>*) (MediaDecoder.cpp:705)
   by 0x60A169F: mozilla::OggReader::ReadOggChain() (OggReader.cpp:749)
   by 0x60A1815: mozilla::OggReader::DecodeAudioData() (OggReader.cpp:640)
   by 0x5DB0B30: mozilla::MediaDecoderStateMachine::DecodeLoop() (MediaDecoderStateMachine.cpp:933)
   by 0x5DB0E7E: mozilla::MediaDecoderStateMachine::DecodeThreadRun() (MediaDecoderStateMachine.cpp:510)
   by 0x56940BD: nsRunnableMethodImpl<void (nsPACMan::*)(), true>::Run() (nsThreadUtils.h:356)
   by 0x6A69A43: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:622)
   by 0x6A2F394: NS_ProcessNextEvent(nsIThread*, bool) (nsThreadUtils.cpp:238)
   by 0x6A6AB98: nsThread::ThreadFunc(void*) (nsThread.cpp:250)

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Julian Seward from comment #0)
> I think this is new -- I didn't see it a couple of days ago.
> 
> TEST_PATH=content/media/test/test_chaining.html
> 
> Conditional jump or move depends on uninitialised value(s)
>    at 0x5D32892: mozilla::dom::HTMLMediaElement::MetadataLoaded(int, int,
> bool, bool, nsDataHashtable<nsCStringHashKey, nsCString
> > const*) (HTMLMediaElement.cpp:2793)

This line is an if statement on a parameter of the function being called. Assuredly the parameter is initialized, are you sure this isn't a bug in valgrind?

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#2793



> Conditional jump or move depends on uninitialised value(s)
>    at 0x5D31D82:
> mozilla::dom::HTMLMediaElement::UpdateReadyStateForData(mozilla::
> MediaDecoderOwner::NextFrameStatus) (HTMLMediaE
> lement.cpp:3013)

The constructor initializes everything used on that line:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#1955

Comment 2

[cajbir (:cajbir)]

(In reply to Chris Pearce (:cpearce) from comment #1)
> 
> This line is an if statement on a parameter of the function being called.
> Assuredly the parameter is initialized, are you sure this isn't a bug in
> valgrind?

I assume valgrind is saying that the parameter it is initialized with is also uninitialized. We need to follow the chain back where that variable gets its value from. In this case it's OggReader::HasVideo:

  virtual bool HasVideo() {
    return mTheoraState != 0 && mTheoraState->mActive;
  }

Which I guess means mTheoraState and/or mActive is uninitiaized (BTW, why is 0 used and not nsnull?).

In the second case the constructor initializes everything on that line but is mHasVideo later set to a value that is uninitialized. MetadataLoaded for example sets it from a value passed as an argument which could have been not initialized somehow. Especially given it's also HasVideo() related in the first error.

Comment 3

[Julian Seward [:jseward]]

This looks a bit suspect to me.  It copies aHasAudio into metadata, but
ignores the supplied aHasVideo.  Red herring?

void MediaDecoderStateMachine::QueueMetadata(int64_t aPublishTime,
                                             int aChannels,
                                             int aRate,
                                             bool aHasAudio,
                                             bool aHasVideo,
                                             MetadataTags* aTags)
{
  NS_ASSERTION(OnDecodeThread(), "Should be on decode thread.");
  mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
  TimedMetadata* metadata = new TimedMetadata;
  metadata->mPublishTime = aPublishTime;
  metadata->mChannels = aChannels;
  metadata->mRate = aRate;
  metadata->mHasAudio = aHasAudio;
  metadata->mTags = aTags;
  mMetadataManager.QueueMetadata(metadata);
}

Comment 4

[cajbir (:cajbir)]

(In reply to Julian Seward from comment #3)
> This looks a bit suspect to me.  It copies aHasAudio into metadata, but
> ignores the supplied aHasVideo.  Red herring?

Yeah, that looks like the issue.

Comment 5

[cajbir (:cajbir)]

Attachment: Fix

Pass aHasVideo into the mHasVideo field in the metadata.

Comment 6

[cajbir (:cajbir)]

Try build:  https://tbpl.mozilla.org/?tree=Try&rev=23a754ec0bce

Comment 7

[Julian Seward [:jseward]]

(In reply to Chris Double (:doublec) from comment #5)
> Pass aHasVideo into the mHasVideo field in the metadata.

With this in place, the V/memcheck complaints disappear .. LGTM, therefore.

Comment 8

[cajbir (:cajbir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/106dba161a79

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/106dba161a79