Crash in GetGraphicBuffer while stability testing

RESOLVED FIXED in Firefox 41

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ggrisco, Assigned: sotaro)

Tracking

({crash})

unspecified
mozilla41
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

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

Details

(Whiteboard: [b2g-crash][caf-crash 505][caf priority: p1][CR 837243], crash signature)

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Saw this crash signature a couple of times on latest build:

[@ mozilla::OffTheBooksMutex::Lock | mozilla::layers::SharedBufferManagerParent::GetGraphicBuffer | mozilla::layers::SharedBufferManagerParent::GetGraphicBuffer | mozilla::layers::GetGraphicBufferFrom ]

In both instances of this crash, seeing this at the end of the log:

05-12 21:24:54.614   268  2812 E HWComposer: Non-uniform vsync interval: 650007031

cafbot will upload minidump and extra file (with logs).
(In reply to Greg Grisco from comment #0)
> In both instances of this crash, seeing this at the end of the log:
> 
> 05-12 21:24:54.614   268  2812 E HWComposer: Non-uniform vsync interval:
> 650007031
> 
> cafbot will upload minidump and extra file (with logs).

The following log exits before the above log. It seems to related to the problem.

> [Parent 264] WARNING: pipe error (279): Connection reset by peer:
Implementation of SharedBufferManagerParent::GetGraphicBuffer() seems to have a problem.
It does not handle a case that SharedBufferManagerParent of mOwner is already destroyed.
Assignee: nobody → sotaro.ikeda.g
Attachment #8605408 - Flags: review?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> 
> The following log exits before the above log. It seems to related to the
> problem.
> 
> > [Parent 264] WARNING: pipe error (279): Connection reset by peer:

It means, parent side detect IPC connection problem and the connection is disconnected. It typically happens when child process freeze or crash.
Whiteboard: [CR 837243] → [caf priority: p1][CR 837243]
Whiteboard: [caf priority: p1][CR 837243] → [b2g-crash][caf-crash 505][caf priority: p1][CR 837243]
Keywords: crash

Comment 11

4 years ago
Triage:blocking
blocking-b2g: 2.2? → 2.2+
Comment on attachment 8605408 [details] [diff] [review]
patch - Add nullptr check

Milan, can you review the patch? It is easy fix. And nical is PTO until next Tuesday.
Attachment #8605408 - Flags: review?(nical.bugzilla) → review?(milan)
Comment on attachment 8605408 [details] [diff] [review]
patch - Add nullptr check

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

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +363,5 @@
>  android::sp<android::GraphicBuffer>
>  SharedBufferManagerParent::GetGraphicBuffer(GrallocBufferRef aRef)
>  {
>    MonitorAutoLock lock(*sManagerMonitor.get());
> +  if (!GetInstance(aRef.mOwner)) {

This would certainly fix the crash in this code.  I'm assuming calling GetInstance twice (when we succeed) is OK, and there are no performance implications.  Are the callers ready for nullptr?  Just to make sure we don't crash in the caller instead.
Attachment #8605408 - Flags: review?(milan) → review+
> Are the callers ready for nullptr?

I re-checked the code, there is a one place that nullptr check is not exist. The code is used only by layer scope now.
Apply the comment. Carry "r=milan".
Attachment #8605408 - Attachment is obsolete: true
Attachment #8605932 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/16767d529dfe
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8605932 [details] [diff] [review]
patch - Add nullptr check

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 959089
User impact if declined: system boot could happen in some cases. 
Testing completed: locally tested.
Risk to taking this patch (and alternatives if risky): low.
String or UUID changes made by this patch: none.
Attachment #8605932 - Flags: approval-mozilla-b2g37?
(Reporter)

Comment 19

4 years ago
I applied the patch internally and it will be built into AU 157+, thanks.

Updated

4 years ago
Attachment #8605932 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.