860463 - Channel errors cause assertions/crashes in B2G chrome process

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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)

Comment 1

[Nicolas Silva [:nical]]

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

Comment 2

[Nicolas Silva [:nical]]

Attachment: Make composite layers not crash when destruction order is incorrect

Comment 3

[Nick Cameron [:nrc]]

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.

Comment 4

[Nicolas Silva [:nical]]

(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.

Comment 5

[Nicolas Silva [:nical]]

(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.

Comment 6

[Nicolas Silva [:nical]]

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.

Comment 7

[Nick Cameron [:nrc]]

(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+

Comment 8

[Nicolas Silva [:nical]]

Attachment: Ensure we call propagate Destroy() on the shadow layer manager on CompositorParent::ActorDestroy

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.

Comment 9

[Nick Cameron [:nrc]]

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

Comment 10

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6248d561a978

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6248d561a978