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:
---
Dependency tree / graph

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

Comment 3

4 years ago
(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:
Assignee

Comment 4

4 years ago
Implementation of SharedBufferManagerParent::GetGraphicBuffer() seems to have a problem.
It does not handle a case that SharedBufferManagerParent of mOwner is already destroyed.
Assignee

Updated

4 years ago
Assignee: nobody → sotaro.ikeda.g
Assignee

Comment 5

4 years ago
Posted patch patch - Add nullptr check (obsolete) — Splinter Review
Assignee

Updated

4 years ago
Attachment #8605408 - Flags: review?(nical.bugzilla)
Assignee

Comment 6

4 years ago
(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+
Assignee

Comment 12

4 years ago
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+
Assignee

Comment 14

4 years ago
> 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.
Assignee

Comment 15

4 years ago
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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee

Updated

4 years ago
Assignee

Comment 18

4 years ago
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.