commit dbc93d7ae8c7660aab002bcec2ade86daad7c172
author Garfield Tan <xutan@google.com> Tue Oct 26 18:28:57 2021 -0700
committer Garfield Tan <xutan@google.com> Wed Oct 27 10:40:55 2021 -0700
tree 2124f63e1509dd42686894d2b0c89b77b329236b
parent b47287e492c4e8ad6a487451971a848fc699807a

Save Layer instead of Handle to LayerCreatedState

Saving Handle to LayerCreatedState can lead to the destruction of Handle
when handleLayerCreatedLocked returns if the client side releases the
Handle before that. That destruction will attempt to acquire mStateLock
in the main thread again and lead to a deadlock.

Not saving Handle to LayerCreatedState will avoid Handle being destroyed
in the main thread. The destruction of Layer in the main thread is OK.

To make that happen I moved the final resolution of addToRoot to
createLayer as well because the treatments for invalid parentHandle
and invalid parentLayer are different.

Bug: 204204635
Bug: 202621651
Test: atest SurfaceFlinger_test
Test: atest libsurfaceflinger_unittest
Change-Id: I0854203c0949302d7f514d34b956eb7ae976c51f

services/surfaceflinger/SurfaceFlinger.h

diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 276c7f6..a1e431b 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -795,8 +795,8 @@
     // add a layer to SurfaceFlinger
     status_t addClientLayer(const sp<Client>& client, const sp<IBinder>& handle,
                             const sp<IGraphicBufferProducer>& gbc, const sp<Layer>& lbc,
-                            const sp<IBinder>& parentHandle, const sp<Layer>& parentLayer,
-                            bool addToRoot, uint32_t* outTransformHint);
+                            const wp<Layer>& parentLayer, bool addToRoot,
+                            uint32_t* outTransformHint);
 
     // Traverse through all the layers and compute and cache its bounds.
     void computeLayerBounds();
@@ -1345,18 +1345,16 @@
             GUARDED_BY(mStateLock);
     mutable Mutex mCreatedLayersLock;
     struct LayerCreatedState {
-        LayerCreatedState(const wp<Layer>& layer, const sp<IBinder>& parent,
-                          const wp<Layer> parentLayer, const wp<IBinder>& producer, bool addToRoot)
+        LayerCreatedState(const wp<Layer>& layer, const wp<Layer> parent,
+                          const wp<IBinder>& producer, bool addToRoot)
               : layer(layer),
                 initialParent(parent),
-                initialParentLayer(parentLayer),
                 initialProducer(producer),
                 addToRoot(addToRoot) {}
         wp<Layer> layer;
         // Indicates the initial parent of the created layer, only used for creating layer in
         // SurfaceFlinger. If nullptr, it may add the created layer into the current root layers.
-        sp<IBinder> initialParent;
-        wp<Layer> initialParentLayer;
+        wp<Layer> initialParent;
         // Indicates the initial graphic buffer producer of the created layer, only used for
         // creating layer in SurfaceFlinger.
         wp<IBinder> initialProducer;
@@ -1370,8 +1368,7 @@
     // thread.
     std::unordered_map<BBinder*, std::unique_ptr<LayerCreatedState>> mCreatedLayers;
     void setLayerCreatedState(const sp<IBinder>& handle, const wp<Layer>& layer,
-                              const sp<IBinder>& parent, const wp<Layer> parentLayer,
-                              const wp<IBinder>& producer, bool addToRoot);
+                              const wp<Layer> parent, const wp<IBinder>& producer, bool addToRoot);
     auto getLayerCreatedState(const sp<IBinder>& handle);
     sp<Layer> handleLayerCreatedLocked(const sp<IBinder>& handle) REQUIRES(mStateLock);