SurfaceFlinger/LayerRenderArea: Hold lock while modifying hierarchy

Multiple issues are seen in RenderArea when destroying
the screenshotParentLayer container layer used by LayerRenderArea.
This code executes on the main thread without a lock. Our rules for
locking in SurfaceFlinger are like this:
	1. The main thread will hold the lock when writing
	to the state, and be the only writer
	2. Anyone can read the state if they hold the lock
	3. Main thread can read from the state without the lock
	since it is the only writer.
LayerRenderArea's reparentForDrawing operation violates these rules
by updating mDrawingParent without holding the lock. If there were
some other binder thread which was accessing the hierarchy (say calling
getAlpha on a layer in the hierarchy descending from our screenshot
layer), we could have a sequence like this:
	1. Other thread calls mDrawingParent.promote(), on the wp
	we provided constructing ReparentForDrawing
	2. At the same time we destroy ReparentForDrawing and overwrite
	the wp
	3. This causes wp::~wp and wp::promote to interleave
	4. While the wp counters themselves are atomic, remember
	the object itself is not locked, and so after ~wp the
	counters that we think are stored in our ~wp could be anything
	and so we could promote without properly incremeting
	the reference ccount
	5. When the sp returned from promote is destroyed, it
	could then destroy the layer, and when our original sp
	from LayerRenderArea is destroyed, we then get the issue
        (which is what we see in the traces).
It's hard to say what this other thread is. Over time we have tried to
move usage of the Layer hierarchy off the Binder threads to achieve
a totally lock-free model. At the moment, I can't find spots in
tip-of-tree which would race with mDrawingParent in this fashion.
Until we are ready to get rid of mStateLock, apply it's usage
consistently with our three rules of locking.

Bug: 220176775
Bug: 223069308
Bug: 223081111
Change-Id: I89c5add585f96b7de1f1b21f76b6ea0c6b0de127
(cherry picked from commit 22ceeaaff9be3e71c5473aeabccdb02705565190)
CRs-Fixed: 3159089

2 files changed, 14 insertions, 11 deletions

diff --git a/services/surfaceflinger/LayerRenderArea.cpp b/services/surfaceflinger/LayerRenderArea.cpp
index 11fe6d0755..bced928101 100644
--- a/services/surfaceflinger/LayerRenderArea.cpp
+++ b/services/surfaceflinger/LayerRenderArea.cpp
@@ -26,18 +26,12 @@
 namespace android {
 namespace {
 
-struct ReparentForDrawing {
-    const sp<Layer>& oldParent;
-
-    ReparentForDrawing(const sp<Layer>& oldParent, const sp<Layer>& newParent,
-                       const Rect& drawingBounds)
-          : oldParent(oldParent) {
+void reparentForDrawing(const sp<Layer>& oldParent, const sp<Layer>& newParent,
+                   const Rect& drawingBounds) {
         // Compute and cache the bounds for the new parent layer.
         newParent->computeBounds(drawingBounds.toFloatRect(), ui::Transform(),
-                                 0.f /* shadowRadius */);
+            0.f /* shadowRadius */);
         oldParent->setChildrenDrawingParent(newParent);
-    }
-    ~ReparentForDrawing() { oldParent->setChildrenDrawingParent(oldParent); }
 };
 
 } // namespace
@@ -116,11 +110,19 @@ void LayerRenderArea::render(std::function<void()> drawLayers) {
         uint32_t h = static_cast<uint32_t>(getHeight());
         // In the "childrenOnly" case we reparent the children to a screenshot
         // layer which has no properties set and which does not draw.
+        //  We hold the statelock as the reparent-for-drawing operation modifies the
+        //  hierarchy and there could be readers on Binder threads, like dump.
         sp<ContainerLayer> screenshotParentLayer = mFlinger.getFactory().createContainerLayer(
                 {&mFlinger, nullptr, "Screenshot Parent"s, w, h, 0, LayerMetadata()});
-
-        ReparentForDrawing reparent(mLayer, screenshotParentLayer, sourceCrop);
+        {
+            Mutex::Autolock _l(mFlinger.mStateLock);
+            reparentForDrawing(mLayer, screenshotParentLayer, sourceCrop);
+        }
         drawLayers();
+        {
+            Mutex::Autolock _l(mFlinger.mStateLock);
+            mLayer->setChildrenDrawingParent(mLayer);
+        }
     }
 }
 
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index dad158d89b..c4f555db74 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -443,6 +443,7 @@ private:
     friend class RefreshRateOverlay;
     friend class RegionSamplingThread;
     friend class SurfaceTracing;
+    friend class LayerRenderArea;
 
     // For unit tests
     friend class TestableSurfaceFlinger;