1055653 - Most MediaResource::GetDownloadRate() implementions ignore their out-parameter

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Julian Seward [:jseward]]

path = content/media/mediasource/test/test_MediaSource.html

content/media/MediaResource.h
has class MediaResource, with this method

  // Get the estimated download rate in bytes per second (assuming no
  // pausing of the channel is requested by Gecko).
  // *aIsReliable is set to true if we think the estimate is useful.
  virtual double GetDownloadRate(bool* aIsReliable) = 0;

There are about a half dozen implementations of GetDownloadRate in
derived classes, various of which simply "return 0" and don't assign
anything to *aIsReliable.  This is ungood given that the pointed-to
values (MediaDecoder::Statistics::mDownloadRateReliable) are later
used in conditional branching.

Valgrind complaints are shown in the next comment.  The claimed origin
("non-virtual thunk to ..") is bizarre and I haven't a clue what it
means.  However, changing the suspect GetDownloadRate implementations
like this

  virtual double GetDownloadRate(bool* aIsReliable) {
    volatile bool bogus;
    *aReliable = bogus;
    return 0;
  }

and re-running V exposes the real culprit:

  Uninitialised value was created by a stack allocation
    at 0x6C45FD6: mozilla::MediaSourceResource::GetDownloadRate(bool*)
                  (content/media/mediasource/MediaSourceResource.h:35)

The forthcoming patch fixes all the bogus implementations, to be on 
the safe side.

Comment 1

[Julian Seward [:jseward]]

Conditional jump or move depends on uninitialised value(s)
   at 0x6B966AD: mozilla::dom::HTMLMediaElement::UpdateReadyStateForData(mozilla::MediaDecoderOwner::NextFrameStatus) (content/html/content/src/HTMLMediaElement.cpp:3175)
   by 0x6C05106: mozilla::MediaDecoder::UpdateReadyStateForData() (content/media/MediaDecoder.cpp:1063)
   by 0x6C20A4E: nsRunnableMethodImpl<void (mozilla::MediaDecoder::*)(), void, true>::Run() (ff-O-linux64/content/media/../../dist/include/nsThreadUtils.h:391)
   by 0x583D957: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:770)
   by 0x585D1AF: NS_ProcessNextEvent(nsIThread*, bool) (xpcom/glue/nsThreadUtils.cpp:265)
   by 0x5ABC3AB: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (ipc/glue/MessagePump.cpp:99)
   by 0x5A98209: MessageLoop::RunInternal() (ipc/chromium/src/base/message_loop.cc:229)
   by 0x5A98214: MessageLoop::RunHandler() (ipc/chromium/src/base/message_loop.cc:222)
   by 0x5A984D9: MessageLoop::Run() (ipc/chromium/src/base/message_loop.cc:196)
   by 0x6A19D6F: nsBaseAppShell::Run() (widget/xpwidgets/nsBaseAppShell.cpp:164)
   by 0x73D0D23: nsAppStartup::Run() (toolkit/components/startup/nsAppStartup.cpp:278)
   by 0x741108C: XREMain::XRE_mainRun() (toolkit/xre/nsAppRunner.cpp:4021)
   by 0x7411353: XREMain::XRE_main(int, char**, nsXREAppData const*) (toolkit/xre/nsAppRunner.cpp:4092)
   by 0x74115B2: XRE_main (toolkit/xre/nsAppRunner.cpp:4306)
   by 0x40379B: do_main(int, char**, nsIFile*) (browser/app/nsBrowserApp.cpp:282)
   by 0x4038E4: main (browser/app/nsBrowserApp.cpp:643)

 Uninitialised value was created by a stack allocation
   at 0x6B966D5: non-virtual thunk to mozilla::dom::HTMLMediaElement::UpdateReadyStateForData(mozilla::MediaDecoderOwner::NextFrameStatus) (content/html/content/src/HTMLMediaElement.cpp:3183)

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1055653-1.diff

This fixes the four offending GetDownloadRate implementations
(all that I could find) to set *aIsReliable to false.  This 
assumes that (1) aIsReliable is always non-null, and (2) setting
it to false is the right thing to do.

Comment 3

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

Comment on attachment 8475283 [details] [diff] [review]
bug1055653-1.diff

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

::: content/media/BufferMediaResource.h
@@ +105,5 @@
>    virtual int64_t Tell() { return mOffset; }
>  
>    virtual void Pin() {}
>    virtual void Unpin() {}
> +  virtual double GetDownloadRate(bool* aIsReliable) {

May as well stick this all on one line like the others.

Comment 4

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a36f190b9ad1

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a36f190b9ad1