1026998 - Uninitialised value use in CanvasRenderingContext2D::EnsureTarget

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Julian Seward [:jseward]]

TEST_PATH=content/base/test/test_fileapi_slice.html

produces a whole avalanche of uninitialised value errors, the first of
which is

Conditional jump or move depends on uninitialised value(s)
   at 0x6AC0452: mozilla::dom::CanvasRenderingContext2D::EnsureTarget() (content/canvas/src/CanvasRenderingContext2D.cpp:916)
   by 0x6ACDFF6: mozilla::dom::CanvasRenderingContext2D::SetIsOpaque(bool) (content/canvas/src/CanvasRenderingContext2D.cpp:1073)
   by 0x6B07D3A: mozilla::dom::HTMLCanvasElement::UpdateContext(JSContext*, JS::Handle<JS::Value>) (content/html/content/src/HTMLCanvasElement.cpp:806)
   by 0x6B080F1: mozilla::dom::HTMLCanvasElement::GetContext(JSContext*, nsAString_internal const&, JS::Handle<JS::Value>, mozilla::ErrorResult&) (content/html/content/src/HTMLCanvasElement.cpp:732)
   by 0x62631E2: mozilla::dom::HTMLCanvasElementBinding::getContext(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLCanvasElement*, JSJitMethodCallArgs const&) (ff-O-linux64/dom/bindings/HTMLCanvasElementBinding.cpp:218)
   by 0x65B9026: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (dom/bindings/BindingUtils.cpp:2347)
   by 0x7C3ABC8: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/jscntxtinlines.h:241)
   by 0x7C35902: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2566)
   by 0x7C39033: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:402)
   by 0x7C39209: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) (js/src/vm/Interpreter.cpp:610)
   by 0x7C39331: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (js/src/vm/Interpreter.cpp:647)
   by 0x7B8AB18: Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::Value*) (js/src/jsapi.cpp:4988)

 Uninitialised value was created by a heap allocation
   at 0x4809064: malloc (/home/sewardj/VgTRUNK/merge/coregrind/m_replacemalloc/vg_replace_malloc.c:292)
   by 0x481D86B: moz_xmalloc (memory/mozalloc/mozalloc.cpp:52)
   by 0x6B079DC: mozilla::dom::HTMLCanvasElement::GetContextHelper(nsAString_internal const&, nsICanvasRenderingContextInternal**) (ff-O-linux64/content/html/content/src/../../../../dist/include/mozilla/mozalloc.h:201)
   by 0x6B0806D: mozilla::dom::HTMLCanvasElement::GetContext(JSContext*, nsAString_internal const&, JS::Handle<JS::Value>, mozilla::ErrorResult&) (content/html/content/src/HTMLCanvasElement.cpp:717)
   by 0x62631E2: mozilla::dom::HTMLCanvasElementBinding::getContext(JSContext*, JS::Handle<JSObject*>, mozilla::dom::HTMLCanvasElement*, JSJitMethodCallArgs const&) (ff-O-linux64/dom/bindings/HTMLCanvasElementBinding.cpp:218)
   by 0x65B9026: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (dom/bindings/BindingUtils.cpp:2347)
   by 0x7C3ABC8: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) (js/src/jscntxtinlines.h:241)
   by 0x7C35902: Interpret(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:2566)
   by 0x7C39033: js::RunScript(JSContext*, js::RunState&) (js/src/vm/Interpreter.cpp:402)
   by 0x7C39209: js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) (js/src/vm/Interpreter.cpp:610)
   by 0x7C39331: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (js/src/vm/Interpreter.cpp:647)
   by 0x7B8AB18: Evaluate(JSContext*, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::Value*) (js/src/jsapi.cpp:4988)

Comment 1

[Julian Seward [:jseward]]

Peering a bit at the code, the complaints start here

CanvasRenderingContext2D.cpp, CanvasRenderingContext2D::EnsureTarget()

915   IntSize size(mWidth, mHeight);
916   if (size.width <= 0xFFFF && size.height <= 0xFFFF &&
917       size.width >= 0 && size.height >= 0) {

It complains about all 4 comparisons, which implies both mWidth
and mHeight are undefined.  

CanvasRenderingContext2D::CanvasRenderingContext2D() at line 561
doesn't appear to initialise them.  I tried the following hack

 CanvasRenderingContext2D::CanvasRenderingContext2D()
   : mForceSoftware(false), mZero(false), mOpaque(false), mResetLayer(true)
   , mIPC(false)
   , mStream(nullptr)
   , mIsEntireFrameInvalid(false)
   , mPredictManyRedrawCalls(false), mPathTransformWillUpdate(false)
   , mInvalidateCount(0)
+,mWidth(0),mHeight(0)
 {
   sNumLivingContexts++;
   SetIsDOMBinding();
 }

and that seems to get rid of all of the Memcheck complaints.

Anyone familiar with this code know if that's a valid fix, or if it
merely papers over some deeper problem?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Jeff, is this class one of those that magically gets all the fields zero'd out on construction, without having to do this explicitly, and Memcheck is just "confused"?  I can never remember where to look.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Intialize the canvas width and height members to the default values

This looks like a bug to me. Here's a possible fix.

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8447460 [details] [diff] [review]
Intialize the canvas width and height members to the default values

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

I'm fine with this, but don't we technically expect the size to be set by the time EnsureTarget is called? It sort of sounds to me like there might also be some other unexpected behavior. Although this patch by itself is a good idea.

Comment 5

[Julian Seward [:jseward]]

Attachment: bug1026998c5.diff

Removes extraneous comma.  No other changes.

I verified that this gets rid of the V complaints I saw.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b00d9897dc6

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b00d9897dc6