1190705 - Canvas rotate & fillText: SkFixed.h: failed assertion "n64 == n32"

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Jesse Ruderman]

Attachment: testcase

gfx/skia/skia/include/core/SkFixed.h:46: failed assertion "n64 == n32"

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: part 1 - ensure that canvas 2d matrix transforms are finite

The webidl for the Canvas functions currently handles if explicit Infs or NaNs are passed in to the transform functions, but it does not handle the case where a large double number becomes an Inf through truncation to float.

Further, in certain cases like the rotate function, the Inf angles passed in to trig functions give out NaNs.

Skia canvas' Mac scaled font backend and also apparently the cross-platform Cairo scaled font backend both can't handle any non-finite numbers in the font transform, otherwise they can trigger assertions like above or various downwind failures like null-derefs (for the Cairo backend).

To mitigate this, this patch tries to verify that the resulting draw target transform has all finite values, especially after it has been composed with the existing transform. This weed out both cases where truncation directly produces non-finite values and cases where valid inputs through repeated composition may still yield a non-finite transform.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: part 2 - add crashtest for canvas 2d bug 1190705

Comment 4

[Lee Salzman [:lsalzman]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2104e8fcce3e

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 8644073 [details] [diff] [review]
part 1 - ensure that canvas 2d matrix transforms are finite

Jeff, I feel okay with this, do you agree?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

How do other browsers handle these cases?

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> How do other browsers handle these cases?

Chrome ignores it in this case.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Lee Salzman [:eihrul] from comment #7)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> > How do other browsers handle these cases?
> 
> Chrome ignores it in this case.

So this makes us match them?

Comment 9

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> (In reply to Lee Salzman [:eihrul] from comment #7)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> > > How do other browsers handle these cases?
> > 
> > Chrome ignores it in this case.
> 
> So this makes us match them?

More or less, the transform seems to not take effect for them. In any event, this is much better than bombing out.

Comment 10

[Lee Salzman [:lsalzman]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> (In reply to Lee Salzman [:eihrul] from comment #7)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> > > How do other browsers handle these cases?
> > 
> > Chrome ignores it in this case.
> 
> So this makes us match them?

Also verified against Internet Explorer. This patch would make us match both Chrome and IE (and not crash).

Comment 11

[Jeff Muizelaar [:jrmuizel]]

It would be nice to add a test for case this to the web-platform-tests

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97de68014b7a
https://hg.mozilla.org/integration/mozilla-inbound/rev/da1c20faa1d3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/97de68014b7a
https://hg.mozilla.org/mozilla-central/rev/da1c20faa1d3