Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 9c977c4f authored by Alec Mouri's avatar Alec Mouri
Browse files

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
parent 9d533913

services/surfaceflinger/CompositionEngine/include/compositionengine/impl/planner/CachedSet.h

+0 −1
Original line number Diff line number Diff line
@@ -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; }

services/surfaceflinger/CompositionEngine/src/planner/Flattener.cpp

+6 −2
Original line number Diff line number Diff line
@@ -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];

services/surfaceflinger/CompositionEngine/tests/planner/CachedSetTest.cpp

+0 −3
Original line number Diff line number Diff line
@@ -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;