917602 - Fix some shutdown crashes

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[[:fabrice] Fabrice Desré]

While working on bug 915626 that uses the nsIAppStartup service to shutdown gecko I encountered a few different shutdown crashes.

This patch fixes all the ones I caught. I'm not sure that we don't have other lurking around unfortunately.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: patch

Here's the pot pourri.

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 806338 [details] [diff] [review]
patch

Asking for review to:
- bent for the hal/ code
- benWa for the gfx/ code.

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 806338 [details] [diff] [review]
patch

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

Sorry but this fix will take us in the wrong direction.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +197,5 @@
>  
>  void
>  CompositableQuirksGonkOGL::DeleteTextureIfPresent()
>  {
> +  if (mTexture && gl()) {

Unless we have a strong case for not being able to release these resources before we lose/destroy the context we shouldn't do this. We need to release the texture properly first which probably means destroying this object (or mTexture) before the gl context.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Benoit Girard (:BenWa) from comment #3)
> Comment on attachment 806338 [details] [diff] [review]
> patch
> 
> Review of attachment 806338 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry but this fix will take us in the wrong direction.
> 
> ::: gfx/layers/opengl/TextureHostOGL.cpp
> @@ +197,5 @@
> >  
> >  void
> >  CompositableQuirksGonkOGL::DeleteTextureIfPresent()
> >  {
> > +  if (mTexture && gl()) {
> 
> Unless we have a strong case for not being able to release these resources
> before we lose/destroy the context we shouldn't do this. We need to release
> the texture properly first which probably means destroying this object (or
> mTexture) before the gl context.

Ok, I'll gather a stack trace (I lost it) to see what we can do better here.

Comment 5

[Benoit Girard (:BenWa)]

Do we have a way to test a proper shutdown on b2g? We were wondering that when talking about proper shutdown last week. Getting proper leak logs from MOZ_COUNT_CTOR/DTOR is useful. If it can't be fixed easily it might be better for you to toss this issue to the gfx team.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 806338 [details] [diff] [review]
patch

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

r=me on the HAL stuff with this change:

::: hal/gonk/GonkDiskSpaceWatcher.cpp
@@ +133,5 @@
>    NS_IMETHOD Run()
>    {
>      MOZ_ASSERT(NS_IsMainThread());
> +    if (gHalDiskSpaceWatcher) {
> +      delete gHalDiskSpaceWatcher;

Please also null this out.

Comment 7

[[:fabrice] Fabrice Desré]

Pushed the non-gfx parts in https://hg.mozilla.org/integration/b2g-inbound/rev/1fce29969d8d

I will open a new bug with gfx issues once I'll get crash stacks.

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/1fce29969d8d