Canvas isPointInPath crash

RESOLVED FIXED in Firefox 41

Status

()

--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jruderman, Assigned: lsalzman)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
mozilla42
crash, regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed)

Details

(Whiteboard: [fuzzblocker:CanvasRenderingContext2D], crash signature)

Attachments

(5 attachments)

(Reporter)

Description

3 years ago
Created attachment 8633129 [details]
testcase (crashes Firefox)
(Reporter)

Comment 1

3 years ago
Created attachment 8633131 [details]
stack
(Reporter)

Comment 3

3 years ago
Created attachment 8633239 [details]
testcase that crashes in mozilla::dom::CanvasRenderingContext2D::MoveTo

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

Updated

3 years ago
Whiteboard: [fuzzblocker:CanvasRenderingContext2D]
(Reporter)

Comment 4

3 years ago
I'm seeing similar problems with lots of signatures, so this effectively prevents me from fuzzing canvas.
Flags: needinfo?(jgilbert)
Canvas2D, so over to :jrmuizel.
Flags: needinfo?(jgilbert) → needinfo?(jmuizelaar)
(Assignee)

Comment 6

3 years ago
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.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 7

3 years ago
Created attachment 8633296 [details] [diff] [review]
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.
Attachment #8633296 - Flags: review?(bas)
Attachment #8633296 - Flags: review?(bas) → review+
(Assignee)

Comment 8

3 years ago
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4abb4f6161f
Keywords: checkin-needed
(Assignee)

Comment 9

3 years ago
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
Attachment #8633296 - Flags: approval-mozilla-aurora?
Can we land the testcase as a crashtest too?
Flags: needinfo?(lsalzman)
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/827fc7ff5fee
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Comment 13

3 years ago
Created attachment 8634359 [details] [diff] [review]
add canvas crashtest for bug 1183363

This just the testcases into a single crashtest for tracking both.
Flags: needinfo?(lsalzman)
Attachment #8634359 - Flags: review?(bas)
Attachment #8634359 - Flags: review?(bas) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Flags: in-testsuite? → in-testsuite+

Updated

3 years ago
status-firefox41: --- → affected
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.
Attachment #8633296 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.