Closed Bug 860463 Opened 10 years ago Closed 10 years ago

Channel errors cause assertions/crashes in B2G chrome process

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jrmuizel, Assigned: nical)

References

Details

(Keywords: crash, Whiteboard: [b2g-crash])

Crash Data

Attachments

(1 file, 1 obsolete file)

I believe this will cause stacks like this:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 531.553]
0x41d99e80 in ~ImageLayerComposite (this=0x49504400, __in_chrg=<value optimized out>)
    at /hack/mozilla-graphics/gfx/layers/composite/ImageLayerComposite.cpp:36
36        MOZ_ASSERT(mDestroyed);
(gdb) bt
#0  0x41d99e80 in ~ImageLayerComposite (this=0x49504400, __in_chrg=<value optimized out>)
    at /hack/mozilla-graphics/gfx/layers/composite/ImageLayerComposite.cpp:36
#1  0x41d99f1a in ~ImageLayerComposite (this=0x49504400, __in_chrg=<value optimized out>)
    at /hack/mozilla-graphics/gfx/layers/composite/ImageLayerComposite.cpp:39
#2  0x40e96dea in mozilla::layers::Layer::Release (this=0x49504490) at ../../dist/include/Layers.h:588
#3  0x41d98cec in mozilla::layers::ContainerLayerComposite::RemoveChild (this=0x47eae000, aChild=0x49504490)
    at /hack/mozilla-graphics/gfx/layers/composite/ContainerLayerComposite.cpp:246
#4  0x41d98938 in ~ContainerLayerComposite (this=0x47eae000, __in_chrg=<value optimized out>)
    at /hack/mozilla-graphics/gfx/layers/composite/ContainerLayerComposite.cpp:176
#5  0x41d989be in ~ContainerLayerComposite (this=0x47eae000, __in_chrg=<value optimized out>)
    at /hack/mozilla-graphics/gfx/layers/composite/ContainerLayerComposite.cpp:178
#6  0x40e96dea in mozilla::layers::Layer::Release (this=0x47eae090) at ../../dist/include/Layers.h:588
#7  0x41d98cec in mozilla::layers::ContainerLayerComposite::RemoveChild (this=0x493ccc00, aChild=0x47eae090)
    at /hack/mozilla-graphics/gfx/layers/composite/ContainerLayerComposite.cpp:246
#8  0x41d98938 in ~ContainerLayerComposite (this=0x493ccc00, __in_chrg=<value optimized out>)
    at /hack/mozilla-graphics/gfx/layers/composite/ContainerLayerComposite.cpp:176
#9  0x41d989be in ~ContainerLayerComposite (this=0x493ccc00, __in_chrg=<value optimized out>)
    at /hack/mozilla-graphics/gfx/layers/composite/ContainerLayerComposite.cpp:178
#10 0x40e96dea in mozilla::layers::Layer::Release (this=0x493ccc90) at ../../dist/include/Layers.h:588
#11 0x41d6727a in ~nsRefPtr (this=0x47ebd0e0, __in_chrg=<value optimized out>) at ../../dist/include/nsAutoPtr.h:880
#12 0x41dc9df8 in ~ShadowLayersParent (this=0x47ebd0b0, __in_chrg=<value optimized out>)
    at /hack/mozilla-graphics/gfx/layers/ipc/ShadowLayersParent.cpp:150
#13 0x41dc9e56 in ~ShadowLayersParent (this=0x47ebd0b0, __in_chrg=<value optimized out>)
    at /hack/mozilla-graphics/gfx/layers/ipc/ShadowLayersParent.cpp:150
#14 0x41db5b60 in mozilla::layers::CrossProcessCompositorParent::DeallocPLayers (this=0x487bb0e0, aLayers=0x47ebd0b0)
    at /hack/mozilla-graphics/gfx/layers/ipc/CompositorParent.cpp:1430
#15 0x41990f5e in mozilla::layers::PCompositorParent::DeallocSubtree (this=0x487bb0e0)
    at /hack/b2g/B2G/objdir-gecko/ipc/ipdl/PCompositorParent.cpp:831
