Closed Bug 1524554 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox66 + fixed
firefox67 --- fixed

People

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

References

Details

Crash Data

Attachments

(3 files)

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.

See Also: → 1521368

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]
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

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+
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Flags: qe-verify-
See Also: → 1526045

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
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
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED

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

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]
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

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)

(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)

Filed 1539118.

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