745818 - Crash with large canvas as source pattern

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Opt: bp-61dcf722-21ad-45eb-bff5-ecb1c2120416

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jeff Muizelaar [:jrmuizel]]

It's very sad, but it might be best to just pref this off for this cycle.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Turn off coregraphics azure

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Bas, do you think you could look to see how hard this is to fix properly?

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 615385 [details] [diff] [review]
Turn off coregraphics azure

Review of attachment 615385 [details] [diff] [review]:
-----------------------------------------------------------------

Very sad indeed. I'd love to look at it.

Comment 6

[Alex Keybl [:akeybl]]

Comment on attachment 615385 [details] [diff] [review]
Turn off coregraphics azure

[Triage Comment]
Pre-approving since this is a simple pref change and we really want to get this into Beta 6 if this is how we need to ship FF12.

Comment 7

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a2c0eba4cd29

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/releases/mozilla-beta/rev/7446fa39b65c - you'll need to adjust our expectations about a couple of reftests, since that makes one unexpectedly pass and one unexpectedly fail.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

One thing worth consider is whether this bug is severe enough to warrant the big pref change. As far as I can see, this is a NULL deference on malicious input which isn't _that_ bad...

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> One thing worth consider is whether this bug is severe enough to warrant the
> big pref change. As far as I can see, this is a NULL deference on malicious
> input which isn't _that_ bad...

Jeff and I discussed this bug in #gfx today. We discussed the fact that the bug has very low crash volume, shouldn't happen in normal web content, and may cause unforeseen regressions since the pref has been set the other way this entire cycle. Untracking for FF12 and wontfixing.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Bas, how do you handle this situation with Direct2D?

Comment 12

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> Bas, how do you handle this situation with Direct2D?

We fallback to Thebes-canvas(and longer term Skia-Azure), when a canvas is more than 8192 pixels in any direction, because SetDimensions fails to create a DrawTarget. It can cause some issues if you make the canvas larger than supported -after- it's already been created, in that case we fail silently having a dummy canvas. All this logic is in CanvasRenderingContext2DAzure so all you really need to do is make CreateDrawTarget return NULL.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Attachment: Ignore invalid cairo surfaces

With bug 686453, we're not really supposed to get NULL surfaces anymore. Instead we get Invalid ones that we should still ignore.

Comment 14

[Jesse Ruderman]

You could update the comment to something like "Ignore NULL and invalid surfaces".

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 615994 [details] [diff] [review]
Ignore invalid cairo surfaces

[Approval Request Comment]
Regression caused by (bug #): 692879
User impact if declined: Occasional crashes
Testing completed (on m-c, etc.): none yet
Risk to taking this patch (and alternatives if risky): very minimal, we take an already tested bail out earlier
String changes made by this patch: none

Comment 16

[Jeff Muizelaar [:jrmuizel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b31c3121e98

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f474ef88b11c

Comment 18

[Ed Morley [:emorley]]

and https://hg.mozilla.org/mozilla-central/rev/3b31c3121e98

Comment 19

[Jeff Muizelaar [:jrmuizel]]

Reopening because this crash still happens with other backends.

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 615994 [details] [diff] [review]
Ignore invalid cairo surfaces

[Triage Comment]
Approved for Aurora 14 since this is low risk and we're still very early in the cycle.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Closing ye olde bug; if there are still crashes with other backends we can file a new bug for them.