If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Uninitialised value use in CanvasRenderingContext2D::EnsureTarget

RESOLVED FIXED in mozilla33

Status

()

Core
Canvas: 2D
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jseward, Assigned: jrmuizel)

Tracking

Trunk
mozilla33
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

3 years ago
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?
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.
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 3

3 years ago
Created attachment 8447460 [details] [diff] [review]
Intialize the canvas width and height members to the default values

This looks like a bug to me. Here's a possible fix.
Assignee: nobody → jmuizelaar
Attachment #8447460 - Flags: review?(bas)
Flags: needinfo?(jmuizelaar)
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.
Attachment #8447460 - Flags: review?(bas) → review+
(Reporter)

Comment 5

3 years ago
Created attachment 8448759 [details] [diff] [review]
bug1026998c5.diff

Removes extraneous comma.  No other changes.

I verified that this gets rid of the V complaints I saw.
Attachment #8447460 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8448759 - Flags: review+
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b00d9897dc6
https://hg.mozilla.org/mozilla-central/rev/2b00d9897dc6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

Updated

3 years ago
Depends on: 1035244
You need to log in before you can comment on or make changes to this bug.