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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[Cervantes Yu [:cyu] [:cervantes]]

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

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

Any ideas, Lee? Thanks.

Comment 2

[Lee Salzman [: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?

Comment 3

[Jamie Nicol [:jnicol]]

Also possible is that it is failing because the mutex is still locked.

Comment 4

[Jamie Nicol [: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 5

[Jamie Nicol [:jnicol]]

Attachment: Bug 1430550 - Don't keep mutex locked if SourceSurfaceSkia::Map fails.

This was leading to a mutex never being unlocked, eventually causing a
crash when it was destroyed.

Review commit: https://reviewboard.mozilla.org/r/214058/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214058/

Comment 6

[Lee Salzman [:lsalzman]]

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 7

[Lee Salzman [:lsalzman]]

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 8

[Jamie Nicol [:jnicol]]

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 9

[Bas Schouten (:bas.schouten)]

Comment on attachment 8943670 [details]
Bug 1430550 - Don't keep mutex locked if SourceSurfaceSkia::Map fails.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47f0db7bf34d
Don't keep mutex locked if SourceSurfaceSkia::Map fails. r=bas

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/47f0db7bf34d

Comment 12

[Lee Salzman [:lsalzman]]

Remember to uplift this to 59, since the SourceSurfaceSkia regression ultimately started then.

Comment 13

[Jamie Nicol [:jnicol]]

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

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 15

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a6efeac534b