Closed Bug 1357092 Opened 3 years ago Closed 3 years ago

Crash [@mozilla::dom::CanvasRenderingContext2D::BezierTo]


(Core :: Canvas: 2D, defect, critical)

Not set



Tracking Status
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed


(Reporter: jkratzer, Assigned: mats)


(Blocks 1 open bug)


(Keywords: crash, csectype-nullptr, testcase, Whiteboard: [gfx-noted])

Crash Data


(2 files)

Attached file Testcase
Testcase found while fuzzing mozilla-central rev 20170414-1a1069b27f40.  This appears to be a recent regression due to the high number of testcases in the past 72 hours.

==29393==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f35fb9d014f bp 0x7ffd96247530 sp 0x7ffd96247460 T0)
==29393==The signal is caused by a READ memory access.
==29393==Hint: address points to the zero page.
    #0 0x7f35fb9d014e in mozilla::dom::CanvasRenderingContext2D::BezierTo(mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&) /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/CanvasRenderingContext2D.h:532:23
    #1 0x7f35fb9cfc7b in BezierCurveTo /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/CanvasRenderingContext2D.h:351:5
    #2 0x7f35fb9cfc7b in mozilla::dom::CanvasRenderingContext2DBinding::bezierCurveTo(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:4850
    #3 0x7f35fc56987e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2953:13
    #4 0x7f3601de10a3 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #5 0x7f3601de10a3 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:455
    #6 0x7f3601dc9fde in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:506:12
    #7 0x7f3601dc9fde in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3010
    #8 0x7f3601db00f8 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:395:12
    #9 0x7f3601de35b7 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:684:15
    #10 0x7f3601de3e22 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:716:12
    #11 0x7f360277f4ad in Evaluate(JSContext*, js::ScopeKind, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:4599:19
    #12 0x7f36027800c0 in Evaluate /home/worker/workspace/build/src/js/src/jsapi.cpp:4624:12
    #13 0x7f36027800c0 in JS::Evaluate(JSContext*, JS::AutoObjectVector&, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:4682
    #14 0x7f35facc4662 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&) /home/worker/workspace/build/src/dom/base/nsJSUtils.cpp:232:8
    #15 0x7f35fad46244 in nsScriptLoader::EvaluateScript(nsScriptLoadRequest*) /home/worker/workspace/build/src/dom/base/nsScriptLoader.cpp:2267:23
Flags: in-testsuite?
mozregression points to bug 1313972 - Avoid racing condition by making sure postMessage events are consumed by the correct tests. r=smaug
Flags: needinfo?(bugs)
(In reply to Milan Sreckovic [:milan] from comment #1)
> mozregression points to bug 1313972 - Avoid racing condition by making sure
> postMessage events are consumed by the correct tests. r=smaug

Which doesn't make much sense.  I'll do the run again.
Flags: needinfo?(bugs)
Whiteboard: [gfx-noted]
BezierTo uses mTarget and mDSPathBuilder without checking if they are valid:
Don't know why bug 1355873 would cause this change in behavior though.
I'll investigate...

I could be wrong, but it seems to me this is an existing bug though.
Maybe should call EnsureTarget there, or at least check with IsTargetValid()
and null-check mDSPathBuilder? because I don't think either of those are
guaranteed to be valid (and weren't before bug 1355873 either).

Seems like we have the same issue in QuadraticCurveTo:
So, the "o1.setAttribute("width", "100000")" in the test makes EnsureTarget() fail here:
That happens with/without my changes though.

The regression is that I added this check in EnsureWritablePath(), which is called
at the start of QuadraticCurveTo/BezierCurveTo:
so now we return with a null mPathBuilder which makes us take the else-branch
in BezierTo and use a null mTarget.  Before my change, EnsureWritablePath()
would go ahead and create a path builder (for sErrorTarget) and use that.

I guess I can remove the IsTargetValid() check I added in EnsureWritablePath()
since callers seems to depend on that behavior...
Flags: needinfo?(mats)
Comment on attachment 8858939 [details] [diff] [review]

Review of attachment 8858939 [details] [diff] [review]:

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3652,2 @@
>    if (mDSPathBuilder) {

Should we add a MOZ_RELEASE_ASSERT(mPath) to this early return?
Attachment #8858939 - Flags: review?(mstange) → review+
Just want to correct my earlier comment for the record:

> so now we return with a null mPathBuilder which makes us take the else-branch
> in BezierTo and use a null mTarget.

It was mDSPathBuilder that was null that we crashed on accessing.
mTarget was sErrorTarget at that point.
Oh, ok, disregard my comment then.
Pushed by
Make EnsureWritablePath() always try to create a mPathBuilder, also when we don't have a valid target (i.e. for sErrorTarget).  r=mstange
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Crash Signature: [@ mozilla::dom::CanvasRenderingContext2D::BezierTo]
Please nominate this for Beta when you feel it's ready.
Flags: needinfo?(mats)
Flags: in-testsuite? → in-testsuite+
Comment on attachment 8858939 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]:bug 1355873
[User impact if declined]:crash
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:bug 1355873
[Is the change risky?]:no
[Why is the change risky/not risky?]:just reverting a change from bug 1355873 to the old behavior
[String changes made/needed]:none
Flags: needinfo?(mats)
Attachment #8858939 - Flags: approval-mozilla-beta?
Comment on attachment 8858939 [details] [diff] [review]

Fix a crash. Beta54+. Should be in 54 beta 1.
Attachment #8858939 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Mats Palmgren (:mats) from comment #13)
> [Is this code covered by automated tests?]:yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Mats' assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment on attachment 8858939 [details] [diff] [review]

I assume we'll want to uplift this to ESR52 as well once bug 1355873 gets approved.
Attachment #8858939 - Flags: approval-mozilla-esr52?
Comment on attachment 8858939 [details] [diff] [review]

This is needed for bug 1355873, ESR52.2+
Attachment #8858939 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.