924647 - Fix shutdown issues with IPC and gralloc layers

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Kelsey Gilbert [:jgilbert]]

In bug 914823, we (appear to be going to) hold a weak-pointer to the surface allocator. This way, when it gets destroyed too early, we know we can't access it anymore.

While this more-or-less works for main-thread code, off-main-thread code (like workers) would need to temporarily promote this to a strong-ptr, so we have the same issues of holding a strong-ptr to the allocator. (It will try to use the now-dead IPC subsystem)

We need a better fix. What form this should take is still in discussion. We either need to do IPC teardown only after GFX teardown, or have a better way to handle having IPC torn out from under us. Really, we need a better way to handle IPC-tear-outs, since I understand this can happen if one of the processes crashes.

Comment 1

[(no longer active)]

Strongly promoting the weakptr to a strong ptr won't fix anything, since that doesn't guarantee that the object is going to be in a consistent state.  Specifically if the IPC subsystem is dead, even if you keep an actor alive, it won't be able to do anything useful any more, and it seems to me like the weakptr trick will just move the crash elsewhere.

Can't we just destroy the IPC subsystem as the last thing we do in our shutdown, specifically after the last GC+CC when the DOM objects finally die?

Comment 2

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

It's also very easy to set a flag on the stronly-owned actor when the IPC parts die so that we don't crash later.

Comment 3

[Sotaro Ikeda [:sotaro]]

weakptr is just going to necessary to prevent dual/circular strong pointer references. Basically we are thinking about to use stronly-owned actor. Necessity of weakptr is just under consideration.

Comment 4

[Sotaro Ikeda [:sotaro]]

Android's referenced object support the following functions. If gecko's referenced object supports such functions, implementation becomes easiser.
- void onFirstRef();
- void onLastStrongRef();
- bool onIncStrongAttempted();
- void onLastWeakRef();

http://androidxref.com/4.3_r2.1/xref/frameworks/native/include/utils/RefBase.h#145

Comment 5

[Sotaro Ikeda [:sotaro]]

I feel that the implementation patterns seems to be categorized to following patterns.
-[1] single threaded, single user
-[2] single threaded, multiple user
-[3] multiple threaded, single user
-[4] multiple threaded, multiple user

Comment 6

[Sotaro Ikeda [:sotaro]]

Android uses "thread safe weak pointer" in c++ code, but it's usage is limited only to observer pattern like the following.
  http://androidxref.com/4.3_r2.1/xref/frameworks/av/media/libstagefright/AwesomePlayer.cpp#588

I also feel that weak pointer usage should be basically limited to such observer pattern. If we want to use weak pointer in another use case, it might be better to think about changing the architecture to fix the problem. And rethink about whether the weak pointer is necessary in that case.

Comment 7

[Sotaro Ikeda [:sotaro]]

patch in Bug 914823 seems not fit to the weak pointer usage pattern in Comment 6. I think that it is OK as a short time solution to fix the problem. But longer term, we should change the weak pointer usage by correcting an architecture. We already talked to change ISurfaceAllocator independent from LayerTransaction somewhere a little bit. It seems to solve the weak pointer usage in Bug 914823.

Comment 8

[Florian Bender]

Is this bug still valid or can it be closed?

Comment 9

[Kelsey Gilbert [:jgilbert]]

This is a b2g-only bug.