1164513 - Crash in GetGraphicBuffer while stability testing

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Greg Grisco]

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

Comment 1

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment -

Comment 2

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump -

Comment 3

[Sotaro Ikeda [:sotaro]]

(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:

Comment 4

[Sotaro Ikeda [:sotaro]]

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

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add nullptr check

Comment 6

[Sotaro Ikeda [:sotaro]]

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

Comment 7

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8909
Gonk Version: AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.154
Moz BuildID: 20150511002500
Manifest: https://www.codeaurora.org/cgit/quic/lf/b2g/manifest/tree/caf_AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.154.xml?h=release
B2G Version: v2.2
Gecko Version: 37.0
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=528ef60e7cda09ad43478065f5d33bda398fbeb7
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=f7e168e362809afa274c2ecb9d346aeda90aa519
Patches: bug 1057091, bug 1162663, bug 1157029, bug 1155797, bug 1133147, bug 1156063, bug 1152439, bug 1156503, bug 1159434

Comment 8

[cafbot (PoC: ggrisco)]

Attachment: EXTRA file attachment - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.154

Comment 9

[cafbot (PoC: ggrisco)]

Attachment: decoded minidump - AU_LINUX_GECKO_LF.BR.1.2.3.00.00.00.000.154

Comment 10

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab97d4944cd6

Comment 11

[howie [:howie]]

Triage:blocking

Comment 12

[Sotaro Ikeda [:sotaro]]

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.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 14

[Sotaro Ikeda [:sotaro]]

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

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add nullptr check

Apply the comment. Carry "r=milan".

Comment 16

[Sotaro Ikeda [:sotaro]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/16767d529dfe

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/16767d529dfe

Comment 18

[Sotaro Ikeda [:sotaro]]

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.

Comment 19

[Greg Grisco]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/da5919916ba7