1170061 - Handle shutdown case for HwcComposer2D

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Boris Chiou [:boris]]

We should call ClearOnShutdown() while creating HwcComposer2D object to make sure we handle the shutdown case properly.

Comment 1

[Boris Chiou [:boris]]

Reference:
https://dxr.mozilla.org/mozilla-central/source/widget/gonk/GeckoTouchDispatcher.cpp#59

Comment 2

[Boris Chiou [:boris]]

Attachment: [WIP] ClearOnShutdown for hwcomposer (v1)

Wait for Bug 1144012

Comment 3

[Boris Chiou [:boris]]

Attachment: ClearOnShutdown for hwcomposer (v2)

Call ClearOnShutdown when new HwcComposer2D object and clear
HwcHAL in the destructor of HwcComposer2D.

Comment 4

[Boris Chiou [:boris]]

Comment on attachment 8613452 [details] [diff] [review]
ClearOnShutdown for hwcomposer (v2)

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

Hi Sotaro,
This patch is based on Bug 1144012 Comment 146. I added ClearOnShutDown() in HwcComposer2D and a new public API in HwcHALBase to nullify the StaticRefPtr, so now we could manually clear StaticRefPtr<HwcHALBase> in HwcComposer2D destructor. However, GonkDisplayJB still holds this ref-counted pointer (nsRefPtr<HwcHALBase>) and no one would clear GonkDisplayJB object. It's a potential bug we have to fix later.

Comment 5

[Boris Chiou [:boris]]

Attachment: ClearOnShutdown for hwcomposer (v3)

Call ClearOnShutdown while creaing HwcComposer2D object.

I think we don't need to clear HwcHAL manually in this bug because HwcHAL will be
destructed automatically according to Bug 1144012.

Comment 6

[Boris Chiou [:boris]]

Attachment: ClearOnShutdown for hwcomposer (v3)

Call ClearOnShutdown while creaing HwcComposer2D object.

I think we don't need to clear HwcHAL manually in this bug because HwcHAL
will be destructed automatically by UniquePtr, according to Bug 1144012.

