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

RESOLVED FIXED in Firefox 39, Firefox OS v2.2

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: pyang, Assigned: jerry)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla39
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Depends on: 1034485, 1121812
(Reporter)

Comment 1

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

Another crash in same module.

Comment 2

3 years ago
Jerry, please help to check this crash.

Updated

3 years ago
Flags: needinfo?(hshih)
(Assignee)

Updated

3 years ago
Assignee: nobody → hshih
Flags: needinfo?(hshih)
(Assignee)

Comment 3

3 years ago
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
(Assignee)

Comment 4

3 years ago
Created attachment 8582325 [details] [diff] [review]
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.
Attachment #8582325 - Flags: review?(sotaro.ikeda.g)
Attachment #8582325 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 5

3 years ago
Created attachment 8582326 [details] [diff] [review]
Part2: add more checking rules for GrallocBuffer allocation.
Attachment #8582326 - Flags: review?(sotaro.ikeda.g)
Attachment #8582326 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Updated

3 years ago
Component: Stability → Graphics: Layers
Product: Firefox OS → Core
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.
Attachment #8582325 - Flags: review?(sotaro.ikeda.g) → review+

Updated

3 years ago
Attachment #8582326 - Flags: review?(sotaro.ikeda.g) → review+
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())"?
Attachment #8582326 - Flags: review+

Updated

3 years ago
Flags: needinfo?(hshih)
(Assignee)

Comment 10

3 years ago
Created attachment 8582813 [details] [diff] [review]
Part2: add more checking rules for GrallocBuffer allocation.
Attachment #8582326 - Attachment is obsolete: true
Attachment #8582326 - Flags: review?(nical.bugzilla)
Attachment #8582813 - Flags: review?(sotaro.ikeda.g)
Attachment #8582813 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 11

3 years ago
(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!
Flags: needinfo?(hshih)
Comment on attachment 8582813 [details] [diff] [review]
Part2: add more checking rules for GrallocBuffer allocation.

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

Looks good.
Attachment #8582813 - Flags: review?(sotaro.ikeda.g) → review+

Updated

3 years ago
Attachment #8582325 - Flags: review?(nical.bugzilla) → review+

Updated

3 years ago
Attachment #8582813 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Comment 13

3 years ago
The try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84b0f4445304
Waiting some re-triggered tests.
Status: NEW → ASSIGNED
(Assignee)

Comment 14

3 years ago
(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.
Keywords: checkin-needed
Set 2.2 blocking since it was crashed with v2.2 pre-release.
blocking-b2g: --- → 2.2+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c08aa7c7142
https://hg.mozilla.org/integration/mozilla-inbound/rev/03f1dc776f27
Keywords: checkin-needed
(Assignee)

Comment 17

3 years ago
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
Attachment #8582325 - Flags: approval-mozilla-b2g37?
(Assignee)

Comment 18

3 years ago
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
Attachment #8582813 - Flags: approval-mozilla-b2g37?
https://hg.mozilla.org/mozilla-central/rev/6c08aa7c7142
https://hg.mozilla.org/mozilla-central/rev/03f1dc776f27
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Updated

3 years ago
Attachment #8582325 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Attachment #8582813 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5867b04ed5b3
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/77418148c44b
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
status-firefox37: --- → wontfix
status-firefox38: --- → wontfix
(Reporter)

Updated

2 years ago
Blocks: 1034485, 1121812
No longer depends on: 1034485, 1121812
You need to log in before you can comment on or make changes to this bug.