Closed Bug 917602 Opened 6 years ago Closed 6 years ago

Fix some shutdown crashes

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.2 FC (16sep)

People

(Reporter: fabrice, Assigned: fabrice)

Details

Attachments

(1 file)

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.
Attached patch patchSplinter Review
Here's the pot pourri.
Assignee: nobody → fabrice
Comment on attachment 806338 [details] [diff] [review]
patch

Asking for review to:
- bent for the hal/ code
- benWa for the gfx/ code.
Attachment #806338 - Flags: review?(bgirard)
Attachment #806338 - Flags: review?(bent.mozilla)
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.
Attachment #806338 - Flags: review?(bgirard) → review-
(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.
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 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.
Attachment #806338 - Flags: review?(bgirard)
Attachment #806338 - Flags: review?(bent.mozilla)
Attachment #806338 - Flags: review-
Attachment #806338 - Flags: review+
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.
https://hg.mozilla.org/mozilla-central/rev/1fce29969d8d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
You need to log in before you can comment on or make changes to this bug.