Closed Bug 1713769 Opened 2 years ago Closed 1 year ago

Crash in [@ SkImage::makeShader]

Categories

(Core :: Graphics, defect)

defect

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 94+ fixed
firefox92 --- wontfix
firefox93 --- wontfix
firefox94 --- fixed
firefox95 --- fixed

People

(Reporter: gsvelto, Assigned: lsalzman)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

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:

Many comments specifically point to the first page

Severity: -- → S2

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

Flags: needinfo?(lsalzman)

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.

Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8b350f26920
Avoid null Skia image shader. r=gfx-reviewers,jgilbert
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch
Flags: needinfo?(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:
Attachment #9241608 - Flags: approval-mozilla-beta?

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:
Attachment #9241608 - Flags: approval-mozilla-esr91?

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

Attachment #9241608 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

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?

Flags: needinfo?(lsalzman)

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.

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

Flags: needinfo?(lsalzman)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0651bee52061
Ensure extracting SkImage subset is actually successful. r=gfx-reviewers,jrmuizel
Status: REOPENED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED

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:
Attachment #9246256 - Flags: approval-mozilla-beta?

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

Approved for 94.0b7.

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

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.

Attachment #9241608 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+
Attachment #9246256 - Flags: approval-mozilla-esr91+
Target Milestone: 94 Branch → 95 Branch
You need to log in before you can comment on or make changes to this bug.