commit 0e24a8385a63be6a799da902e1d5ffcbb7519c2a
author Marissa Wall <marissaw@google.com> Wed Jul 10 15:32:50 2019 -0700
committer Marissa Wall <marissaw@google.com> Wed Jul 10 15:38:37 2019 -0700
tree 13c980bfbda1d330ac8b7cc3b2fba846e80f9862
parent ccf624a3045223c295a4db5a6f36a5d89ced4dbc

blast: fix leak on BufferStateLayer death

SurfaceFlinger can occasionally leak graphic buffers.

The leak happens when:
1) a transaction comes in and is placed in a queue
2) Chrome crashes
3) the parent layer is cleaned up
4) the child layer is told to release its buffer because it is
        no longer on screen
5) the transaction is applied with sets a callback handle on the
        layer which has a sp<> to the layer

To fix this, the callback handle should not have a sp<> to layer.
It is safe for the callback handle can have wp<> to the layer.
The client side has a sp<> so during normal operation, SurfaceFlinger
can promote the wp<>. The only time the promote will fail is if the
client side is dead. If the client side is dead, there is no one
to send a callback to so it doesn't matter if the promote fails.

Bug: 135951943
Test: https://buganizer.corp.google.com/issues/135951943#comment34
Change-Id: I756ace14c90b03a6499a3187d235b42d91cdd05a

services/surfaceflinger/TransactionCompletedThread.cpp

diff --git a/services/surfaceflinger/TransactionCompletedThread.cpp b/services/surfaceflinger/TransactionCompletedThread.cpp
index 5cf8eb1..fd466de 100644
--- a/services/surfaceflinger/TransactionCompletedThread.cpp
+++ b/services/surfaceflinger/TransactionCompletedThread.cpp
@@ -197,8 +197,14 @@
     }
 
     transactionStats->latchTime = handle->latchTime;
-    transactionStats->surfaceStats.emplace_back(handle->surfaceControl, handle->acquireTime,
-                                                handle->previousReleaseFence);
+    // If the layer has already been destroyed, don't add the SurfaceControl to the callback.
+    // The client side keeps a sp<> to the SurfaceControl so if the SurfaceControl has been
+    // destroyed the client side is dead and there won't be anyone to send the callback to.
+    sp<IBinder> surfaceControl = handle->surfaceControl.promote();
+    if (surfaceControl) {
+        transactionStats->surfaceStats.emplace_back(surfaceControl, handle->acquireTime,
+                                                    handle->previousReleaseFence);
+    }
     return NO_ERROR;
 }
 

services/surfaceflinger/TransactionCompletedThread.h

diff --git a/services/surfaceflinger/TransactionCompletedThread.h b/services/surfaceflinger/TransactionCompletedThread.h
index 21e2678..e849f71 100644
--- a/services/surfaceflinger/TransactionCompletedThread.h
+++ b/services/surfaceflinger/TransactionCompletedThread.h
@@ -49,7 +49,7 @@
 
     sp<ITransactionCompletedListener> listener;
     std::vector<CallbackId> callbackIds;
-    sp<IBinder> surfaceControl;
+    wp<IBinder> surfaceControl;
 
     bool releasePreviousBuffer = false;
     sp<Fence> previousReleaseFence;