1627616 - Crash in [@ mozilla::gfx::SourceSurfaceOffset::GetUnderlyingSurface]

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Gabriele Svelto [:gsvelto]]

This bug is for crash report bp-93789af8-def6-48da-8806-7149c0200405.

Top 10 frames of crashing thread:

0 xul.dll mozilla::gfx::SourceSurfaceOffset::GetUnderlyingSurface gfx/2d/DrawTargetOffset.h:38
1 xul.dll mozilla::gfx::DrawTargetD2D1::GetImageForSurface gfx/2d/DrawTargetD2D1.cpp:2182
2 xul.dll mozilla::gfx::DrawTargetD2D1::PushLayer gfx/2d/DrawTargetD2D1.cpp:1015
3 xul.dll gfxContext::PushGroupForBlendBack gfx/thebes/gfxContext.cpp:666
4 xul.dll static nsSVGIntegrationUtils::PaintMaskAndClipPath layout/svg/nsSVGIntegrationUtils.cpp:1055
5 xul.dll nsDisplayMasksAndClipPaths::PaintAsLayer layout/painting/nsDisplayList.cpp:9895
6 xul.dll mozilla::FrameLayerBuilder::PaintItems layout/painting/FrameLayerBuilder.cpp:7101
7 xul.dll static mozilla::FrameLayerBuilder::DrawPaintedLayer layout/painting/FrameLayerBuilder.cpp:7283
8 xul.dll mozilla::layers::BasicPaintedLayer::PaintThebes gfx/layers/basic/BasicPaintedLayer.cpp:92
9 xul.dll mozilla::layers::BasicLayerManager::PaintSelfOrChildren gfx/layers/basic/BasicLayerManager.cpp:700

Low volume crash that's been around for a while. The oldest crash I found is for 69.0b11. The stacks are deep but consistent across Firefox versions and across different Windows versions.

From the looks of it this seems like a NULL pointer access here so mSurface is likely NULL.

Comment 1

[Miko Mynttinen]

This looks like a grab bag of various graphics problems with device resets, OOM, and driver issues. Most reports have graphics critical errors such as:
0x8007000e: E_OUTOFMEMORY
0x887A0005: DXGI_ERROR_DEVICE_REMOVED
0x8899000C: D2DERR_RECREATE_TARGET

This has come up previously in bug 1135073 and bug 1188006.

Comment 2

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Comment 3

[Wayne Mery (:wsmwk)]

Increased 10x since April. 5x since May 5.
https://crash-stats.mozilla.org/signature/?signature=mozilla%3A%3Agfx%3A%3ASourceSurfaceOffset%3A%3AGetUnderlyingSurface&date=%3E%3D2020-03-13T00%3A28%3A00.000Z&date=%3C2020-06-13T00%3A28%3A00.000Z#graphs

Comment 4

[Wayne Mery (:wsmwk)]

Seems like this must be a regression https://crash-stats.mozilla.org/signature/?signature=mozilla%3A%3Agfx%3A%3ASourceSurfaceOffset%3A%3AGetUnderlyingSurface&date=%3E%3D2020-02-05T00%3A00%3A00.000Z&date=%3C2020-08-05T23%3A59%3A00.000Z#graphs

Also, #6 crash for Thunderbird 78.1.0

Comment 5

[Magnus Melin [:mkmelin]]

May 5 was the 76 release. Could point to bug 1618345 (just a wild guess based on the logs for https://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetOffset.h#38)

Comment 6

[Kris Taeleman (:ktaeleman)]

@Miko: Can you take a look at the more recent crashes and see if you can spot a pattern?

Comment 7

[Miko Mynttinen]

I have a speculative fix for this.

I think the issue might be that DrawTargetOffset::Snapshot() unconditionally creates a snapshot, when the wrapped DrawTarget might be uninitialized or invalid. My patch also adds a null-check at the only call site of this function, which should at least avoid the crash.

Comment 8

[Miko Mynttinen]

Attachment: Bug 1627616 - Avoid wrapping SourceSurfaceOffset around uninitialized SourceSurface r=bas

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9de80fbdf625
Avoid wrapping SourceSurfaceOffset around uninitialized SourceSurface r=bas

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9de80fbdf625

Comment 11

[Miko Mynttinen]

Comment on attachment 9169478 [details]
Bug 1627616 - Avoid wrapping SourceSurfaceOffset around uninitialized SourceSurface r=bas

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, possibly related to device resets or OOM events, when surface creation fails
• 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): The patch adds a null pointer check, which avoids the crash. The resulting rendering might not be completely correct, since the mask surface (inferred from crash report stacktraces) is not available, but this should be preferable and recoverable as opposed to crash.
• String changes made/needed:

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9169478 [details]
Bug 1627616 - Avoid wrapping SourceSurfaceOffset around uninitialized SourceSurface r=bas

approved for 80.0.1

Comment 13

[Wayne Mery (:wsmwk)]

I'd also like to request this for ESR - risen to #5 crash for 78.1.1 https://crash-stats.mozilla.org/topcrashers/?product=Thunderbird&version=78.1.1

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/56e2e58fd9f7

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Hi Miko, do we want this on ESR78? Please nominate if yes :)

Comment 16

[Miko Mynttinen]

Comment on attachment 9169478 [details]
Bug 1627616 - Avoid wrapping SourceSurfaceOffset around uninitialized SourceSurface r=bas

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes one of the top crashes.
• User impact if declined: Crashes, possibly related to device resets or OOM events, when surface creation fails.
• Fix Landed on Version: 80
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch adds a null pointer check, which avoids the crash. The resulting rendering might not be completely correct, since the mask surface (inferred from crash report stacktraces) is not available, but this should be preferable and recoverable as opposed to crash.
• String or UUID changes made by this patch:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9169478 [details]
Bug 1627616 - Avoid wrapping SourceSurfaceOffset around uninitialized SourceSurface r=bas

Approved for 78.4esr.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/ae6ad218f89f

Comment 19

[Wayne Mery (:wsmwk)]

I can confirm this is gone in thunderbird 78.4.0