#16 0x41991162 in mozilla::layers::PCompositorParent::OnChannelError (this=0x487bb0e0)
Severity: normal → critical
Crash Signature: [@ ~ImageLayerComposite]
Keywords: crash
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: [b2g-crash]
This looks very much like we don't have a proper destruction sequence on b2g (using CrossProcessCompositorParent) at all.
There is bug 774388 that was filed but not fixed. The difference is that with the refactoring we assert that the destruction sequence is actually happening properly.

see https://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#1294
Depends on: 774388
Comment on attachment 736103 [details] [diff] [review]
Make composite layers not crash when destruction order is incorrect

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

This seems like a bad idea - Destroy will sometimes involve deallocating textures which will use the layer manager as a deallocator, which may well have kicked off the destruction sequence by dying itself and so we'll get virtual method call badness.
(In reply to Nick Cameron [:nrc] from comment #3)
> Comment on attachment 736103 [details] [diff] [review]
> Make composite layers not crash when destruction order is incorrect
> 
> Review of attachment 736103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like a bad idea - Destroy will sometimes involve deallocating
> textures which will use the layer manager as a deallocator, which may well
> have kicked off the destruction sequence by dying itself and so we'll get
> virtual method call badness.

That's true, I'll try overloading ActorDestroy to make sure we propagate Destroy() before getting to the protocol's dtor.
(In reply to Nick Cameron [:nrc] from comment #3)
> Comment on attachment 736103 [details] [diff] [review]
> Make composite layers not crash when destruction order is incorrect
> 
> Review of attachment 736103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like a bad idea - Destroy will sometimes involve deallocating
> textures which will use the layer manager as a deallocator, which may well
> have kicked off the destruction sequence by dying itself and so we'll get
> virtual method call badness.

Actually this does not trigger any code path that leads to destroying the shared data, it only detaches the compositable, destroy the device resources like gl textures and set the compositor to null.

So it does not produce any ISurfaceAllocator call.
ideally we should have an API like layer->Destroy(RESOURCE_SHARED|RESOURCE_DEVICE)
to make it clearer what we destroy when we are in the tear-down sequence.
(In reply to Nicolas Silva [:nical] from comment #5)
> (In reply to Nick Cameron [:nrc] from comment #3)
> > Comment on attachment 736103 [details] [diff] [review]
> > Make composite layers not crash when destruction order is incorrect
> > 
> > Review of attachment 736103 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This seems like a bad idea - Destroy will sometimes involve deallocating
> > textures which will use the layer manager as a deallocator, which may well
> > have kicked off the destruction sequence by dying itself and so we'll get
> > virtual method call badness.
> 
> Actually this does not trigger any code path that leads to destroying the
> shared data, it only detaches the compositable, destroy the device resources
> like gl textures and set the compositor to null.
> 
> So it does not produce any ISurfaceAllocator call.

"destroy the device resources" does not include destroying surface descriptors? I think if we detach the compositable, it could die, taking its texture with it and have to destroy a surface descriptor. If it doesn't, then we're golden - r+
How about this: ActorDestroy is always called before an IPDL actor's dtor. This patch overloads CompositorParent::ActorDestroy in order to call the layer manager's Destroy().

With this and CompositableParent::ActorDestroy also calling Detach on the compositable, we can make sure that Detach will always be called before the manager protocol's dtor.
Attachment #736103 - Attachment is obsolete: true
Attachment #736103 - Flags: review?(ncameron)
Attachment #736131 - Flags: review?
Attachment #736131 - Flags: review? → review?(ncameron)
Comment on attachment 736131 [details] [diff] [review]
Ensure we call propagate Destroy() on the shadow layer manager on CompositorParent::ActorDestroy

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

I think this sounds OK, not 100% sure though
Attachment #736131 - Flags: review?(ncameron) → review+
https://hg.mozilla.org/mozilla-central/rev/6248d561a978
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.