1713769 - Crash in [@ SkImage::makeShader]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Crash report: https://crash-stats.mozilla.org/report/index/7d7c8f9e-6c22-4cd6-8af1-d205a0210527

Reason: SIGSEGV /SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so SkImage::makeShader const /build/firefox-BXnEmc/firefox-88.0.1+build1/gfx/skia/skia/src/image/SkImage.cpp:88
1 libxul.so mozilla::gfx::SetPaintPattern /build/firefox-BXnEmc/firefox-88.0.1+build1/gfx/2d/DrawTargetSkia.cpp:569
2 libxul.so mozilla::gfx::DrawTargetSkia::FillRect /build/firefox-BXnEmc/firefox-88.0.1+build1/gfx/2d/DrawTargetSkia.cpp:811
3 libxul.so mozilla::gfx::RecordedFillRect::PlayEvent const /build/firefox-BXnEmc/firefox-88.0.1+build1/gfx/2d/RecordedEventImpl.h:2200
4 libxul.so std::_Function_handler<bool  /usr/include/c++/10/bits/std_function.h:291
5 libxul.so bool mozilla::gfx::RecordedEvent::DoWithEvent<mozilla::gfx::InlineTranslator::TranslateRecording /build/firefox-BXnEmc/firefox-88.0.1+build1/gfx/2d/RecordedEventImpl.h:3912
6 libxul.so mozilla::gfx::InlineTranslator::TranslateRecording /build/firefox-BXnEmc/firefox-88.0.1+build1/gfx/2d/InlineTranslator.cpp:72
7 libxul.so mozilla::gfx::CrossProcessPaint::Start const /build/firefox-BXnEmc/firefox-88.0.1+build1/gfx/ipc/CrossProcessPaint.cpp:252
8 libxul.so mozilla::MozPromise<nsRefCountedHashtable<nsUint64HashKey, RefPtr<mozilla::gfx::RecordedDependentSurface> >, nsresult, true>::ThenValue<mozilla::gfx::CrossProcessPaint::Start /build/firefox-BXnEmc/firefox-88.0.1+build1/obj-x86_64-linux-gnu/dist/include/mozilla/MozPromise.h:846
9 libxul.so mozilla::MozPromise<nsRefCountedHashtable<nsUint64HashKey, RefPtr<mozilla::gfx::RecordedDependentSurface> >, nsresult, true>::ThenValueBase::ResolveOrRejectRunnable::Run /build/firefox-BXnEmc/firefox-88.0.1+build1/obj-x86_64-linux-gnu/dist/include/mozilla/MozPromise.h:487

This is a crash caused by a NULL pointer access with consistent frames across all platforms but very low volume. The crash URLs indicate this is happening on these three pages:

• https://rollercoin.com/game/play_game
• https://app.schul.cloud
• https://support.oculus.com/

Many comments specifically point to the first page

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Hi Lee, this is showing up pretty high on the Nightly topcrash list. Any thoughts on what might be going wrong?

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: Bug 1713769 - Avoid null Skia image shader. r?jrmuizel

If makeShader() gets passed a non-invertible matrix, it can fail and return null.
This can then subsequently cause problems when this is passed to SkPaint which
blindly tries to use it. For other pattern types, we gracefully handle this by
just making the SkPaint transparent, so let's do likewise for surface patterns.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8b350f26920
Avoid null Skia image shader. r=gfx-reviewers,jgilbert

Comment 4

[Atila Butkovits]

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

Comment 5

[Lee Salzman [:lsalzman]]

Comment on attachment 9241608 [details]
Bug 1713769 - Avoid null Skia image shader. r?jrmuizel

Beta/Release Uplift Approval Request

• User impact if declined: Potential crashes on some web and canvas content.
• 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): Just skips rendering in a situation where a null pointer would be dereferenced.
• String changes made/needed:

Comment 6

[Lee Salzman [:lsalzman]]

Comment on attachment 9241608 [details]
Bug 1713769 - Avoid null Skia image shader. r?jrmuizel

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9241608 [details]
Bug 1713769 - Avoid null Skia image shader. r?jrmuizel

It's not a high volume crash and we don't know if it is effective yet as we have no crash on nightly. I think it can ride the trains. I am keeping the status as fix-optional for 93 in case we see in our first 94 betas that the fix works, for a potential uplift in a 93 dot release. Thanks

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Lee, we're still seeing crashes on Beta (and it's a bit hard to determine if the rate is lower than before). Anything interesting standing out to you in the newer reports?

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: Bug 1713769 - Ensure extracting SkImage subset is actually successful.

If ExtractSubset fails on an SkImage, we may accidentally call image->makeShader
with a null image below that. We need to bail out in that case so that we don't
do that.

Comment 10

[Lee Salzman [:lsalzman]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)

Lee, we're still seeing crashes on Beta (and it's a bit hard to determine if the rate is lower than before). Anything interesting standing out to you in the newer reports?

I have one other potential guess at what may be going on here. Patch up.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0651bee52061
Ensure extracting SkImage subset is actually successful. r=gfx-reviewers,jrmuizel

Comment 12

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/0651bee52061

Comment 13

[Lee Salzman [:lsalzman]]

Comment on attachment 9246256 [details]
Bug 1713769 - Ensure extracting SkImage subset is actually successful.

Beta/Release Uplift Approval Request

• User impact if declined: Potential crashing during content or canvas rendering.
• 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): Just adds a null pointer check in an area where we are seeing null dereferences in crash reports and otherwise shouldn't add any risky behavior. Given that this is the only other place I can see it is coming from, that we don't have a definitive testcase/STR yet, and given that this is extremely hard to test in nightly given the low crash volume there, uplifting this to beta where the incidence is higher will let us know if this is really the source of the crashing...
• String changes made/needed:

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9246256 [details]
Bug 1713769 - Ensure extracting SkImage subset is actually successful.

Approved for 94.0b7.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2cae279f297e13ff6bf4b5c272599b411b6e4d55

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9241608 [details]
Bug 1713769 - Avoid null Skia image shader. r?jrmuizel

Looks like the follow-up fix took care of the remaining crashes, thanks! Approved for 91.3esr.

Comment 17

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/2e54c38b1763
https://hg.mozilla.org/releases/mozilla-esr91/rev/0339c44e4a79