1036419 - Add lock to GrallocReporter::CollectReports()

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

In Bug 1034294, SharedBufferManagerParent's lock problems are fixed except GrallocReporter::CollectReports().

To fix Bug 1034294 quickly, the fix of GrallocReporter::CollectReports() was put off. This bug fixes remaining GrallocReporter::CollectReports().

See Bug 1034294 Comment 19.

Comment 1

[Sotaro Ikeda [:sotaro]]

GrallocReporter::CollectReports() does not have a problem on b2g-v2.0. It need to be fixed only on master.

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add lock to GrallocReporter::CollectReports()

Comment 3

[Sotaro Ikeda [:sotaro]]

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

Comment 4

[Sotaro Ikeda [:sotaro]]

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

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 8453076 [details] [diff] [review]
patch - Add lock to GrallocReporter::CollectReports()

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

::: gfx/layers/ipc/SharedBufferManagerParent.cpp
@@ +43,5 @@
>    NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,
>                              nsISupports* aData, bool aAnonymize)
>    {
> +    if (SharedBufferManagerParent::sManagerMonitor) {
> +      MonitorAutoLock lock(*SharedBufferManagerParent::sManagerMonitor.get());

What is this doing (there is nothing in the scope of this branch to lock)?

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #5)
> Comment on attachment 8453076 [details] [diff] [review]
> patch - Add lock to GrallocReporter::CollectReports()
> 
> Review of attachment 8453076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/SharedBufferManagerParent.cpp
> @@ +43,5 @@
> >    NS_IMETHOD CollectReports(nsIHandleReportCallback* aHandleReport,
> >                              nsISupports* aData, bool aAnonymize)
> >    {
> > +    if (SharedBufferManagerParent::sManagerMonitor) {
> > +      MonitorAutoLock lock(*SharedBufferManagerParent::sManagerMonitor.get());
> 
> What is this doing (there is nothing in the scope of this branch to lock)?

Oh, I do not know why I wrote such code... I just wanted to handle the case that static was already deleted because of xpcom shutdown. I am going to update the patch.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch ver 2 - Add lock to GrallocReporter::CollectReports()

Comment 8

[Sotaro Ikeda [:sotaro]]

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

Comment 9

[Sotaro Ikeda [:sotaro]]

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

Comment 10

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25e80aa9ebd9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25e80aa9ebd9