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

RESOLVED FIXED in Firefox -esr52



2 years ago
2 years ago


(Reporter: jkratzer, Assigned: mats)


(Blocks: 2 bugs, {crash, csectype-nullptr, testcase})

crash, csectype-nullptr, testcase
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr52 fixed, firefox53 wontfix, firefox54 fixed, firefox55 fixed)


(Whiteboard: [gfx-noted], crash signature)


(2 attachments)



2 years ago
Created attachment 8858817 [details]

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]

Comment 4

2 years ago
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:

Comment 5

2 years ago
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+

Comment 8

2 years ago
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.

Comment 10

2 years ago
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

Comment 11

2 years ago
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Crash Signature: [@ mozilla::dom::CanvasRenderingContext2D::BezierTo]
Please nominate this for Beta when you feel it's ready.
status-firefox52: --- → wontfix
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Flags: needinfo?(mats)
Flags: in-testsuite? → in-testsuite+

Comment 13

2 years ago
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?


2 years ago
Blocks: 1289929
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+

Comment 15

2 years ago
status-firefox54: affected → fixed
(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+

Comment 19

2 years ago
status-firefox-esr52: affected → fixed
You need to log in before you can comment on or make changes to this bug.