Closed Bug 1183363 Opened 7 years ago Closed 7 years ago

Canvas isPointInPath crash


(Core :: Graphics: Canvas2D, defect)

Not set



Tracking Status
firefox41 --- fixed
firefox42 --- fixed


(Reporter: jruderman, Assigned: lsalzman)



(Keywords: crash, regression, testcase, Whiteboard: [fuzzblocker:CanvasRenderingContext2D])

Crash Data


(5 files)

No description provided.
Attached file stack
In both cases, mTarget is null and something tries to call a GetTransform on it.
Whiteboard: [fuzzblocker:CanvasRenderingContext2D]
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)
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
Flags: needinfo?(jmuizelaar)
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+
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?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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+
Keywords: checkin-needed
Flags: in-testsuite? → in-testsuite+
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.