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

Commit 9445c929 authored by Josh Gao's avatar Josh Gao Committed by Ying Wei
Browse files

SurfaceFlinger: add more thread-safety annotations. 

As part of debugging a crash in SurfaceFlinger, we became suspicious of
markLayerPendingRemovalLocked because it wasn't annotated with thread
safety annotations. This ended up being a red herring, because
mStateLock is held on all paths into that function. Add some helpers to
MutexUtils.h to require/assert mutexes, and use them to sprinkle some
thread safety annotations to make things more clear.

Test: m surfaceflinger
Change-Id: Ib48433765bb6ffcb351ec10cd5bc89e254b48545
Merged-In: Ib48433765bb6ffcb351ec10cd5bc89e254b48545
parent b1e815fe

services/surfaceflinger/Layer.cpp

+9 −4
Original line number Diff line number Diff line
@@ -72,6 +72,7 @@ 
#include "LayerProtoHelper.h"

#include "LayerRejecter.h"

#include "MonitoredProducer.h"

#include "MutexUtils.h"

#include "SurfaceFlinger.h"

#include "TimeStats/TimeStats.h"

#include "TunnelModeEnabledReporter.h"
@@ -254,7 +255,9 @@ void Layer::onRemovedFromCurrentState() { 
    auto layersInTree = getRootLayer()->getLayersInTree(LayerVector::StateSet::Current);

    std::sort(layersInTree.begin(), layersInTree.end());



    traverse(LayerVector::StateSet::Current, [&](Layer* layer) {

    REQUIRE_MUTEX(mFlinger->mStateLock);

    traverse(LayerVector::StateSet::Current,

             [&](Layer* layer) REQUIRES(layer->mFlinger->mStateLock) {

                 layer->removeFromCurrentState();

                 layer->removeRelativeZ(layersInTree);

             });
@@ -936,10 +939,12 @@ bool Layer::setBackgroundColor(const half3& color, float alpha, ui::Dataspace da 
        mFlinger->mLayersAdded = true;

        // set up SF to handle added color layer

        if (isRemovedFromCurrentState()) {

            MUTEX_ALIAS(mFlinger->mStateLock, mDrawingState.bgColorLayer->mFlinger->mStateLock);

            mDrawingState.bgColorLayer->onRemovedFromCurrentState();

        }

        mFlinger->setTransactionFlags(eTransactionNeeded);

    } else if (mDrawingState.bgColorLayer && alpha == 0) {

        MUTEX_ALIAS(mFlinger->mStateLock, mDrawingState.bgColorLayer->mFlinger->mStateLock);

        mDrawingState.bgColorLayer->reparent(nullptr);

        mDrawingState.bgColorLayer = nullptr;

        return true;

services/surfaceflinger/Layer.h

+8 −7
Original line number Diff line number Diff line
@@ -409,7 +409,7 @@ public: 
    virtual ui::LayerStack getLayerStack() const;

    virtual bool setMetadata(const LayerMetadata& data);

    virtual void setChildrenDrawingParent(const sp<Layer>&);

    virtual bool reparent(const sp<IBinder>& newParentHandle);

    virtual bool reparent(const sp<IBinder>& newParentHandle) REQUIRES(mFlinger->mStateLock);

    virtual bool setColorTransform(const mat4& matrix);

    virtual mat4 getColorTransform() const;

    virtual bool hasColorTransform() const;
@@ -433,7 +433,8 @@ public: 
    virtual bool setSidebandStream(const sp<NativeHandle>& /*sidebandStream*/) { return false; };

    virtual bool setTransactionCompletedListeners(

            const std::vector<sp<CallbackHandle>>& /*handles*/);

    virtual bool setBackgroundColor(const half3& color, float alpha, ui::Dataspace dataspace);

    virtual bool setBackgroundColor(const half3& color, float alpha, ui::Dataspace dataspace)

            REQUIRES(mFlinger->mStateLock);

    virtual bool setColorSpaceAgnostic(const bool agnostic);

    virtual bool setDimmingEnabled(const bool dimmingEnabled);

    virtual bool setFrameRateSelectionPriority(int32_t priority);
@@ -715,13 +716,13 @@ public: 
    /*

     * Remove from current state and mark for removal.

     */

    void removeFromCurrentState();

    void removeFromCurrentState() REQUIRES(mFlinger->mStateLock);



    /*

     * called with the state lock from a binder thread when the layer is

     * removed from the current list to the pending removal list

     */

    void onRemovedFromCurrentState();

    void onRemovedFromCurrentState() REQUIRES(mFlinger->mStateLock);



    /*

     * Called when the layer is added back to the current state list.
@@ -899,6 +900,9 @@ public: 


    virtual bool simpleBufferUpdate(const layer_state_t&) const { return false; }



    // Exposed so SurfaceFlinger can assert that it's held

    const sp<SurfaceFlinger> mFlinger;



protected:

    friend class impl::SurfaceInterceptor;
@@ -974,9 +978,6 @@ protected: 
     */

    virtual Rect getInputBounds() const;



    // constant

    sp<SurfaceFlinger> mFlinger;



    bool mPremultipliedAlpha{true};

    const std::string mName;

    const std::string mTransactionName{"TX - " + mName};

services/surfaceflinger/MutexUtils.h

+10 −0
Original line number Diff line number Diff line
@@ -50,4 +50,14 @@ struct SCOPED_CAPABILITY TimedLock { 
    const status_t status;

};



// Require, under penalty of compilation failure, that the compiler thinks that a mutex is held.

#define REQUIRE_MUTEX(expr) ([]() REQUIRES(expr) {})()



// Tell the compiler that we know that a mutex is held.

#define ASSERT_MUTEX(expr) ([]() ASSERT_CAPABILITY(expr) {})()



// Specify that one mutex is an alias for another.

// (e.g. SurfaceFlinger::mStateLock and Layer::mFlinger->mStateLock)

#define MUTEX_ALIAS(held, alias) (REQUIRE_MUTEX(held), ASSERT_MUTEX(alias))



} // namespace android

services/surfaceflinger/SurfaceFlinger.cpp

+2 −0
Original line number Diff line number Diff line
@@ -4415,6 +4415,7 @@ uint32_t SurfaceFlinger::setClientStateLocked(const FrameTimelineInfo& frameTime 
        }

        return 0;

    }

    MUTEX_ALIAS(mStateLock, layer->mFlinger->mStateLock);



    // Only set by BLAST adapter layers

    if (what & layer_state_t::eProducerDisconnect) {
@@ -7304,6 +7305,7 @@ void SurfaceFlinger::handleLayerCreatedLocked(const LayerCreatedState& state) { 
        ALOGD("Layer was destroyed soon after creation %p", state.layer.unsafe_get());

        return;

    }

    MUTEX_ALIAS(mStateLock, layer->mFlinger->mStateLock);



    sp<Layer> parent;

    bool addToRoot = state.addToRoot;

services/surfaceflinger/SurfaceFlinger.h

+1 −1
Original line number Diff line number Diff line
@@ -859,7 +859,7 @@ private: 
    // this layer meaning it is entirely safe to destroy all

    // resources associated to this layer.

    void onHandleDestroyed(BBinder* handle, sp<Layer>& layer);

    void markLayerPendingRemovalLocked(const sp<Layer>& layer);

    void markLayerPendingRemovalLocked(const sp<Layer>& layer) REQUIRES(mStateLock);



    // add a layer to SurfaceFlinger

    status_t addClientLayer(const sp<Client>& client, const sp<IBinder>& handle,