commit f0baa24d4333642c55e38200b80474134d2df748
author John Reck <jreck@google.com> Thu May 13 18:14:45 2021 -0400
committer John Reck <jreck@google.com> Thu May 13 18:15:50 2021 -0400
tree d0afcfa43beb6c434fa9448701c5884b1dbfde5a
parent c49b886d7316354cede2b9510777882bc4c8598e

Fix some FD lifecycle issues

Use unique_fd, it's cool

Bug: 186919605
Test: make, guessing from a pretty clear error message & documentation
Change-Id: Ied546565261708b7e4c2d220dce6d6a89725cc06

libs/hwui/renderthread/VulkanSurface.cpp

diff --git a/libs/hwui/renderthread/VulkanSurface.cpp b/libs/hwui/renderthread/VulkanSurface.cpp
index 474d2cc..c559425 100644
--- a/libs/hwui/renderthread/VulkanSurface.cpp
+++ b/libs/hwui/renderthread/VulkanSurface.cpp
@@ -329,20 +329,16 @@
 
         if (bufferInfo.buffer.get() != nullptr && bufferInfo.dequeued) {
             int err = mNativeWindow->cancelBuffer(mNativeWindow.get(), bufferInfo.buffer.get(),
-                                                  bufferInfo.dequeue_fence);
+                                                  bufferInfo.dequeue_fence.release());
             if (err != 0) {
                 ALOGE("cancelBuffer[%u] failed during destroy: %s (%d)", i, strerror(-err), err);
             }
             bufferInfo.dequeued = false;
-
-            if (bufferInfo.dequeue_fence >= 0) {
-                close(bufferInfo.dequeue_fence);
-                bufferInfo.dequeue_fence = -1;
-            }
+            bufferInfo.dequeue_fence.reset();
         }
 
         LOG_ALWAYS_FATAL_IF(bufferInfo.dequeued);
-        LOG_ALWAYS_FATAL_IF(bufferInfo.dequeue_fence != -1);
+        LOG_ALWAYS_FATAL_IF(bufferInfo.dequeue_fence.ok());
 
         bufferInfo.skSurface.reset();
         bufferInfo.buffer.clear();
@@ -365,8 +361,12 @@
     // Since auto pre-rotation is enabled, dequeueBuffer to get the consumer driven buffer size
     // from ANativeWindowBuffer.
     ANativeWindowBuffer* buffer;
-    int fence_fd;
-    err = mNativeWindow->dequeueBuffer(mNativeWindow.get(), &buffer, &fence_fd);
+    base::unique_fd fence_fd;
+    {
+        int rawFd = -1;
+        err = mNativeWindow->dequeueBuffer(mNativeWindow.get(), &buffer, &rawFd);
+        fence_fd.reset(rawFd);
+    }
     if (err != 0) {
         ALOGE("dequeueBuffer failed: %s (%d)", strerror(-err), err);
         return nullptr;
@@ -387,7 +387,7 @@
             if (err != 0) {
                 ALOGE("native_window_set_buffers_transform(%d) failed: %s (%d)", transformHint,
                       strerror(-err), err);
-                mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd);
+                mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd.release());
                 return nullptr;
             }
             mWindowInfo.transform = transformHint;
@@ -405,19 +405,19 @@
     for (idx = 0; idx < mWindowInfo.bufferCount; idx++) {
         if (mNativeBuffers[idx].buffer.get() == buffer) {
             mNativeBuffers[idx].dequeued = true;
-            mNativeBuffers[idx].dequeue_fence = fence_fd;
+            mNativeBuffers[idx].dequeue_fence = std::move(fence_fd);
             break;
         } else if (mNativeBuffers[idx].buffer.get() == nullptr) {
             // increasing the number of buffers we have allocated
             mNativeBuffers[idx].buffer = buffer;
             mNativeBuffers[idx].dequeued = true;
-            mNativeBuffers[idx].dequeue_fence = fence_fd;
+            mNativeBuffers[idx].dequeue_fence = std::move(fence_fd);
             break;
         }
     }
     if (idx == mWindowInfo.bufferCount) {
         ALOGE("dequeueBuffer returned unrecognized buffer");
-        mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd);
+        mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd.release());
         return nullptr;
     }
 
@@ -429,7 +429,7 @@
                 kTopLeft_GrSurfaceOrigin, mWindowInfo.colorspace, nullptr);
         if (bufferInfo->skSurface.get() == nullptr) {
             ALOGE("SkSurface::MakeFromAHardwareBuffer failed");
-            mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd);
+            mNativeWindow->cancelBuffer(mNativeWindow.get(), buffer, fence_fd.release());
             return nullptr;
         }
     }
@@ -460,25 +460,23 @@
 
     LOG_ALWAYS_FATAL_IF(!mCurrentBufferInfo);
     VulkanSurface::NativeBufferInfo& currentBuffer = *mCurrentBufferInfo;
-    int queuedFd = (semaphoreFd != -1) ? semaphoreFd : currentBuffer.dequeue_fence;
+    // queueBuffer always closes fence, even on error
+    int queuedFd = (semaphoreFd != -1) ? semaphoreFd : currentBuffer.dequeue_fence.release();
     int err = mNativeWindow->queueBuffer(mNativeWindow.get(), currentBuffer.buffer.get(), queuedFd);
 
     currentBuffer.dequeued = false;
-    // queueBuffer always closes fence, even on error
     if (err != 0) {
         ALOGE("queueBuffer failed: %s (%d)", strerror(-err), err);
+        // cancelBuffer takes ownership of the fence
         mNativeWindow->cancelBuffer(mNativeWindow.get(), currentBuffer.buffer.get(),
-                                    currentBuffer.dequeue_fence);
+                                    currentBuffer.dequeue_fence.release());
     } else {
         currentBuffer.hasValidContents = true;
         currentBuffer.lastPresentedCount = mPresentCount;
         mPresentCount++;
     }
 
-    if (currentBuffer.dequeue_fence >= 0) {
-        close(currentBuffer.dequeue_fence);
-        currentBuffer.dequeue_fence = -1;
-    }
+    currentBuffer.dequeue_fence.reset();
 
     return err == 0;
 }