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

RESOLVED FIXED in Firefox 66

Status

()

defect
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: bas.schouten, Assigned: bas.schouten)

Tracking

unspecified
mozilla67
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox66+ fixed, firefox67 fixed)

Details

(crash signature)

Attachments

(3 attachments)

Assignee

Description

4 months ago

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.

Assignee

Updated

4 months ago
See Also: → 1521368
Assignee

Comment 1

4 months ago

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.

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

Crash Signature: [@ mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern]

Comment 3

4 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/161ce405f2e0
Ensure Canvas surfaces are initialized on the main thread and ensure their validity. r=rhunt
Assignee

Comment 4

4 months ago

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

Attachment #9040680 - Flags: approval-mozilla-beta?

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]

Attachment #9040680 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 6

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Flags: qe-verify-

Updated

3 months ago
See Also: → 1526045
Assignee

Comment 8

3 months ago

I still see some of these. Investigating.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #9048120 - Attachment description: Bug 1524554 - Part 2: Ensure DrawTarget validity for AdjustedTargets inside Canvas. r=rhunt → Bug 1524554 - Part 2: Ensure DrawTarget validity for DTs created inside Canvas. r=rhunt

Comment 10

3 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb993d3d4e66
Part 2: Ensure DrawTarget validity for DTs created inside Canvas. r=rhunt

Comment 11

3 months ago
bugherder
Status: REOPENED → RESOLVED
Last Resolved: 4 months ago3 months ago
Resolution: --- → FIXED
Assignee

Comment 12

3 months ago

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

Assignee

Comment 13

3 months ago

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
Attachment #9048120 - Attachment description: Bug 1524554 - Part 2: Ensure DrawTarget validity for DTs created inside Canvas. r=rhunt → Bug 1524554 - Part 2: Ensure DrawTarget validity for AdjustedTargets inside Canvas. r=rhunt
Attachment #9048120 - Flags: approval-mozilla-beta?

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.

Attachment #9048120 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1526656
Crash Signature: [@ mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern] → [@ mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern] [@ mozilla::gfx::DrawTargetD2D1::GetImageForSurface]

Comment 18

2 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4896662a24a
Part 3: Ensure DT validation in internal Canvas fallback path. r=rhunt
Assignee

Comment 20

2 months ago

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

Crash Signature: [@ mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern] [@ mozilla::gfx::DrawTargetD2D1::GetImageForSurface] → [@ mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern] [@ mozilla::gfx::DrawTargetD2D1::GetImageForSurface]
Flags: needinfo?(lhenry)
Assignee

Comment 21

2 months ago

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

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.

Flags: needinfo?(lhenry) → needinfo?(bas)
Assignee

Comment 23

2 months ago

Filed 1539118.

Flags: needinfo?(bas)
You need to log in before you can comment on or make changes to this bug.