1524554 - Crash in mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern when using Canvas

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

This was an error handling case I hadn't quite predicted. In some situations the device can be reset inbetween a Canvas surface being created and it being first used. This situation should be rare, but there are examples of it happening.

For example 1519e40e-6183-40c7-90a5-7f1e80190201.

Adding a patch which is not superpretty, but minimally invasive so we can upload to beta if this bug turns out to be present in sufficient numbers there.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1524554: Ensure Canvas surfaces are initialized on the main thread and ensure their validity. r=rhunt

This isn't the prettiest solution but it's minimally invasive. More long-term a better solution may be to expose a call on DrawTargets to ensure their initialization even if they're on the main thread. IsValid probably isn't a good candidate for this as we want it to be usable freely on the main thread to ensure none of the basic conditions of the surface are still valid.

Comment 2

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Sounds like a nice crash fix might be coming to beta 66 soon.... Tracking so I keep an eye on this.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/161ce405f2e0
Ensure Canvas surfaces are initialized on the main thread and ensure their validity. r=rhunt

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 9040680 [details]
Bug 1524554: Ensure Canvas surfaces are initialized on the main thread and ensure their validity. r=rhunt

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1519760

User impact if declined

Crashes

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This just forces on main thread initialization of the Canvas, we've always done this in the past.

String changes made/needed

None

Comment 5

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9040680 [details]
Bug 1524554: Ensure Canvas surfaces are initialized on the main thread and ensure their validity. r=rhunt

Part of fix for top crash in beta; tests look ok on inbound, I'll take it for beta 5.

[Triage Comment]

Comment 6

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/161ce405f2e0

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/02146faf5ac1

Comment 8

[Bas Schouten (:bas.schouten)]

I still see some of these. Investigating.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1524554 - Part 2: Ensure DrawTarget validity for AdjustedTargets inside Canvas. r=rhunt

Depends on D21900

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb993d3d4e66
Part 2: Ensure DrawTarget validity for DTs created inside Canvas. r=rhunt

Comment 11

[Natalia Csoregi [:nataliaCs]]

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

Comment 12

[Bas Schouten (:bas.schouten)]

Still seeing one of these crashes on Trunk, I'll have to keep a close eye on this.

Comment 13

[Bas Schouten (:bas.schouten)]

Comment on attachment 9048120 [details]
Bug 1524554 - Part 2: Ensure DrawTarget validity for AdjustedTargets inside Canvas. r=rhunt

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1519760
• User impact if declined: Crashes
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just additional verification
• String changes made/needed: None

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 9048120 [details]
Bug 1524554 - Part 2: Ensure DrawTarget validity for AdjustedTargets inside Canvas. r=rhunt

More help for a high volume DrawTarget crash, let's uplift for the RC.

Comment 15

[Narcis Beleuzu [:NarcisB]]

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

Comment 16

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1524554 - Part 3: Ensure DT validation in internal Canvas fallback path. r=rhunt

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4896662a24a
Part 3: Ensure DT validation in internal Canvas fallback path. r=rhunt

Comment 19

[Oana Pop-Rus]

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

Comment 20

[Bas Schouten (:bas.schouten)]

Looks like this last patch has finally eliminated all the crashes, do we want to upload it in some way?

Comment 21

[Bas Schouten (:bas.schouten)]

(The overview seems to be lying when it says 67.0b1 has the patch, so those crashes are without the patch)

Comment 22

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Wow! That's great news. It is just a little too late to get this into the 66 release. But, I could put it on the list of patches we'd take for a 66 dot release in a couple of weeks.

Can you file a new bug for that, for clarity? Since we have multiple patches that landed at different times here, this bug is confusing, and I'd like to keep the status flags more clear.

Comment 23

[Bas Schouten (:bas.schouten)]

Filed 1539118.