821395 - Uninitialised value use in gfxUtils::GetYCbCrToRGBDestFormatAndSize

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Julian Seward [:jseward]]

This one has been around quite a long time, and I thought it was a
Valgrind false positive, but I don't think so any more.

TEST_PATH=content/canvas/test/crossorigin/test_video_crossorigin.html

running on a 16-bit deep X server

Conditional jump or move depends on uninitialised value(s)
   at 0x726FB17: gfxUtils::GetYCbCrToRGBDestFormatAndSize(mozilla::layers::
                 PlanarYCbCrImage::Data const&, gfxASurface::gfxImageFormat&,
                 nsIntSize&) (gfx/thebes/gfxUtils.cpp:647)
   by 0x7291C6F: mozilla::layers::PlanarYCbCrImage::GetAsSurface()
                 (gfx/layers/ImageContainer.cpp:490)
   by 0x729254C: mozilla::layers::ImageContainer::GetCurrentAsSurface(nsIntSize*)
                 (gfx/layers/ImageContainer.cpp:323)
   by 0x6572BC4: nsLayoutUtils::SurfaceFromElement(nsHTMLVideoElement*, unsigned int)
                 (layout/base/nsLayoutUtils.cpp:4515)
   by 0x6572E3B: nsLayoutUtils::SurfaceFromElement(mozilla::dom::Element*, unsigned int)
                (layout/base/nsLayoutUtils.cpp:4552)
   by 0x67F6D17: mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::
                 HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElement const&, double,
                 double, double, double, double, double, double, double, unsigned char,
                 mozilla::ErrorResult&)
                 (content/canvas/src/CanvasRenderingContext2D.cpp:2972)
   by 0x7122641: mozilla::dom::CanvasRenderingContext2DBinding::drawImage(JSContext*,
                 JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*,
                 unsigned int, JS::Value*) (ff-opt/dom/bindings/../../dist/include/
                 mozilla/dom/CanvasRenderingContext2D.h:258)
   by 0x711D807: mozilla::dom::CanvasRenderingContext2DBinding::genericMethod(JSContext*,
                 unsigned int, JS::Value*)
                 (ff-opt/dom/bindings/CanvasRenderingContext2DBinding.cpp:3244)
   by 0x7800115: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct)
                 (js/src/jscntxtinlines.h:364)
   by 0x77FA56D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode)
                 (js/src/jsinterp.cpp:2348)
   by 0x77FFB1F: js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*)
                 (js/src/jsinterp.cpp:346)
   by 0x7800277: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct)
                 (js/src/jsinterp.cpp:404)

 Uninitialised value was created by a heap allocation
   at 0x402ADDC: malloc (coregrind/m_replacemalloc/vg_replace_malloc.c:270)
   by 0x403F080: moz_xmalloc (memory/mozalloc/mozalloc.cpp:54)
   by 0x7292924: mozilla::layers::ImageFactory::CreateImage(mozilla::ImageFormat const*,
                 unsigned int, nsIntSize const&, mozilla::layers::BufferRecycleBin*)
                 (ff-opt/gfx/layers/../../dist/include/mozilla/mozalloc.h:200)
   by 0x729209B: mozilla::layers::ImageContainer::CreateImage(mozilla::ImageFormat const*,
                 unsigned int) (gfx/layers/ImageContainer.cpp:154)
   by 0x6B6940B: mozilla::VideoData::Create(mozilla::VideoInfo&,
                 mozilla::layers::ImageContainer*, long, long, long,
                 mozilla::VideoData::YCbCrBuffer const&, bool, long, nsIntRect)
                 (content/media/MediaDecoderReader.cpp:210)
   by 0x6B7EB16: mozilla::OggReader::DecodeTheora(ogg_packet*, long) (content/media
                 /ogg/OggReader.cpp:770)
   by 0x6B7FCAF: mozilla::OggReader::DecodeVideoFrame(bool&, long) (content/media
                 /ogg/OggReader.cpp:815)
   by 0x6B68917: mozilla::MediaDecoderStateMachine::DecodeLoop() (content/media
                 /MediaDecoderStateMachine.cpp:851)
   by 0x6B68BB8: mozilla::MediaDecoderStateMachine::DecodeThreadRun() (content/media
                 /MediaDecoderStateMachine.cpp:483)
   by 0x632C994: nsRunnableMethodImpl<void (nsPACMan::*)(), true>::Run()
                 (ff-opt/content/html/content/src/../../../../dist/include
                 /nsThreadUtils.h:350)
   by 0x71FBFDA: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:627)
   by 0x71C63C8: NS_ProcessNextEvent_P(nsIThread*, bool)
                 (ff-opt/xpcom/build/nsThreadUtils.cpp:221)

Comment 1

[Julian Seward [:jseward]]

Digging around a bit more: the reported error point is

gfxUtils.cpp: gfxUtils::GetYCbCrToRGBDestFormatAndSize
647  if (aSuggestedFormat == gfxASurface::ImageFormatRGB16_565) {

gfxASurface::ImageFormatRGB16_565 is a constant, so it must be
aSuggestedFormat.  That is passed in as the middle parameter
in this call:

ImageContainer.cpp: PlanarYCbCrImage::GetAsSurface()
487  gfxASurface::gfxImageFormat format = GetOffscreenFormat();
488
489  gfxIntSize size(mSize);
490  gfxUtils::GetYCbCrToRGBDestFormatAndSize(mData, format, size);

so the bad value is returned by GetOffscreenFormat(), which is
just this:

gfx/layers/ImageContainer.h:
725  gfxASurface::gfxImageFormat GetOffscreenFormat() { return mOffscreenFormat; }

Sticking a VALGRIND_CHECK_VALUE_IS_DEFINED(mOffscreenFormat) confirms
that mOffscreenFormat is undefined.



Figuring out the place where the offending object was allocated (and
hence its type) is not too easy, because some frames are missing in
the trace.  More digging around revealed this sequence:

gfx/layers/ImageContainer.cpp:
142 already_AddRefed<Image>
143 ImageContainer::CreateImage(const ImageFormat *aFormats,
144                             uint32_t aNumFormats)
145 {
...
154   return mImageFactory->CreateImage(aFormats, aNumFormats, mScaleHint, mRecycleBin);
155 }

which calls to

gfx/layers/ImageContainer.cpp:
44 already_AddRefed<Image>
45 ImageFactory::CreateImage(const ImageFormat *aFormats,
46                           uint32_t aNumFormats,
47                           const gfxIntSize &,
48                           BufferRecycleBin *aRecycleBin)
49 {
...
60   if (FormatInList(aFormats, aNumFormats, PLANAR_YCBCR)) {
61     img = new PlanarYCbCrImage(aRecycleBin);
62     return img.forget();
63   }
 
which calls to

gfx/layers/ImageContainer.cpp:
401 PlanarYCbCrImage::PlanarYCbCrImage(BufferRecycleBin *aRecycleBin)
402   : Image(nullptr, PLANAR_YCBCR)
403   , mBufferSize(0)
404   , mRecycleBin(aRecycleBin)
405 {
406 }

What this doesn't appear to do is set
PlanarYCbCrImage::mOffscreenFormat to anything.

So either the constructor forgot to initialise mOffscreenFormat, or
there is some logic error in the use of these objects, leading to
::GetOffscreenFormat being called before ::SetOffscreenFormat is.  But
I can't guess which.

Comment 2

[Julian Seward [:jseward]]

Attachment: trivial fix, to initialise mOffscreenFormat

As per comment #1 I have no idea if this is really the right thing
to do, but it stops V complaining at least.

Comment 3

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9159cb7be55f

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9159cb7be55f