1034294 - Fix SharedBufferManagerParent

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #1031527 +++

This bug is created from Bug 1031527 Comment 29. SharedBufferManagerParent has following problems.

- all SharedBufferManagerParent's static members access are not protected by Monitor.
    + SharedBufferManagerParent creates one thread for each content process. 
- Two SharedBufferManagerParent instances are created for each content process.
    + One instance is created by SharedBufferManagerParent::CloneToplevel(),
       another is created by ContentParent::AllocPSharedBufferManagerParent().
    + In ImageBridge case, only one instance seem to be created by ImageBridgeParent::CloneToplevel().

Comment 1

[Sotaro Ikeda [:sotaro]]

Nominate to b2g-v2.0+. This bug blocks 2.0+ bug.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix SharedBufferManagerParent

Comment 3

[Sotaro Ikeda [:sotaro]]

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

Comment 4

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8450548 [details] [diff] [review]
patch - Fix SharedBufferManagerParent

I am going to update a bit more.

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 8450548 [details] [diff] [review]
patch - Fix SharedBufferManagerParent

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

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +102,2 @@
>      sManagerMonitor = new Monitor("Manager Monitor");
> +  }

Thanks!

@@ +110,5 @@
> +
> +  NS_ASSERTION(sManagers.count(aOwner) == 0 , "SharedBufferManagerParent already exists.");
> +  if (sManagers.count(aOwner) != 0) {
> +    printf_stderr("SharedBufferManagerParent already exists.");
> +  }

Pick one of the two (either NS_ASSERTION ot printf_stderr)

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver2 - Fix SharedBufferManagerParent

Fix some lock protections and add some error prints.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> Created attachment 8451006 [details] [diff] [review]
> patch ver2 - Fix SharedBufferManagerParent
> 
> Fix some lock protections and add some error prints.

During creating the code, I recognized that the current SharedBufferManagerParent::RecvAllocateGrallocBuffer() does not handle the race condition of accessing sBufferKey. Each SharedBufferManagerParent runs on a different thread, therefore the RecvAllocateGrallocBuffer() is called on different thread for each instance.

The sBufferKey is incremented and accessed without lock. Therefore, if there is a race condition, a gralloc buffer could be stored to incorrect key.

Comment 8

