Crash in mozilla::detail::MutexImpl::~MutexImpl | mozilla::gfx::SourceSurfaceSkia::~SourceSurfaceSkia

RESOLVED FIXED in Firefox 59

Status

()

defect
P3
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cyu, Assigned: jnicol)

Tracking

(Blocks 1 bug, {crash})

Trunk
mozilla60
x86
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 wontfix, firefox59 fixed, firefox60 fixed)

Details

(Whiteboard: [gfx-noted], crash signature)

Attachments

(1 attachment)

This bug was filed from the Socorro interface and is
report bp-c2627541-6fee-4de9-a5a9-8730e0180114.
=============================================================

Top 10 frames of crashing thread:

0 libmozglue.so mozilla::detail::MutexImpl::~MutexImpl mozglue/misc/Mutex_posix.cpp:67
1 libxul.so mozilla::gfx::SourceSurfaceSkia::~SourceSurfaceSkia xpcom/threads/Mutex.h:59
2 libxul.so mozilla::gfx::SourceSurfaceSkia::~SourceSurfaceSkia gfx/2d/SourceSurfaceSkia.cpp:28
3 libxul.so mozilla::detail::RefCounted<mozilla::gfx::DrawTarget, mozilla::detail::RefCountAtomicity::AtomicRefCount>::Release 
4 libxul.so RefPtr<mozilla::gfx::DataSourceSurface>::~RefPtr 
5 libxul.so mozilla::gfx::DrawTargetSkia::~DrawTargetSkia gfx/2d/DrawTargetSkia.cpp:311
6 libxul.so mozilla::gfx::DrawTargetSkia::~DrawTargetSkia gfx/2d/DrawTargetSkia.cpp:293
7 libxul.so mozilla::detail::RefCounted<mozilla::gfx::DrawTarget, mozilla::detail::RefCountAtomicity::AtomicRefCount>::Release 
8 libxul.so RefPtr<mozilla::gfx::DataSourceSurface>::assign_assuming_AddRef mfbt/RefPtr.h:41
9 libxul.so mozilla::layers::PersistentBufferProviderBasic::Destroy mfbt/RefPtr.h:168

=============================================================

4 crashes out of 2 installations on Android nightly build 20180112100100. MOZ_Crash() in failure in pthread_mutex_destroy().
Any ideas, Lee? Thanks.
Flags: needinfo?(lsalzman)
This appears to be fallout from recent changes Bas did to add mutexing to SourceSurfaceSkia for OMTP. That is to say, that code never had mutexes before, but now it does. We are not currently using OMTP on Android, so technically these mutexes are not necessary there. But Bas recommended we track this down in case we ever do want to enable OMTP on Android rather than merely ifdefing out the mutexing on Android.

The crash is happening down in the posix MutexImpl code, because the pthread_mutex_destroy(&platformData()->ptMutex) call is failing. Note that &platformData()->ptMutex is essentially inline data in the Mutex, which is itself inline data in the SourceSurfaceSkia. So we'd expect that address to be valid if the SourceSurfaceSkia address was valid, and the converse, it would be invalid if the SourceSurfaceSkia address was invalid.

So the question is whether it is failing because the SourceSurfaceSkia is bogus, or because of some unfortunate underlying library issue with pthreads on the particular Android installs in these reports?

Jamie, can you look into this?
Flags: needinfo?(lsalzman) → needinfo?(jnicol)
Priority: -- → P3
Whiteboard: [gfx-noted]
Blocks: omtp
Also possible is that it is failing because the mutex is still locked.
Assignee: nobody → jnicol
Flags: needinfo?(jnicol)
All the reports have the following errors:

> [0][GFX1]: Failed making Skia raster image for GPU surface (t=4.30355)
> [1][GFX1]: Failed accessing pixels for Skia raster image (t=4.30368)

meaning SourceSurfaceSkia::GetData() is failing. I see that SourceSurfaceSkia::Map doesn't unlock the mutex if GetData fails. I don't know the calling code so don't know whether this is intended, but it looks pretty suspicious to me.
Comment on attachment 8943670 [details]
Bug 1430550 - Don't keep mutex locked if SourceSurfaceSkia::Map fails.

https://reviewboard.mozilla.org/r/214058/#review219782

Shouldn't we check if mIsMapped before Unlocking() in Unmap() to avoid calling Unlock() twice and potentially erroring?
Comment on attachment 8943670 [details]
Bug 1430550 - Don't keep mutex locked if SourceSurfaceSkia::Map fails.

https://reviewboard.mozilla.org/r/214058/#review219782

Hmm, I guess the calling code is not supposed to call Unmap() if Map() returns false, so arguably Unmap() should be safe as is.
Comment on attachment 8943670 [details]
Bug 1430550 - Don't keep mutex locked if SourceSurfaceSkia::Map fails.

https://reviewboard.mozilla.org/r/214058/#review219782

Yeah, the calling code should never call Unmap if Map failed. We assert in debug builds that it doesn't.
Comment on attachment 8943670 [details]
Bug 1430550 - Don't keep mutex locked if SourceSurfaceSkia::Map fails.

https://reviewboard.mozilla.org/r/214058/#review220002
Attachment #8943670 - Flags: review?(bas) → review+
Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47f0db7bf34d
Don't keep mutex locked if SourceSurfaceSkia::Map fails. r=bas
https://hg.mozilla.org/mozilla-central/rev/47f0db7bf34d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Remember to uplift this to 59, since the SourceSurfaceSkia regression ultimately started then.
Comment on attachment 8943670 [details]
Bug 1430550 - Don't keep mutex locked if SourceSurfaceSkia::Map fails.

Thanks Lee.

Approval Request Comment
[Feature/Bug causing the regression]: Off main thread painting
[User impact if declined]: Crashes on Android
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple change, unlocks a mutex on failure path
[String changes made/needed]: None
Attachment #8943670 - Flags: approval-mozilla-beta?
Comment on attachment 8943670 [details]
Bug 1430550 - Don't keep mutex locked if SourceSurfaceSkia::Map fails.

Should help avoid a crash for fennec users. Let's take this for the 59 beta 4 build.
Attachment #8943670 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.