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

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: 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

Comment 1

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

mozregression points to bug 1313972 - Avoid racing condition by making sure postMessage events are consumed by the correct tests. r=smaug

Comment 2

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

(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.

Comment 3

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

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

Comment 4

[Mats Palmgren (inactive)]

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

Comment 5

[Mats Palmgren (inactive)]

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...

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix+test

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c11e064c89865b824bfe9955cc3044d33e161cbf

Comment 7

[Markus Stange [: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?

Comment 8

[Mats Palmgren (inactive)]

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.

Comment 9

[Markus Stange [:mstange]]

Oh, ok, disregard my comment then.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c4bfddc020f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta when you feel it's ready.

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

[Gerry Chang [:gchang]]

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

Fix a crash. Beta54+. Should be in 54 beta 1.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5054aabe5c82

Comment 16

[Andrei Vaida [:avaida]]

(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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

This is needed for bug 1355873, ESR52.2+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/e78c943af07f