Surface: Release references to BlastBufferQueue and SurfaceControl on Surface#destroy

SurfaceView clients may hold on to surface references. In S this means
they would extend the lifetime of the SurfaceControl resulting in
"leaking" buffers until the references are cleared or the app is
terminated.

Fix this by calling a new destroy function on Surface which will
explicitly remove references to the SurfaceControl and BBQ it holds.
This is safe because SurfaceView controls the lifecycle of the Surface
and knows when the Surface will become invalid. Once invalid, the Surface
cannot become valid again.

Test: repro steps in bug
Bug: 198133921
Change-Id: I88fe98fa275dd76e20a5403c24bd2cb3bee5346d
Merged-In: I88fe98fa275dd76e20a5403c24bd2cb3bee5346d
(cherry picked from commit fe4d05cd44be3ee805c14c49e59b88d8cf67c4a1)

3 files changed, 31 insertions, 1 deletions

diff --git a/libs/gui/BLASTBufferQueue.cpp b/libs/gui/BLASTBufferQueue.cpp
index 56a9683773..f7ec8ef40e 100644
--- a/libs/gui/BLASTBufferQueue.cpp
+++ b/libs/gui/BLASTBufferQueue.cpp
@@ -630,7 +630,10 @@ bool BLASTBufferQueue::maxBuffersAcquired(bool includeExtraAcquire) const {
 
 class BBQSurface : public Surface {
 private:
+    std::mutex mMutex;
     sp<BLASTBufferQueue> mBbq;
+    bool mDestroyed = false;
+
 public:
     BBQSurface(const sp<IGraphicBufferProducer>& igbp, bool controlledByApp,
                const sp<IBinder>& scHandle, const sp<BLASTBufferQueue>& bbq)
@@ -650,6 +653,10 @@ public:
 
     status_t setFrameRate(float frameRate, int8_t compatibility,
                           int8_t changeFrameRateStrategy) override {
+        std::unique_lock _lock{mMutex};
+        if (mDestroyed) {
+            return DEAD_OBJECT;
+        }
         if (!ValidateFrameRate(frameRate, compatibility, changeFrameRateStrategy,
                                "BBQSurface::setFrameRate")) {
             return BAD_VALUE;
@@ -658,8 +665,20 @@ public:
     }
 
     status_t setFrameTimelineInfo(const FrameTimelineInfo& frameTimelineInfo) override {
+        std::unique_lock _lock{mMutex};
+        if (mDestroyed) {
+            return DEAD_OBJECT;
+        }
         return mBbq->setFrameTimelineInfo(frameTimelineInfo);
     }
+
+    void destroy() override {
+        Surface::destroy();
+
+        std::unique_lock _lock{mMutex};
+        mDestroyed = true;
+        mBbq = nullptr;
+    }
 };
 
 // TODO: Can we coalesce this with frame updates? Need to confirm
diff --git a/libs/gui/Surface.cpp b/libs/gui/Surface.cpp
index 2edb4e4ba4..353a91d062 100644
--- a/libs/gui/Surface.cpp
+++ b/libs/gui/Surface.cpp
@@ -2622,4 +2622,14 @@ status_t Surface::setFrameTimelineInfo(const FrameTimelineInfo& frameTimelineInf
     return composerService()->setFrameTimelineInfo(mGraphicBufferProducer, frameTimelineInfo);
 }
 
+sp<IBinder> Surface::getSurfaceControlHandle() const {
+    Mutex::Autolock lock(mMutex);
+    return mSurfaceControlHandle;
+}
+
+void Surface::destroy() {
+    Mutex::Autolock lock(mMutex);
+    mSurfaceControlHandle = nullptr;
+}
+
 }; // namespace android
diff --git a/libs/gui/include/gui/Surface.h b/libs/gui/include/gui/Surface.h
index 7e4143b1f3..e5403512a9 100644
--- a/libs/gui/include/gui/Surface.h
+++ b/libs/gui/include/gui/Surface.h
@@ -99,7 +99,7 @@ public:
      */
     sp<IGraphicBufferProducer> getIGraphicBufferProducer() const;
 
-    sp<IBinder> getSurfaceControlHandle() const { return mSurfaceControlHandle; }
+    sp<IBinder> getSurfaceControlHandle() const;
 
     /* convenience function to check that the given surface is non NULL as
      * well as its IGraphicBufferProducer */
@@ -333,6 +333,7 @@ public:
     virtual int connect(
             int api, bool reportBufferRemoval,
             const sp<SurfaceListener>& sListener);
+    virtual void destroy();
 
     // When client connects to Surface with reportBufferRemoval set to true, any buffers removed
     // from this Surface will be collected and returned here. Once this method returns, these