Closed Bug 1170061 Opened 5 years ago Closed 5 years ago

Handle shutdown case for HwcComposer2D

Categories

(Core Graveyard :: Widget: Gonk, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(1 file, 9 obsolete files)

We should call ClearOnShutdown() while creating HwcComposer2D object to make sure we handle the shutdown case properly.
Blocks: RefactorHwc
Depends on: 1144012
Assignee: nobody → boris.chiou
Assignee: boris.chiou → nobody
Summary: Handle shutdown case for HwcComposer2D → Handle shutdown case for HwcComposer2D and HwcHAL
OS: Unspecified → Gonk (Firefox OS)
Hardware: Unspecified → ARM
Call ClearOnShutdown when new HwcComposer2D object and clear
HwcHAL in the destructor of HwcComposer2D.
Attachment #8613349 - Attachment is obsolete: true
Assignee: nobody → boris.chiou
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.
Attachment #8613452 - Flags: feedback?(sotaro.ikeda.g)
Attachment #8613452 - Flags: feedback?(sotaro.ikeda.g) → feedback+
No longer depends on: 1144012
Summary: Handle shutdown case for HwcComposer2D and HwcHAL → Handle shutdown case for HwcComposer2D
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.
Attachment #8613452 - Attachment is obsolete: true
Attachment #8617083 - Attachment is obsolete: true
Attachment #8617085 - Flags: review?(sotaro.ikeda.g)
Status: NEW → ASSIGNED
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?
(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.
(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
(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.
Call ClearOnShutdown while creaing HwcComposer2D object.
Attachment #8617085 - Attachment is obsolete: true
Attachment #8617085 - Flags: review?(sotaro.ikeda.g)
Attachment #8617790 - Flags: review?(sotaro.ikeda.g)
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
(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.
(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
(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.
Attachment #8617790 - Flags: review?(sotaro.ikeda.g)
Call ClearOnShutdown while creaing HwcComposer2D object.
Attachment #8622838 - Attachment is obsolete: true
Attachment #8622842 - Flags: review?(sotaro.ikeda.g)
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?
Attachment #8622842 - Flags: review?(sotaro.ikeda.g) → review+
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.
Call ClearOnShutdown while creaing HwcComposer2D object.

Change the logic. Clear HwcComposer2D only if it is created by the main thread.
Attachment #8625885 - Attachment is obsolete: true
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.
Attachment #8625886 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8625886 [details] [diff] [review]
ClearOnShutdown for hwcomposer (v7)

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

review+ if try server test passes.
Attachment #8625886 - Flags: review?(sotaro.ikeda.g) → review+
(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.
(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.
Call ClearOnShutdown while creaing HwcComposer2D object.
Attachment #8625886 - Attachment is obsolete: true
Attachment #8626000 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e4a826279f0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.