[Sotaro Ikeda [:sotaro]]

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

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> (In reply to Sotaro Ikeda [:sotaro] from comment #6)
> > Created attachment 8451006 [details] [diff] [review]
> > patch ver2 - Fix SharedBufferManagerParent
> > 
> > Fix some lock protections and add some error prints.
> 
> During creating the code, I recognized that the current
> SharedBufferManagerParent::RecvAllocateGrallocBuffer() does not handle the
> race condition of accessing sBufferKey. Each SharedBufferManagerParent runs
> on a different thread, therefore the RecvAllocateGrallocBuffer() is called
> on different thread for each instance.
> 
> The sBufferKey is incremented and accessed without lock. Therefore, if there
> is a race condition, a gralloc buffer could be stored to incorrect key.

This could cause bug 1031527.

Comment 10

[Sotaro Ikeda [:sotaro]]

> 
> This could cause bug 1031527.

And this causes memory leak.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2b9cec4663de
https://hg.mozilla.org/mozilla-central/rev/8cdd45218dcb

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Tim Taubert [:ttaubert] (away July 7th-18th) from comment #11)
> https://hg.mozilla.org/mozilla-central/rev/2b9cec4663de
> https://hg.mozilla.org/mozilla-central/rev/8cdd45218dcb

The above is for Bug 1033293. I chcked-in and backed out, because of wrong commit message.

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 8451006 [details] [diff] [review]
patch ver2 - Fix SharedBufferManagerParent

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

Looks good.

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +194,5 @@
>  
> +  int64_t bufferKey;
> +  {
> +    MonitorAutoLock lock(*sManagerMonitor.get());
> +    bufferKey = ++sBufferKey; 

nit: trailing space

Comment 14

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/476485e3e30a

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver3 for b2g v2.0 - Fix SharedBufferManagerParent

A patch for b2g-v2.0.

Comment 16

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

Backed out for b2g mochitest-8 failures in https://hg.mozilla.org/integration/mozilla-inbound/rev/75022b09e69c
https://tbpl.mozilla.org/php/getParsedLog.php?id=43262773&tree=Mozilla-Inbound

I suspect (though haven't bisected the failure completely yet) that this is responsible for the b2g mochitest-debug-12 failure as well: https://tbpl.mozilla.org/php/getParsedLog.php?id=43261472&tree=Mozilla-Inbound

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Wes Kocher (:KWierso) from comment #16)
> Backed out for b2g mochitest-8 failures in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/75022b09e69c
> https://tbpl.mozilla.org/php/getParsedLog.php?id=43262773&tree=Mozilla-
> Inbound
> 
> I suspect (though haven't bisected the failure completely yet) that this is
> responsible for the b2g mochitest-debug-12 failure as well:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=43261472&tree=Mozilla-
> Inbound

Yeah, the patch seems to make the failure. I am going to investigate about it.

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> (In reply to Wes Kocher (:KWierso) from comment #16)
> > Backed out for b2g mochitest-8 failures in
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/75022b09e69c
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=43262773&tree=Mozilla-
> > Inbound
> > 
> > I suspect (though haven't bisected the failure completely yet) that this is
> > responsible for the b2g mochitest-debug-12 failure as well:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=43261472&tree=Mozilla-
> > Inbound
> 
> Yeah, the patch seems to make the failure. I am going to investigate about
> it.

In the above log, there is no direct information about the test failure. The test failure happened during XPCOM shutdown. From it, I suspect that GrallocReporter::CollectReports() might cause the problem. The function is only function that might be called during XPCOM shutdown.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver3 - Fix SharedBufferManagerParent

As a workaround, remove lock from GrallocReporter::CollectReports(). It is going to again by a different bug.

Comment 20

[Sotaro Ikeda [:sotaro]]

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

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #20)
> https://tbpl.mozilla.org/?tree=Try&rev=f3cd119e7c3f

Test results become good :-)

Comment 22

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8452286 [details] [diff] [review]
patch ver3 - Fix SharedBufferManagerParent

Carry "r=nical".

Comment 23

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f74675fb5620

Comment 24

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

@sotaro: 

As discussed in IRC , our test team is testing patch from #comment 15 on FFOS 2.0 for https://bugzilla.mozilla.org/show_bug.cgi?id=1031527#c31

Comment 25

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

https://hg.mozilla.org/mozilla-central/rev/f74675fb5620

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/751bfe1f5fb3

Comment 27

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.026
Moz BuildID: 20140707000200
B2G Version: 2.0
Gecko Version: 32.0a2
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=ef67af27dff3130d41a9139d6ae7ed640c34d922
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=d3eae03cdf4e6944e646d05938a22dc1380a0d95

Comment 28

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8610
Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.029
Moz BuildID: 20140710000201
B2G Version: 2.0
Gecko Version: 32.0a2
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=35a9b715e7348ec738ff6c8a59f50190390a06f2
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=2fb60c777d3f82d580cba249e5e01a167a01de39

Comment 29

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

http://hg.mozilla.org/releases/mozilla-aurora/log?rev=94714370dfc31fd0a35114843555eae5c5f21be9 contains 751bfe1f5fb3 ( http://hg.mozilla.org/releases/mozilla-aurora/graph/201034 ) 

Reopening based on caf bot comment 28

Comment 30

[Sotaro Ikeda [:sotaro]]

(In reply to cafbot (PoC: ggrisco) from comment #28)
> Observed on: 
> 
> Device: msm8610
> Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.029
> Moz BuildID: 20140710000201
> B2G Version: 2.0
> Gecko Version: 32.0a2
> Gaia: 
> http://git.mozilla.org/?p=releases/gaia.git;a=commit;
> h=35a9b715e7348ec738ff6c8a59f50190390a06f2
> Gecko:
> http://git.mozilla.org/?p=releases/gecko.git;a=commit;
> h=2fb60c777d3f82d580cba249e5e01a167a01de39

ggrisco, can we have the actual crash log and logcat?

Comment 31

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: logs

(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> (In reply to cafbot (PoC: ggrisco) from comment #28)
> > Observed on: 
> > 
> > Device: msm8610
> > Gonk Version: AU_LINUX_GECKO_B2G_KK_3.6.01.04.00.000.029
> > Moz BuildID: 20140710000201
> > B2G Version: 2.0
> > Gecko Version: 32.0a2
> > Gaia: 
> > http://git.mozilla.org/?p=releases/gaia.git;a=commit;
> > h=35a9b715e7348ec738ff6c8a59f50190390a06f2
> > Gecko:
> > http://git.mozilla.org/?p=releases/gecko.git;a=commit;
> > h=2fb60c777d3f82d580cba249e5e01a167a01de39
> 
> ggrisco, can we have the actual crash log and logcat

I attached logs here.

Comment 32

[Sotaro Ikeda [:sotaro]]

From attachment 8455590 [details], this seems a different problem. From the log, it failed to map ion.

---------------------------------------------------------
07-14 16:25:00.210  8506  8536 E qdmemalloc: ion: Failed to map memory in the client: Invalid argument
07-14 16:25:00.210  8506  8536 E qdgralloc: Could not mmap handle 0xb0c55c40, fd=45 (Invalid argument)
07-14 16:25:00.210  8506  8536 E qdgralloc: gralloc_register_buffer: gralloc_map failed
07-14 16:25:00.210  8506  8536 W GraphicBufferMapper: registerBuffer(0xb0c55c40) failed -22 (Invalid argument)
07-14 16:25:00.210  8506  8536 E GraphicBuffer: unflatten: registerBuffer failed: Invalid argument (-22)
07-14 16:25:00.210  8506  8536 I Gecko   : ParamTraits<MagicGrallocBufferHandle>::Read() failed to get gralloc buffer
07-14 16:25:00.220  8506  8536 I Gecko   : IPDL protocol error: Error deserializing 'MaybeMagicGrallocBufferHandle'
07-14 16:25:00.220  8506  8536 I Gecko   : [Child 8506] ###!!! ABORT: IPDL error [PSharedBufferManagerChild]: "Error deserializing 'MaybeMagicGrallocBufferHandle'". abort()ing as a result.: file ../../../../../../../../gecko/ipc/glue/ProtocolUtils.cpp, line 198
07-14 16:25:00.230   232   910 W Adreno-GSL: <gsl_ldd_control:412>: ioctl fd 141 code 0xc02c093d (IOCTL_KGSL_SUBMIT_COMMANDS) failed: errno 22 Invalid argument
07-14 16:25:00.230  8506  8536 E Gecko   : mozalloc_abort: [Child 8506] ###!!! ABORT: IPDL error [PSharedBufferManagerChild]: "Error deserializing 'MaybeMagicGrallocBufferHandle'". abort()ing as a result.: file ../../../../../../../../gecko/ipc/glue/ProtocolUtils.cpp, line 198




By the way, attachment 8455590 [details] contains the following log. It is a false alarm. It is going to be suppressed by Bug 1036561.

> SharedBufferManagerChild::GetGraphicBuffer -- invalid key

Comment 33

[Sotaro Ikeda [:sotaro]]

From comment 32, it seems better to create a new bug for the crash. This bug is for fixing SharedBufferManagerParent's mutex handling.

Comment 34

[Sotaro Ikeda [:sotaro]]

Tapas, do you agree to create a new bug for attachment 8455590 [details]?

Comment 35

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

@Sotaro new bug id : https://bugzilla.mozilla.org/show_bug.cgi?id=1038461

Comment 36

[Sotaro Ikeda [:sotaro]]

Set this bug to fixed. The new crash is handled by bug 1038461.