Open Bug 1581475 Opened 7 months ago Updated 2 days ago

Crash in [@ mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern] in void mozilla::SVGGeometryFrame::Render

Categories

(Core :: Graphics, defect, P1)

69 Branch
All
Windows
defect

Tracking

()

Tracking Status
firefox-esr68 --- affected
firefox73 --- wontfix
firefox74 + wontfix
firefox75 --- wontfix
firefox76 --- affected

People

(Reporter: nical, Assigned: nical)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files)

+++ This bug was initially created as a clone of Bug #1576372 +++

This bug is for crash report bp-118eaf84-134c-4c37-bd38-4dc1e0190914

Top 10 frames of crashing thread:

0 xul.dll struct already_AddRefed<ID2D1Brush> mozilla::gfx::DrawTargetD2D1::CreateBrushForPattern gfx/2d/DrawTargetD2D1.cpp
1 xul.dll void mozilla::gfx::DrawTargetD2D1::Fill gfx/2d/DrawTargetD2D1.cpp:674
2 xul.dll void mozilla::SVGGeometryFrame::Render layout/svg/SVGGeometryFrame.cpp:702
3 xul.dll void mozilla::SVGGeometryFrame::PaintSVG layout/svg/SVGGeometryFrame.cpp:264
4 xul.dll void nsDisplaySVGGeometry::Paint layout/svg/SVGGeometryFrame.cpp:120
5 xul.dll void mozilla::FrameLayerBuilder::PaintItems layout/painting/FrameLayerBuilder.cpp:7143
6 xul.dll mozilla::FrameLayerBuilder::DrawPaintedLayer layout/painting/FrameLayerBuilder.cpp:7303
7 xul.dll void mozilla::layers::BasicPaintedLayer::PaintThebes gfx/layers/basic/BasicPaintedLayer.cpp:92
8 xul.dll void mozilla::layers::BasicLayerManager::PaintSelfOrChildren gfx/layers/basic/BasicLayerManager.cpp:700
9 xul.dll void mozilla::layers::BasicLayerManager::PaintLayer gfx/layers/basic/BasicLayerManager.cpp

There are still some crashes with this signature on nightly 71 after the fix of bug 1576372. Looks like these are null dereferences in our code rather than something bad happening in D2D.

:nical, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit auto_nag documentation.

Flags: needinfo?(nical.bugzilla)

It would be misleading to consider that bug 1562091 is the regressing bug since it didn't cause the bug. It led us to (more often) take a different code path with a pre-existing bug which has probably been here for much longer.

Flags: needinfo?(nical.bugzilla)

This is still showing up in the topcrash list for 73.0 (and had a couple thousand reports in 72 as well). Can we find someone to take another look at this?

Flags: needinfo?(jbonisteel)

Will see what we can do.

The crash reports all have allocation failures in their gfx critical log indicating that the best we can do is bail out without crashing and hope that enough memory will be freed before we run into an infallible allocation.

Flags: needinfo?(jbonisteel) → needinfo?(nical.bugzilla)
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8dbb70090704
Add missing null-check in CreateBrudhForPattern. r=jrmuizel

Let's see if this patch catches the null deref. That's the only one I saw but without a way to reproduce the crash it's hard to be sure.

Flags: needinfo?(nical.bugzilla)
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75

We got this report from a Nightly build containing the changeset from comment 8. Does that show anything interesting?
https://crash-stats.mozilla.org/report/index/84e791eb-2049-45db-ad70-d9ee80200219

Flags: needinfo?(nical.bugzilla)

Ok that wasn't it, let's keep looking.

Status: RESOLVED → REOPENED
Flags: needinfo?(nical.bugzilla)
Resolution: FIXED → ---
Attachment #9127840 - Attachment description: Bug 1581475 - Check that the D2D DrawTarget always have a layer. r=jrmuizel → Bug 1581475 - Check that the D2D DrawTarget always has a layer. r=jrmuizel
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36ac2b36ca0b
Check that the D2D DrawTarget always has a layer. r=jrmuizel

That last crash report had a stack where it was pretty obvious that the pattern in CreateBrushFromPattern was a color pattern, which is interesting becaue it limits the possible code paths to afew parts of the function. Towards teh beginning it calls into something that calls CUrrentLayer which silently relies on a later being on the layer stack. I added a check, let's see if this one does mitigate the issue.

If this was the issue, the patch will likely move debug into another tsack, and have release builds silently paper over it by never popping the last layer one stack stack (since it is invalid to not have a layer in the D2D draw target).

Status: REOPENED → RESOLVED
Closed: 1 month ago1 month ago
Resolution: --- → FIXED

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Priority: P3 → P1

No crash in nightly since this landed, do we want it on beta?

Flags: needinfo?(nical.bugzilla)

It papers over rather than fixing a bug but in my opinion it's low risk enough to uplift if the crash volume is worrying.

Flags: needinfo?(nical.bugzilla)

Comment on attachment 9127840 [details]
Bug 1581475 - Check that the D2D DrawTarget always has a layer. r=jrmuizel

Beta/Release Uplift Approval Request

  • User impact if declined: Crashes.
  • Is this code covered by automated tests?: No
  • 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): Low risk, simple patch.
    This doesn't fix the problem of unbalanced Push/PopLayer (just avoids a cathegory of issues occuring as a result of the problem) so it may move some of the crashes of this bug into other signatures.
  • String changes made/needed: None.
Attachment #9127840 - Flags: approval-mozilla-beta?
Attachment #9127110 - Flags: approval-mozilla-beta?

Comment on attachment 9127840 [details]
Bug 1581475 - Check that the D2D DrawTarget always has a layer. r=jrmuizel

P1, medium crasher on beta, , uplift approved for 74.0b8, thanks.

Attachment #9127840 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9127110 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Unfortunately, the crash volume on release 74 looks pretty much unchanged from release 73 :(

Status: RESOLVED → REOPENED
Flags: needinfo?(nical.bugzilla)
Resolution: FIXED → ---
Target Milestone: mozilla75 → mozilla76
Status: REOPENED → NEW
Target Milestone: mozilla76 → ---
Flags: needinfo?(nical.bugzilla)
You need to log in before you can comment on or make changes to this bug.