SF: Fix mGraphicBufferProducerList
This change is primarily to fix a memory leak discovered while
investigating bug 73792507. It turned out that while we were adding
every new IGBP to SF's list (which it uses for authentication), we were
never removing anything from that list, causing us to leak wp<> items.
The root cause was some subtlety around MonitoredProducer, which wraps
a generic IGBP in order to allow SF to perform some work when one is
destroyed. When we were adding elements to mGBPL, we were adding the
address of the MonitoredProducer, but upon destruction, the
MonitoredProducer was trying to remove the address of its wrapped IGBP,
which, naturally, wasn't present in the list.
In order to address this, the key functional change here is to pass the
IBinder address of the MonitoredProducer from its destructor rather
than that of the wrapped IGBP.
On top of the bug fix, however, this also switches from a custom
MessageBase-derived class to LambdaMessage and converts mGBPL from a
SortedVector to a std::set.
Bug: 73792507
Test: Manual - log lines to verify mGBPL no longer increases over time
Change-Id: Idabae211354561a0f13c8d9e594c7acc4822aab0
diff --git a/services/surfaceflinger/MonitoredProducer.cpp b/services/surfaceflinger/MonitoredProducer.cpp
index 1a5a85e..389fbd2 100644
--- a/services/surfaceflinger/MonitoredProducer.cpp
+++ b/services/surfaceflinger/MonitoredProducer.cpp
@@ -33,26 +33,13 @@
// because we don't know where this destructor is called from. It could be
// called with the mStateLock held, leading to a dead-lock (it actually
// happens).
- class MessageCleanUpList : public MessageBase {
- public:
- MessageCleanUpList(const sp<SurfaceFlinger>& flinger,
- const wp<IBinder>& producer)
- : mFlinger(flinger), mProducer(producer) {}
+ sp<LambdaMessage> cleanUpListMessage =
+ new LambdaMessage([flinger = mFlinger, asBinder = wp<IBinder>(onAsBinder())]() {
+ Mutex::Autolock lock(flinger->mStateLock);
+ flinger->mGraphicBufferProducerList.erase(asBinder);
+ });
- virtual ~MessageCleanUpList() {}
-
- virtual bool handler() {
- Mutex::Autolock _l(mFlinger->mStateLock);
- mFlinger->mGraphicBufferProducerList.remove(mProducer);
- return true;
- }
-
- private:
- sp<SurfaceFlinger> mFlinger;
- wp<IBinder> mProducer;
- };
-
- mFlinger->postMessageAsync(new MessageCleanUpList(mFlinger, asBinder(mProducer)));
+ mFlinger->postMessageAsync(cleanUpListMessage);
}
status_t MonitoredProducer::requestBuffer(int slot, sp<GraphicBuffer>* buf) {
diff --git a/services/surfaceflinger/SurfaceFlinger.cpp b/services/surfaceflinger/SurfaceFlinger.cpp
index ea43c58..4d7e4c8 100644
--- a/services/surfaceflinger/SurfaceFlinger.cpp
+++ b/services/surfaceflinger/SurfaceFlinger.cpp
@@ -701,7 +701,7 @@
bool SurfaceFlinger::authenticateSurfaceTextureLocked(
const sp<IGraphicBufferProducer>& bufferProducer) const {
sp<IBinder> surfaceTextureBinder(IInterface::asBinder(bufferProducer));
- return mGraphicBufferProducerList.indexOf(surfaceTextureBinder) >= 0;
+ return mGraphicBufferProducerList.count(surfaceTextureBinder.get()) > 0;
}
status_t SurfaceFlinger::getSupportedFrameTimestamps(
@@ -2893,7 +2893,9 @@
parent->addChild(lbc);
}
- mGraphicBufferProducerList.add(IInterface::asBinder(gbc));
+ mGraphicBufferProducerList.insert(IInterface::asBinder(gbc).get());
+ LOG_ALWAYS_FATAL_IF(mGraphicBufferProducerList.size() > MAX_LAYERS,
+ "Suspected IGBP leak");
mLayersAdded = true;
mNumLayers++;
}
diff --git a/services/surfaceflinger/SurfaceFlinger.h b/services/surfaceflinger/SurfaceFlinger.h
index 6da1409..b774d49 100644
--- a/services/surfaceflinger/SurfaceFlinger.h
+++ b/services/surfaceflinger/SurfaceFlinger.h
@@ -715,7 +715,9 @@
bool mTransactionPending;
bool mAnimTransactionPending;
SortedVector< sp<Layer> > mLayersPendingRemoval;
- SortedVector< wp<IBinder> > mGraphicBufferProducerList;
+
+ // Can't be unordered_set because wp<> isn't hashable
+ std::set<wp<IBinder>> mGraphicBufferProducerList;
// protected by mStateLock (but we could use another lock)
bool mLayersRemoved;