Comment 7

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8617085 [details] [diff] [review]
ClearOnShutdown for hwcomposer (v3)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +170,5 @@
>  {
>      if (!sInstance) {
>          LOGI("Creating new instance");
>          sInstance = new HwcComposer2D();
> +        ClearOnShutdown(&sInstance);

This should be called on main thread. How is it ensured?

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +170,5 @@
> >  {
> >      if (!sInstance) {
> >          LOGI("Creating new instance");
> >          sInstance = new HwcComposer2D();
> > +        ClearOnShutdown(&sInstance);
> 
> This should be called on main thread. How is it ensured?

And please add it as a comment, since Composer2D api is normally called on Compositor thread.

Comment 9

[Boris Chiou [:boris]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> Comment on attachment 8617085 [details] [diff] [review]
> ClearOnShutdown for hwcomposer (v3)
> 
> Review of attachment 8617085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +170,5 @@
> >  {
> >      if (!sInstance) {
> >          LOGI("Creating new instance");
> >          sInstance = new HwcComposer2D();
> > +        ClearOnShutdown(&sInstance);
> 
> This should be called on main thread. How is it ensured?

Thanks for your reminder. I will add a comment and NS_IsMainThread() to make sure it is called on main thread.

BTW: Flame (with vsync) will call this on main thread according to current implementation:
https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxAndroidPlatform.cpp?from=gfxAndroidPlatform.cpp#457

Comment 10

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #9)
> (In reply to Sotaro Ikeda [:sotaro] from comment #7)
> > Comment on attachment 8617085 [details] [diff] [review]
> > ClearOnShutdown for hwcomposer (v3)
> > 
> > Review of attachment 8617085 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/gonk/HwcComposer2D.cpp
> > @@ +170,5 @@
> > >  {
> > >      if (!sInstance) {
> > >          LOGI("Creating new instance");
> > >          sInstance = new HwcComposer2D();
> > > +        ClearOnShutdown(&sInstance);
> > 
> > This should be called on main thread. How is it ensured?
> 
> Thanks for your reminder. I will add a comment and NS_IsMainThread() to make
> sure it is called on main thread.
> 
> BTW: Flame (with vsync) will call this on main thread according to current
> implementation:
> https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxAndroidPlatform.
> cpp?from=gfxAndroidPlatform.cpp#457

BTW, there is another problem. KillClearOnShutdown() is called in ShutdownXPCOM(), 
(https://dxr.mozilla.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp#884), and I'm not sure whether it is a good place to clear HwcComposer2D in ShutdownXPCOM() or not.

Comment 11

[Boris Chiou [:boris]]

Attachment: ClearOnShutdown for hwcomposer (v4)

Call ClearOnShutdown while creaing HwcComposer2D object.

Comment 12

[Sotaro Ikeda [:sotaro]]

From gfx layer's rendering point of view, it is enough, because gfxPlatform::ShutdownLayersIPC() is already called.

It might better to have a singleton check like MediaManager::Get()
  http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1601

Comment 13

[Boris Chiou [:boris]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> From gfx layer's rendering point of view, it is enough, because
> gfxPlatform::ShutdownLayersIPC() is already called.
> 
> It might better to have a singleton check like MediaManager::Get()
>  
> http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1601

OK. Do you mean that we can write a help checker function, ex. HwcComposer2D::Get(), to check the object status and creation, and put the main thread check in this checker function? Therefore, HwcComposer2D::GetInstance() could be simpler.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Boris Chiou [:boris] from comment #13)
> (In reply to Sotaro Ikeda [:sotaro] from comment #12)
> > From gfx layer's rendering point of view, it is enough, because
> > gfxPlatform::ShutdownLayersIPC() is already called.
> > 
> > It might better to have a singleton check like MediaManager::Get()
> >  
> > http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1601
> 
> OK. Do you mean that we can write a help checker function, ex.
> HwcComposer2D::Get(), to check the object status and creation, and put the
> main thread check in this checker function? Therefore,
> HwcComposer2D::GetInstance() could be simpler.

Sorry, for confusion. I just said to add a code like the following. And 

>    NS_ASSERTION(NS_IsMainThread(),
> #ifdef DEBUG
>    static int timesCreated = 0;
>    timesCreated++;
>    MOZ_ASSERT(timesCreated == 1);
>#endif

And it seems better to handle nsWindow::~nsWindow() case. To handle it, it seems better that the nsWindow holds a reference to HwcComposer2D.
  https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#87

Comment 15

[Boris Chiou [:boris]]

(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> (In reply to Boris Chiou [:boris] from comment #13)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #12)
> > > From gfx layer's rendering point of view, it is enough, because
> > > gfxPlatform::ShutdownLayersIPC() is already called.
> > > 
> > > It might better to have a singleton check like MediaManager::Get()
> > >  
> > > http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1601
> > 
> > OK. Do you mean that we can write a help checker function, ex.
> > HwcComposer2D::Get(), to check the object status and creation, and put the
> > main thread check in this checker function? Therefore,
> > HwcComposer2D::GetInstance() could be simpler.
> 
> Sorry, for confusion. I just said to add a code like the following. And 
> 
> >    NS_ASSERTION(NS_IsMainThread(),
> > #ifdef DEBUG
> >    static int timesCreated = 0;
> >    timesCreated++;
> >    MOZ_ASSERT(timesCreated == 1);
> >#endif
> 
> And it seems better to handle nsWindow::~nsWindow() case. To handle it, it
> seems better that the nsWindow holds a reference to HwcComposer2D.
>   https://dxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#87

Good. Thanks for your advice. I will fix it soon.

Comment 16

[Boris Chiou [:boris]]

Attachment: ClearOnShutdown for hwcomposer (v5)

Call ClearOnShutdown while creaing HwcComposer2D object.

Comment 17

[Boris Chiou [:boris]]

Attachment: ClearOnShutdown for hwcomposer (v6)

Call ClearOnShutdown while creaing HwcComposer2D object.

Comment 18

[Boris Chiou [:boris]]

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

Comment 19

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8622842 [details] [diff] [review]
ClearOnShutdown for hwcomposer (v6)

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

Review+ by addressing the comment.

::: widget/gonk/HwcComposer2D.cpp
@@ +169,5 @@
>  HwcComposer2D::GetInstance()
>  {
>      if (!sInstance) {
> +        // We should make sure only main thread create HwcComposer2D,
> +        // so ClearOnShutdown() can work properly.

Can you add a comment about which code ensures on main thread creation?

Comment 20

[Boris Chiou [:boris]]

Comment on attachment 8622842 [details] [diff] [review]
ClearOnShutdown for hwcomposer (v6)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +169,5 @@
>  HwcComposer2D::GetInstance()
>  {
>      if (!sInstance) {
> +        // We should make sure only main thread create HwcComposer2D,
> +        // so ClearOnShutdown() can work properly.

Actually, I am thinking about this implementation:
if(NS_IsMainThread()) {
  // creation
} else {
  // post a creation task to main thread
}

By this implementation, we can make sure only main thread can create HwcComposer2D, but I'm not sure the post is necessary here.
Do you have any concern if I use the if-else to check main thread and post creation task? It may be safer than using comments. Thanks.

Comment 21

[Boris Chiou [:boris]]

Attachment: ClearOnShutdown for hwcomposer (v7)

Call ClearOnShutdown while creaing HwcComposer2D object.

Change the logic. Clear HwcComposer2D only if it is created by the main thread.

Comment 22

[Boris Chiou [:boris]]

Attachment: ClearOnShutdown for hwcomposer (v7)

Call ClearOnShutdown while creaing HwcComposer2D object.

Change the logic. Clear HwcComposer2D only if it is created by the main thread.

Comment 23

[Boris Chiou [:boris]]

Comment on attachment 8625886 [details] [diff] [review]
ClearOnShutdown for hwcomposer (v7)

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

Hi Sotaro,

Sorry, this patch should be reviewed again. In this patch, we call ClearOnShutdown() only if we create HwcComposer2D in the main thread because only main thread can handle this case properly. If anyone changes the logic and use the compositor thread to create HwcComposer2D in the future, we just skip this function. If ClearOnShutdown() can handle objects in other threads in the future, I will remove this check.

Comment 24

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8625886 [details] [diff] [review]
ClearOnShutdown for hwcomposer (v7)

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

review+ if try server test passes.

Comment 25

[Sotaro Ikeda [:sotaro]]

(In reply to Boris Chiou [:boris] from comment #23)
> Comment on attachment 8625886 [details] [diff] [review]
> ClearOnShutdown for hwcomposer (v7)
> 
> Review of attachment 8625886 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Sotaro,
> 
> Sorry, this patch should be reviewed again. In this patch, we call
> ClearOnShutdown() only if we create HwcComposer2D in the main thread because
> only main thread can handle this case properly. If anyone changes the logic
> and use the compositor thread to create HwcComposer2D in the future, we just
> skip this function. If ClearOnShutdown() can handle objects in other threads
> in the future, I will remove this check.

It seems better to be mentioned in the code comment.

Comment 26

[Boris Chiou [:boris]]

(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> (In reply to Boris Chiou [:boris] from comment #23)
> > Comment on attachment 8625886 [details] [diff] [review]
> > ClearOnShutdown for hwcomposer (v7)
> > 
> > Review of attachment 8625886 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hi Sotaro,
> > 
> > Sorry, this patch should be reviewed again. In this patch, we call
> > ClearOnShutdown() only if we create HwcComposer2D in the main thread because
> > only main thread can handle this case properly. If anyone changes the logic
> > and use the compositor thread to create HwcComposer2D in the future, we just
> > skip this function. If ClearOnShutdown() can handle objects in other threads
> > in the future, I will remove this check.
> 
> It seems better to be mentioned in the code comment.

OK. I will add this into the patch. Thanks for your review.

Comment 27

[Boris Chiou [:boris]]

Attachment: ClearOnShutdown for hwcomposer (v8, carry sotaro's r+)

Call ClearOnShutdown while creaing HwcComposer2D object.

Comment 28

[Boris Chiou [:boris]]

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

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e4a826279f0

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e4a826279f0