1145015 - [MTBF] crash in gfx/layers/opengl/OGLShaderProgram

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Paul Yang [:pyang] (away)]

Crash report: https://crash-stats.mozilla.com/report/index/3230457e-d49b-4aaa-8518-6f89a2150319

STR: running mtbf and set memory at 319 MB

Version info:
Build ID               20150317162504
Gaia Revision          306772a58335ac4cad285d27c3805090a8cc6886
Gaia Date              2015-03-17 17:12:36
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/83251e534b33
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150317.195036
Firmware Date          Tue Mar 17 19:50:45 EDT 2015
Bootloader             L1TC100118D0

Comment 1

[Paul Yang [:pyang] (away)]

https://crash-stats.mozilla.com/report/index/eed5a3d0-4f88-4e65-ae1e-769622150318

Another crash in same module.

Comment 2

[Peter Chang[:pchang]]

Jerry, please help to check this crash.

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I guess that we have memory corruption for mBuffers.

There is a mutex at:
https://hg.mozilla.org/mozilla-central/annotate/e730012260a4/gfx/layers/ipc/SharedBufferManagerChild.cpp#l359
But we don't have mutex at:
https://hg.mozilla.org/mozilla-central/annotate/e730012260a4/gfx/layers/ipc/SharedBufferManagerChild.cpp#l369

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: remove IsValidKey() check in MagicGrallocBufferHandle serializer.

There is a data racing problem in IsValidKey() and we don't need to check the
index in serializer. The index is always invalid when we alloc new buffer.

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: add more checking rules for GrallocBuffer allocation.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8582325 [details] [diff] [review]
Part1: remove IsValidKey() check in MagicGrallocBufferHandle serializer.

Review of attachment 8582325 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ -150,5 @@
> -    if (SharedBufferManagerChild::GetSingleton()->IsValidKey(index)) {
> -      aResult->mGraphicBuffer = SharedBufferManagerChild::GetSingleton()->GetGraphicBuffer(index);
> -    }
> -    MOZ_ASSERT(!aResult->mGraphicBuffer.get());
> -

It's a data racing in IsValidKey(). 
We already have an assert here to assume that the key is always invalid, So I remove all checking here.

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8582326 [details] [diff] [review]
Part2: add more checking rules for GrallocBuffer allocation.

Review of attachment 8582326 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +112,5 @@
>                                              void** aIter, paramType* aResult)
>  {
> +  MOZ_ASSERT(aResult->mGraphicBuffer.get());
> +  MOZ_ASSERT(aResult->mRef.mOwner == 0);
> +  MOZ_ASSERT(aResult->mRef.mKey == -1);

Assume we pass the empty MagicGrallocBufferHandle data into read().

@@ +167,5 @@
>      if (NO_ERROR != flattenable->unflatten(data, nbytes, fds, nfds)) {
>        buffer = nullptr;
>      }
>  #endif
> +    if (buffer.get()) {

We already check the mGraphicBuffer before.

::: gfx/layers/ipc/SharedBufferManagerChild.cpp
@@ +274,5 @@
>    MOZ_ASSERT(aSize.width >= 0 && aSize.height >= 0);
>  
>  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
>    mozilla::layers::MaybeMagicGrallocBufferHandle handle;
> +  if (!SendAllocateGrallocBuffer(aSize, aFormat, aUsage, &handle)) {

Check the ipc return value.

@@ +284,5 @@
>    *aHandle = handle.get_MagicGrallocBufferHandle().mRef;
>  
>    {
>      MutexAutoLock lock(mBufferMutex);
> +    MOZ_ASSERT(mBuffers.count(handle.get_MagicGrallocBufferHandle().mRef.mKey)==0);

The key should be the new one.

Comment 8

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8582325 [details] [diff] [review]
Part1: remove IsValidKey() check in MagicGrallocBufferHandle serializer.

Review of attachment 8582325 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. This code could be removed if sanity check is done in another place.

Comment 9

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8582326 [details] [diff] [review]
Part2: add more checking rules for GrallocBuffer allocation.

Review of attachment 8582326 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +110,5 @@
>  bool
>  ParamTraits<MagicGrallocBufferHandle>::Read(const Message* aMsg,
>                                              void** aIter, paramType* aResult)
>  {
> +  MOZ_ASSERT(aResult->mGraphicBuffer.get());

Isn't it mean "MOZ_ASSERT(!aResult->mGraphicBuffer.get())"?

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: add more checking rules for GrallocBuffer allocation.

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> Comment on attachment 8582326 [details] [diff] [review]
> Part2: add more checking rules for GrallocBuffer allocation.
> 
> Review of attachment 8582326 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
> @@ +110,5 @@
> >  bool
> >  ParamTraits<MagicGrallocBufferHandle>::Read(const Message* aMsg,
> >                                              void** aIter, paramType* aResult)
> >  {
> > +  MOZ_ASSERT(aResult->mGraphicBuffer.get());
> 
> Isn't it mean "MOZ_ASSERT(!aResult->mGraphicBuffer.get())"?

yes, thx!

Comment 12

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8582813 [details] [diff] [review]
Part2: add more checking rules for GrallocBuffer allocation.

Review of attachment 8582813 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

The try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84b0f4445304
Waiting some re-triggered tests.

Comment 14

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #13)
> The try result:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=84b0f4445304
> Waiting some re-triggered tests.

Please land the attachment 8582325 [details] [diff] [review] and attachment 8582813 [details] [diff] [review] to m-c.
I can see a lot of similar failed test my try, so I think that's not related to these patches.

Comment 15

[Peter Chang[:pchang]]

Set 2.2 blocking since it was crashed with v2.2 pre-release.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c08aa7c7142
https://hg.mozilla.org/integration/mozilla-inbound/rev/03f1dc776f27

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8582325 [details] [diff] [review]
Part1: remove IsValidKey() check in MagicGrallocBufferHandle serializer.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined:
We have a data racing problem and b2g might get crash in some timing.
Testing completed: 
Pass the try on m-c. Manual testing on 2.2 and doesn't get crash.
Risk to taking this patch (and alternatives if risky):
Low. These patch remove the data racing code and add more assert checking. 
String or UUID changes made by this patch: none

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8582813 [details] [diff] [review]
Part2: add more checking rules for GrallocBuffer allocation.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined:
We have a data racing problem and b2g might get crash in some timing.
Testing completed: 
Pass the try on m-c. Manual testing on 2.2 and doesn't get crash.
Risk to taking this patch (and alternatives if risky):
Low. These patch remove the data racing code and add more assert checking. 
String or UUID changes made by this patch: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c08aa7c7142
https://hg.mozilla.org/mozilla-central/rev/03f1dc776f27

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5867b04ed5b3
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/77418148c44b