From 9c977c4f313cd77ea716931d5aabf26bdbf52f58 Mon Sep 17 00:00:00 2001
From: Alec Mouri <alecmouri@google.com>
Date: Wed, 28 Apr 2021 11:39:57 -0700
Subject: Fix two crash causes in Flattener related to hashing

First: Add an ID check to isSameStack in Flattener. Without this check,
mergeWithCachedSets may try to update the override buffer from a cached
set containing a constituent layer that no longer exists, because that
layer was replaced by another layer with the same geometry.

Second: Replace the finger print check for identifying a cached set with
checking the first layer's ID. Otherwise, if there are two cached sets
whose first layer happens to hash to the same value because their
geometries are the same, then the incorrect cached set may be updated,
which breaks the entire skipLayers logic.

Bug: 186438649
Test: Idling in maps does not crash
Test: Incoming notifications while maps is onscreen does not crash the
device.

Change-Id: Idc050d13b840342725a0ea24f1dd512d19868416
---
 .../include/compositionengine/impl/planner/CachedSet.h            | 1 -
 .../surfaceflinger/CompositionEngine/src/planner/Flattener.cpp    | 8 ++++++--
 .../CompositionEngine/tests/planner/CachedSetTest.cpp             | 3 ---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h
index a6c4eafbab..6160db1b9e 100644
--- a/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h
+++ b/services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h
@@ -60,7 +60,6 @@ public:
     void addLayer(const LayerState*, std::chrono::steady_clock::time_point lastUpdate);
 
     std::chrono::steady_clock::time_point getLastUpdate() const { return mLastUpdate; }
-    NonBufferHash getFingerprint() const { return mFingerprint; }
     size_t getLayerCount() const { return mLayers.size(); }
     const Layer& getFirstLayer() const { return mLayers[0]; }
     const Rect& getBounds() const { return mBounds; }
diff --git a/services/surfaceflinger/CompositionEngine/src/planner/Flattener.cpp b/services/surfaceflinger/CompositionEngine/src/planner/Flattener.cpp
index b4e761004f..dcb970bfe6 100644
--- a/services/surfaceflinger/CompositionEngine/src/planner/Flattener.cpp
+++ b/services/surfaceflinger/CompositionEngine/src/planner/Flattener.cpp
@@ -45,7 +45,10 @@ bool isSameStack(const std::vector<const LayerState*>& incomingLayers,
     }
 
     for (size_t i = 0; i < incomingLayers.size(); i++) {
-        if (incomingLayers[i]->getDifferingFields(*(existingLayers[i])) != LayerStateField::None) {
+        // Checking the IDs here is very strict, but we do this as otherwise we may mistakenly try
+        // to access destroyed OutputLayers later on.
+        if (incomingLayers[i]->getId() != existingLayers[i]->getId() ||
+            incomingLayers[i]->getDifferingFields(*(existingLayers[i])) != LayerStateField::None) {
             return false;
         }
     }
@@ -232,7 +235,8 @@ bool Flattener::mergeWithCachedSets(const std::vector<const LayerState*>& layers
     auto currentLayerIter = mLayers.begin();
     auto incomingLayerIter = layers.begin();
     while (incomingLayerIter != layers.end()) {
-        if (mNewCachedSet && mNewCachedSet->getFingerprint() == (*incomingLayerIter)->getHash()) {
+        if (mNewCachedSet &&
+            mNewCachedSet->getFirstLayer().getState()->getId() == (*incomingLayerIter)->getId()) {
             if (mNewCachedSet->hasBufferUpdate()) {
                 ALOGV("[%s] Dropping new cached set", __func__);
                 ++mInvalidatedCachedSetAges[0];
diff --git a/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp b/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp
index 2f2c670d84..b98dac2dfb 100644
--- a/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp
+++ b/services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp
@@ -105,7 +105,6 @@ void CachedSetTest::TearDown() {
 }
 
 void expectEqual(const CachedSet& cachedSet, const CachedSet::Layer& layer) {
-    EXPECT_EQ(layer.getHash(), cachedSet.getFingerprint());
     EXPECT_EQ(layer.getLastUpdate(), cachedSet.getLastUpdate());
     EXPECT_EQ(layer.getDisplayFrame(), cachedSet.getBounds());
     EXPECT_TRUE(layer.getVisibleRegion().hasSameRects(cachedSet.getVisibleRegion()));
@@ -154,7 +153,6 @@ TEST_F(CachedSetTest, addLayer) {
     CachedSet cachedSet(layer1);
     cachedSet.addLayer(layer2.getState(), kStartTime + 10ms);
 
-    EXPECT_EQ(layer1.getHash(), cachedSet.getFingerprint());
     EXPECT_EQ(kStartTime, cachedSet.getLastUpdate());
     EXPECT_EQ(Rect(0, 0, 2, 2), cachedSet.getBounds());
     Region expectedRegion;
@@ -243,7 +241,6 @@ TEST_F(CachedSetTest, append) {
     cachedSet1.addLayer(layer3.getState(), kStartTime + 10ms);
     cachedSet1.append(cachedSet2);
 
-    EXPECT_EQ(layer1.getHash(), cachedSet1.getFingerprint());
     EXPECT_EQ(kStartTime, cachedSet1.getLastUpdate());
     EXPECT_EQ(Rect(0, 0, 3, 3), cachedSet1.getBounds());
     Region expectedRegion;
-- 
cgit v1.2.3-59-g8ed1b