1183363 - Canvas isPointInPath crash

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox)

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

Nightly: bp-ec197e46-c0b8-46d5-9ae9-3e3562150713

Comment 3

[Jesse Ruderman]

Attachment: testcase that crashes in mozilla::dom::CanvasRenderingContext2D::MoveTo

In both cases, mTarget is null and something tries to call a GetTransform on it.

Comment 4

[Jesse Ruderman]

I'm seeing similar problems with lots of signatures, so this effectively prevents me from fuzzing canvas.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Canvas2D, so over to :jrmuizel.

Comment 6

[Lee Salzman [:lsalzman]]

I tracked this to a regression caused by the part 3 patch from Bas in bug 1167235. 

CanvasRenderingContext2D::ReturnTarget() gets called, nulling out mTarget, so when mTarget->GetTransform() is called, mTarget is null. Prior to said patch, mTarget would have still been around by now. But it looks like the transform can be picked up from elsewhere instead of the draw target in this case. There are a few little other possible crashes nearby.

I'll work up a fixed for this soon.

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: make EnsureWritablePath and EnsureUserSpacePath always imply EnsureTarget

This patch makes sure that EnsureWritablePath and EnsureUserSpacePath always call EnsureTarget so that any downstream users of mTarget will always have a valid mTarget to work with.

Comment 8

[Lee Salzman [:lsalzman]]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4abb4f6161f

Comment 9

[Lee Salzman [:lsalzman]]

Comment on attachment 8633296 [details] [diff] [review]
make EnsureWritablePath and EnsureUserSpacePath always imply EnsureTarget

Approval Request Comment
[Feature/regressing bug #]: bug 1167235 - Persist buffer provider requires that the draw target between transactions, so code that previously relied on the existence of a draw target could possibly now manipulate a null draw target.
[User impact if declined]: JS Canvas2D code can be used to crash Firefox.
[Describe test coverage new/current, TreeHerder]: mochitest
[Risks and why]: There should be little risk of regression here as concerns stability. It's possible that certain operations like IsPointInPath might slightly regress in performance due to needing to now acquire a draw target first, but in discussion this was deemed acceptable to keep the code saner/easier to understand/safer.
[String/UUID change made/needed]: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Can we land the testcase as a crashtest too?

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/827fc7ff5fee

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/827fc7ff5fee

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: add canvas crashtest for bug 1183363

This just the testcases into a single crashtest for tracking both.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63ec4e50cb90

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63ec4e50cb90

Comment 16

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

Comment on attachment 8633296 [details] [diff] [review]
make EnsureWritablePath and EnsureUserSpacePath always imply EnsureTarget

This has an associated crashtest and the try push looks clean. Let's uplift to Aurora.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/32b78aec27e8
https://hg.mozilla.org/releases/mozilla-aurora/rev/4938bcc2fb6b