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

RESOLVED FIXED in mozilla26

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jseward, Assigned: cajbir)

Tracking

Trunk
mozilla26
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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)
(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
(Assignee)

Comment 2

4 years ago
(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.
(Reporter)

Comment 3

4 years ago
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);
}
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Updated

4 years ago
Assignee: nobody → chris.double
(Assignee)

Comment 5

4 years ago
Created attachment 790703 [details] [diff] [review]
Fix

Pass aHasVideo into the mHasVideo field in the metadata.
Attachment #790703 - Flags: review?(kinetik)
(Assignee)

Comment 6

4 years ago
Try build:  https://tbpl.mozilla.org/?tree=Try&rev=23a754ec0bce
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
Attachment #790703 - Flags: review?(kinetik) → review+
(Reporter)

Comment 7

4 years ago
(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.
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/106dba161a79
https://hg.mozilla.org/mozilla-central/rev/106dba161a79
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.