746813 - nsCanvasRenderingContext2D::GetMozCurrentTransformInverse crash with large canvas

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Created attachment 616369 [details]
testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Created attachment 616370 [details]
stack trace

Comment 2

[Jesse Ruderman]

Opt: bp-bdbe2955-d9ce-487c-a72e-d0a0a2120419

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Needs moar EnsureCurrentSurface.

phone

Comment 4

[Scoobidiver (away)]

On Windows 7: bp-72e70eb8-0995-401d-b377-406672120419

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Created attachment 616774 [details] [diff] [review]
Check for context allocation failure before returning inverse transform

Thanks Jesse!

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 616774 [details] [diff] [review]
Check for context allocation failure before returning inverse transform

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

This needs to be fixed in nsCanvasRenderingContext2DAzure as well - otherwise we'll a) have this bug on some systems but not others and b) make crashtest go orange.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Well, OK, but in the port to azure canvas many/most of the EnsureCurrentSurface()-style checks were lost.  This is putting ones finger in dike.  Is there a bug on file for restoring them?  Are we fuzzing azure canvas?

Comment 8

[Jesse Ruderman]

We are fuzzing azure canvas, but only on the (OS version, hardware) combinations in the build pool.  The fuzzer randomizes gfx.canvas.azure.enabled so both azure and non-azure canvas are tested on as many platforms as possible.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK.  There are many azure-canvas interfaces that don't check for a valid surface whereas the same cairo-canvas interface does.  (I assumed the checks weren't needed for azure-canvas.)  Apparently we have a dearth of tests for those cases, and perhaps our fuzzer is getting unlucky?

Comment 10

[Bob Clary [:bc:]]

fwiw, crash automation reproduced this on all three branches: Beta/12, Aurora/13, Nightly/14 and all 3 platforms.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Created attachment 631615 [details] [diff] [review]
Check for context allocation failure before returning inverse transform, v2

Was cleaning mq house and came back across this.

This version puts a few more fingers in the azure context dike.

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 631615 [details] [diff] [review]
Check for context allocation failure before returning inverse transform, v2

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

Well, not azure content so much as azure canvas, but hooray. And thanks for fixing both implementations!

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a07fd72eb33d

Comment 14

[Matt Brubeck (:mbrubeck)]

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