1052378 - MediaDecoderStateMachine::mIsVideoDecoding is used uninitialised

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Julian Seward [:jseward]]

A valgrind run for content/html/content/test/test_video_wakelock.html
generates the complaints shown in the next comment.

After a bit of poking around, it appears to be the use of
mIsVideoDecoding in MediaDecoderStateMachine::DecodeVideo():

    if (mState == DECODER_STATE_DECODING &&
        mIsVideoDecoding &&  <-------------------------- HERE
        ((!mIsAudioPrerolling && mIsAudioDecoding &&
          GetDecodedAudioDuration() < mLowAudioThresholdUsecs * mPlaybackRate) ||
          (!mIsVideoPrerolling && IsVideoDecoding() &&
           // don't skip frame when |clock time| <= |mVideoFrameEndTime| for
           // we are still in the safe range without underrunning video frames
           GetClock() > mVideoFrameEndTime &&
          (static_cast<uint32_t>(VideoQueue().GetSize())
            < LOW_VIDEO_FRAMES * mPlaybackRate))) &&
        !HasLowUndecodedData())
    {
      skipToNextKeyFrame = true;
      DECODER_LOG(PR_LOG_DEBUG, "Skipping video decode to the next keyframe");
    }

According to a bit of quick greppery in the tree, I cannot find any
assignment at all to mIsVideoDecoding.  Nor, for that matter, to
mIsAudioDecoding.  Am I looking in the wrong place?

Comment 1

[Julian Seward [:jseward]]

Thread 43 Media Decode #2:

Conditional jump or move depends on uninitialised value(s)
   at 0x6BF9F7E: mozilla::MediaDecoderStateMachine::DecodeVideo() (content/media/MediaDecoderStateMachine.cpp:588)
   by 0x6BFDEA0: nsRunnableMethodImpl<void (mozilla::MediaDecoderStateMachine::*)(), void, true>::Run() (ff-O-linux64/content/media/../../dist/include/nsThreadUtils.h:391)
   by 0x6BEAE0C: mozilla::MediaTaskQueue::Runner::Run() (content/media/MediaTaskQueue.cpp:167)
   by 0x583B037: nsThreadPool::Run() (xpcom/threads/nsThreadPool.cpp:220)
   by 0x5838226: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:766)
   by 0x58577F5: NS_ProcessNextEvent(nsIThread*, bool) (xpcom/glue/nsThreadUtils.cpp:265)
   by 0x5AB5913: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) (ipc/glue/MessagePump.cpp:355)
   by 0x5A91759: MessageLoop::RunInternal() (ipc/chromium/src/base/message_loop.cc:229)
   by 0x5A91764: MessageLoop::RunHandler() (ipc/chromium/src/base/message_loop.cc:222)
   by 0x5A91A29: MessageLoop::Run() (ipc/chromium/src/base/message_loop.cc:196)
   by 0x583B78D: nsThread::ThreadFunc(void*) (xpcom/threads/nsThread.cpp:346)
   by 0x4D0F6F7: _pt_root (nsprpub/pr/src/pthreads/ptthread.c:212)
   by 0x349F407D13: start_thread (/usr/src/debug/glibc-2.15-a316c1f/nptl/pthread_create.c:309)
   by 0x349F0F168C: clone (/usr/src/debug////////glibc-2.15-a316c1f/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:115)

 Uninitialised value was created by a heap allocation
   at 0x4809064: malloc (/home/sewardj/VgTRUNK/mozhx/coregrind/m_replacemalloc/vg_replace_malloc.c:298)
   by 0x481486B: moz_xmalloc (memory/mozalloc/mozalloc.cpp:52)
   by 0x6C2C5FA: operator new (ff-O-linux64/content/media/ogg/../../../dist/include/mozilla/mozalloc.h:201)
   by 0x6C2C5FA: mozilla::OggDecoder::CreateStateMachine() (content/media/ogg/OggDecoder.cpp:15)
   by 0x6BF6F18: mozilla::MediaDecoder::Load(nsIStreamListener**, mozilla::MediaDecoder*) (content/media/MediaDecoder.cpp:543)
   by 0x6B77F89: mozilla::dom::HTMLMediaElement::FinishDecoderSetup(mozilla::MediaDecoder*, mozilla::MediaResource*, nsIStreamListener**, mozilla::MediaDecoder*) (content/html/content/src/HTMLMediaElement.cpp:2673)
   by 0x6B78267: mozilla::dom::HTMLMediaElement::InitializeDecoderForChannel(nsIChannel*, nsIStreamListener**) (content/html/content/src/HTMLMediaElement.cpp:2625)
   by 0x6B78530: mozilla::dom::HTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*, nsISupports*) (content/html/content/src/HTMLMediaElement.cpp:348)
   by 0x5991BC8: mozilla::net::nsHttpChannel::CallOnStartRequest() (netwerk/protocol/http/nsHttpChannel.cpp:920)
   by 0x59920E9: mozilla::net::nsHttpChannel::ContinueProcessNormal(tag_nsresult) (netwerk/protocol/http/nsHttpChannel.cpp:1512)
   by 0x59929AB: mozilla::net::nsHttpChannel::ProcessNormal() (netwerk/protocol/http/nsHttpChannel.cpp:1447)
   by 0x5995DAA: mozilla::net::nsHttpChannel::ProcessResponse() (netwerk/protocol/http/nsHttpChannel.cpp:1284)
   by 0x599624B: mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) (netwerk/protocol/http/nsHttpChannel.cpp:4939)
   by 0x58AE216: nsInputStreamPump::OnStateStart() (netwerk/base/src/nsInputStreamPump.cpp:521)
   by 0x58AFEBB: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (netwerk/base/src/nsInputStreamPump.cpp:433)
   by 0x582B979: nsInputStreamReadyEvent::Run() (xpcom/io/nsStreamUtils.cpp:88)
   by 0x5838226: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:766)

Comment 2

[Julian Seward [:jseward]]

I get the impression, from reading beyond that point, that the
effect would be to make a nonsense of the "skip to next keyframe?"
decisions.

Comment 3

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

Indeed! That mIsVideoDecoding was supposed to be a IsVideoDecoding() function call!

Comment 4

[Julian Seward [:jseward]]

I can hack up a patch if you want.  Should mIsAudioDecoding be fixed in
the same way, given that it is b0rked in the same way?

Comment 5

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

I'll make a patch, it's pretty easy.

Comment 6

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

https://tbpl.mozilla.org/?tree=Try&rev=29c3b513a3db

Comment 7

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

Attachment: Patch

This change should have been made in bug 979104.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dab1ec80e83

Thanks Julian!

Comment 9

[Julian Seward [:jseward]]

I verified that the patch stops V complaining.

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9dab1ec80e83