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

Commit 047fb33d authored by Vishnu Nair's avatar Vishnu Nair
Browse files

SF: Fix tracing layerhandle to layerid map 

Fixes an issue where the layer handle would be
added after it was used in a transaction by adding
the handle before constructing the create layer
transaction in SF.

Fixes an issue where the layer handle pointer
could be reused by a new handle by updating
the map when the handle is destroyed.

Test: presubmit
Bug: 200284593
Change-Id: Ia163e456500fc909bcda6e97dd8c7efa4be50c9e
parent ab6dc5c5

services/surfaceflinger/SurfaceFlinger.cpp

+8 −4
Original line number Diff line number Diff line
@@ -4422,10 +4422,6 @@ status_t SurfaceFlinger::createLayer(LayerCreationArgs& args, sp<IBinder>* outHa 
    if (parentLayer != nullptr) {

        addToRoot = false;

    }

    result = addClientLayer(args.client, *outHandle, layer, parent, addToRoot, outTransformHint);

    if (result != NO_ERROR) {

        return result;

    }



    int parentId = -1;

    // We can safely promote the layer in binder thread because we have a strong reference
@@ -4439,6 +4435,11 @@ status_t SurfaceFlinger::createLayer(LayerCreationArgs& args, sp<IBinder>* outHa 
                                         args.flags, parentId);

    }



    result = addClientLayer(args.client, *outHandle, layer, parent, addToRoot, outTransformHint);

    if (result != NO_ERROR) {

        return result;

    }



    setTransactionFlags(eTransactionNeeded);

    *outLayerId = layer->sequence;

    return result;
@@ -4521,6 +4522,9 @@ void SurfaceFlinger::onHandleDestroyed(BBinder* handle, sp<Layer>& layer) { 
    markLayerPendingRemovalLocked(layer);

    mBufferCountTracker.remove(handle);

    layer.clear();

    if (mTransactionTracingEnabled) {

        mTransactionTracing.onHandleRemoved(handle);

    }

}



// ---------------------------------------------------------------------------

services/surfaceflinger/Tracing/TransactionTracing.cpp

+18 −10
Original line number Diff line number Diff line
@@ -199,7 +199,7 @@ void TransactionTracing::addEntry(const std::vector<CommittedTransactions>& comm 
                entryProto.mutable_transactions()->Add(std::move(it->second));

                mQueuedTransactions.erase(it);

            } else {

                ALOGE("Could not find transaction id %" PRIu64, id);

                ALOGW("Could not find transaction id %" PRIu64, id);

            }

        }

        std::vector<proto::TransactionTraceEntry> entries = mBuffer->emplace(std::move(entryProto));
@@ -228,6 +228,9 @@ void TransactionTracing::onLayerAdded(BBinder* layerHandle, int layerId, const s 
                                      uint32_t flags, int parentId) {

    std::scoped_lock lock(mTraceLock);

    TracingLayerCreationArgs args{layerId, name, flags, parentId, -1 /* mirrorFromId */};

    if (mLayerHandles.find(layerHandle) != mLayerHandles.end()) {

        ALOGW("Duplicate handles found. %p", layerHandle);

    }

    mLayerHandles[layerHandle] = layerId;

    proto::LayerCreationArgs protoArgs = TransactionProtoParser::toProto(args);

    proto::LayerCreationArgs protoArgsCopy = protoArgs;
@@ -238,6 +241,9 @@ void TransactionTracing::onMirrorLayerAdded(BBinder* layerHandle, int layerId, 
                                            const std::string& name, int mirrorFromId) {

    std::scoped_lock lock(mTraceLock);

    TracingLayerCreationArgs args{layerId, name, 0 /* flags */, -1 /* parentId */, mirrorFromId};

    if (mLayerHandles.find(layerHandle) != mLayerHandles.end()) {

        ALOGW("Duplicate handles found. %p", layerHandle);

    }

    mLayerHandles[layerHandle] = layerId;

    mCreatedLayers.emplace_back(TransactionProtoParser::toProto(args));

}
@@ -247,6 +253,11 @@ void TransactionTracing::onLayerRemoved(int32_t layerId) { 
    tryPushToTracingThread();

}



void TransactionTracing::onHandleRemoved(BBinder* layerHandle) {

    std::scoped_lock lock(mTraceLock);

    mLayerHandles.erase(layerHandle);

}



void TransactionTracing::tryPushToTracingThread() {

    // Try to acquire the lock from main thread.

    if (mMainThreadLock.try_lock()) {
@@ -270,7 +281,11 @@ int32_t TransactionTracing::getLayerIdLocked(const sp<IBinder>& layerHandle) { 
        return -1;

    }

    auto it = mLayerHandles.find(layerHandle->localBinder());

    return it == mLayerHandles.end() ? -1 : it->second;

    if (it == mLayerHandles.end()) {

        ALOGW("Could not find layer handle %p", layerHandle->localBinder());

        return -1;

    }

    return it->second;

}



void TransactionTracing::updateStartingStateLocked(
@@ -288,7 +303,7 @@ void TransactionTracing::updateStartingStateLocked( 
        for (const proto::LayerState& layerState : transaction.layer_changes()) {

            auto it = mStartingStates.find(layerState.layer_id());

            if (it == mStartingStates.end()) {

                ALOGE("Could not find layer id %d", layerState.layer_id());

                ALOGW("Could not find layer id %d", layerState.layer_id());

                continue;

            }

            TransactionProtoParser::fromProto(layerState, nullptr, it->second);
@@ -298,13 +313,6 @@ void TransactionTracing::updateStartingStateLocked( 
    // Clean up stale starting states since the layer has been removed and the buffer does not

    // contain any references to the layer.

    for (const int32_t removedLayerId : removedEntry.removed_layers()) {

        auto it = std::find_if(mLayerHandles.begin(), mLayerHandles.end(),

                               [removedLayerId](auto& layer) {

                                   return layer.second == removedLayerId;

                               });

        if (it != mLayerHandles.end()) {

            mLayerHandles.erase(it);

        }

        mStartingStates.erase(removedLayerId);

    }

}

services/surfaceflinger/Tracing/TransactionTracing.h

+1 −0
Original line number Diff line number Diff line
@@ -67,6 +67,7 @@ public: 
    void onMirrorLayerAdded(BBinder* layerHandle, int layerId, const std::string& name,

                            int mirrorFromId);

    void onLayerRemoved(int layerId);

    void onHandleRemoved(BBinder* layerHandle);

    void dump(std::string&) const;

    static constexpr auto CONTINUOUS_TRACING_BUFFER_SIZE = 512 * 1024;

    static constexpr auto ACTIVE_TRACING_BUFFER_SIZE = 100 * 1024 * 1024;