Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 54

Status

()

Core
Canvas: 2D
--
critical
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: jkratzer, Assigned: Mats Palmgren (vacation - back in August))

Tracking

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

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

Firefox Tracking Flags

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

Details

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

Attachments

(2 attachments)

(Reporter)

Description

3 months ago
Created attachment 8858817 [details]
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)
This looks more reasonable:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3243c8fc3ce7831dda843b60d6bb2d7e4acf1fd4&tochange=198effec6e11460e977f13e9cb83050a21a64d27

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e37c70b7fd23f4c5f6f41fb6b13feb39de3048c1&tochange=f40b66381b6f7dac0786cd638a17cda04bdba242

Mats, I'm going to guess bug 1355873
Flags: needinfo?(mats)
Whiteboard: [gfx-noted]
Blocks: 1355873
BezierTo uses mTarget and mDSPathBuilder without checking if they are valid:
http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/dom/canvas/CanvasRenderingContext2D.h#531-532
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:
http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/dom/canvas/CanvasRenderingContext2D.h#338-339
So, the "o1.setAttribute("width", "100000")" in the test makes EnsureTarget() fail here:
http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/dom/canvas/CanvasRenderingContext2D.cpp#1640-1647
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:
http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/dom/canvas/CanvasRenderingContext2D.cpp#3804-3806
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)
Created attachment 8858939 [details] [diff] [review]
fix+test

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c11e064c89865b824bfe9955cc3044d33e161cbf
Assignee: nobody → mats
Attachment #8858939 - Flags: review?(mstange)
Comment on attachment 8858939 [details] [diff] [review]
fix+test

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.

Comment 10

3 months ago
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c4bfddc020f
Make EnsureWritablePath() always try to create a mPathBuilder, also when we don't have a valid target (i.e. for sErrorTarget).  r=mstange
https://hg.mozilla.org/mozilla-central/rev/5c4bfddc020f
Status: NEW → RESOLVED
Last Resolved: 3 months 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 on attachment 8858939 [details] [diff] [review]
fix+test

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?
(Reporter)

Updated

3 months ago
Blocks: 1289929
Comment on attachment 8858939 [details] [diff] [review]
fix+test

Fix a crash. Beta54+. Should be in 54 beta 1.
Attachment #8858939 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 15

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5054aabe5c82
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]
fix+test

I assume we'll want to uplift this to ESR52 as well once bug 1355873 gets approved.
Attachment #8858939 - Flags: approval-mozilla-esr52?

Comment 18

3 months ago
Comment on attachment 8858939 [details] [diff] [review]
fix+test

This is needed for bug 1355873, ESR52.2+
Attachment #8858939 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 19

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/e78c943af07f
status-firefox-esr52: affected → fixed
You need to log in before you can comment on or make changes to this bug.