1011327 - [Flame] FX OS crash in mozilla::layers::SharedBufferManagerParent::RecvDropGrallocBuffer(mozilla::layers::MaybeMagicGrallocBufferHandle const&)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-42a24371-cdcf-42a1-8f9a-d6ec82140515.
=============================================================

Seen while running on Flame device using:

Gaia   3a1d67246a79e3632c3b3f2460a25291e7e2714c
SourceStamp e4843f4f08a7
BuildID 20140515040207
Version 32.0a1

STR:
1. Was swiping through various screens using edge gestures when the crash occurred.

Comment 1

[Andreas Gal :gal]

sotaro is this the missing null check?

Comment 2

[Sotaro Ikeda [:sotaro]]

Yea, it seems mull check for a pointer of GraphicBuffer seems missing. The crash address is 0x34. From the following disassemble info, it seems to related to "buf->getPixelFormat()".


(gdb) disassemble /m
Dump of assembler code for function mozilla::layers::SharedBufferManagerParent::RecvDropGrallocBuffer(mozilla::layers::MaybeMagicGrallocBufferHandle const&):
173	{
   0xb4fdbdc8 <+0>:	push	{r0, r1, r2, r3, r4, lr}
   0xb4fdbdca <+2>:	mov	r4, r0

174	#ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
175	  NS_ASSERTION(handle.type() == MaybeMagicGrallocBufferHandle::TGrallocBufferRef, "We shouldn't interact with the real buffer!");
176	  int bufferKey = handle.get_GrallocBufferRef().mKey;
=> 0xb4fdbdcc <+4>:	ldr	r2, [r1, #4]
   0xb4fdbdd2 <+10>:	str	r2, [sp, #4]

177	  sp<GraphicBuffer> buf = GetGraphicBuffer(bufferKey);
   0xb4fdbdce <+6>:	add	r0, sp, #8
   0xb4fdbdd0 <+8>:	mov	r1, r4
   0xb4fdbdd4 <+12>:	bl	0xb4fdbb68

178	  MutexAutoLock lock(mBuffersMutex);
   0xb4fdbdd8 <+16>:	add.w	r0, r4, #416	; 0x1a0

179	  NS_ASSERTION(mBuffers.count(bufferKey) == 1, "How can you drop others buffer");
180	  mBuffers.erase(bufferKey);
181	
182	  int bpp = 0;
183	  bpp = android::bytesPerPixel(buf->getPixelFormat());
   0xb4fdbdee <+38>:	ldr	r0, [r3, #52]	; 0x34
   0xb4fdbdf0 <+40>:	blx	0xb4c42058
---Type <return> to continue, or q <return> to quit---
   0xb4fdbdf4 <+44>:	ldr	r3, [sp, #8]

184	  if (bpp > 0)
   0xb4fdbdf6 <+46>:	cmp	r0, #0
   0xb4fdbdf8 <+48>:	ble.n	0xb4fdbe08 <mozilla::layers::SharedBufferManagerParent::RecvDropGrallocBuffer(mozilla::layers::MaybeMagicGrallocBufferHandle const&)+64>

185	    GrallocReporter::sAmount -= buf->getStride() * buf->getHeight() * bpp;
   0xb4fdbdfa <+50>:	ldr	r1, [pc, #60]	; (0xb4fdbe38 <mozilla::layers::SharedBufferManagerParent::RecvDropGrallocBuffer(mozilla::layers::MaybeMagicGrallocBufferHandle const&)+112>)
   0xb4fdbdfc <+52>:	ldr	r2, [r3, #44]	; 0x2c
   0xb4fdbdfe <+54>:	add	r1, pc
   0xb4fdbe00 <+56>:	ldr	r3, [r3, #48]	; 0x30
   0xb4fdbe02 <+58>:	muls	r3, r2
   0xb4fdbe04 <+60>:	muls	r0, r3
   0xb4fdbe06 <+62>:	b.n	0xb4fdbe18 <mozilla::layers::SharedBufferManagerParent::RecvDropGrallocBuffer(mozilla::layers::MaybeMagicGrallocBufferHandle const&)+80>

186	  else // Specical case for BSP specific formats(mainly YUV formats, count it as normal YUV buffer)
187	    GrallocReporter::sAmount -= buf->getStride() * buf->getHeight() * 3 / 2;
   0xb4fdbe08 <+64>:	ldr	r1, [pc, #48]	; (0xb4fdbe3c <mozilla::layers::SharedBufferManagerParent::RecvDropGrallocBuffer(mozilla::layers::MaybeMagicGrallocBufferHandle const&)+116>)
   0xb4fdbe0a <+66>:	ldr	r2, [r3, #48]	; 0x30
   0xb4fdbe0c <+68>:	ldr	r0, [r3, #44]	; 0x2c
   0xb4fdbe0e <+70>:	add	r1, pc
   0xb4fdbe10 <+72>:	muls	r0, r2
   0xb4fdbe12 <+74>:	movs	r3, #3
   0xb4fdbe14 <+76>:	muls	r0, r3
---Type <return> to continue, or q <return> to quit---
   0xb4fdbe16 <+78>:	lsrs	r0, r0, #1
   0xb4fdbe18 <+80>:	ldrd	r2, r3, [r1]
   0xb4fdbe1c <+84>:	subs	r2, r2, r0
   0xb4fdbe1e <+86>:	sbc.w	r3, r3, #0
   0xb4fdbe24 <+92>:	strd	r2, r3, [r1]

188	
189	#endif
190	  return true;
   0xb4fdbe22 <+90>:	add	r0, sp, #12
   0xb4fdbe28 <+96>:	bl	0xb4c59c66
   0xb4fdbe2c <+100>:	add	r0, sp, #8
   0xb4fdbe2e <+102>:	bl	0xb4d4ad84

191	}
   0xb4fdbe32 <+106>:	movs	r0, #1
   0xb4fdbe34 <+108>:	add	sp, #16
   0xb4fdbe36 <+110>:	pop	{r4, pc}
   0xb4fdbe38 <+112>:			; <UNDEFINED> instruction: 0x01529196
   0xb4fdbe3c <+116>:	cmpeq	r2, r6, lsl #3

End of assembler dump.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add checks around SharedBufferManagerParent

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Add checks around SharedBufferManagerParent

Update nits.

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 8424038 [details] [diff] [review]
patch v2 - Add checks around SharedBufferManagerParent

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

The assertion below looks wrong, please clarify.

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +194,5 @@
>  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
>    NS_ASSERTION(handle.type() == MaybeMagicGrallocBufferHandle::TGrallocBufferRef, "We shouldn't interact with the real buffer!");
>    int bufferKey = handle.get_GrallocBufferRef().mKey;
>    sp<GraphicBuffer> buf = GetGraphicBuffer(bufferKey);
> +  MOZ_ASSERT(!buf.get());

Did you mean to check MOZ_ASSERT(buf.get()) ?

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #5)
> Comment on attachment 8424038 [details] [diff] [review]
> patch v2 - Add checks around SharedBufferManagerParent
> 
> Review of attachment 8424038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The assertion below looks wrong, please clarify.
> 
> ::: gfx/layers/ipc/SharedBufferManagerParent.cpp
> @@ +194,5 @@
> >  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> >    NS_ASSERTION(handle.type() == MaybeMagicGrallocBufferHandle::TGrallocBufferRef, "We shouldn't interact with the real buffer!");
> >    int bufferKey = handle.get_GrallocBufferRef().mKey;
> >    sp<GraphicBuffer> buf = GetGraphicBuffer(bufferKey);
> > +  MOZ_ASSERT(!buf.get());
> 
> Did you mean to check MOZ_ASSERT(buf.get()) ?

Yes, it's my mistake.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Add checks around SharedBufferManagerParent

Apply the comment.

Comment 8

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=d0c4183203fd

Comment 9

[Sotaro Ikeda [:sotaro]]

 https://hg.mozilla.org/integration/b2g-inbound/rev/182479098f34

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/182479098f34