From bc67a6a8fbcbf0c2622818523f167554613206fc Mon Sep 17 00:00:00 2001
From: Steven Thomas <steventhomas@google.com>
Date: Fri, 7 Jul 2017 12:34:20 -0700
Subject: [PATCH] Guard against racy ComposerClient reconnection

The hardware composer service has a rule that only one client can be
connected at a time. The surface flinger process, when transitioning
composer ownership from surface flinger to vr flinger, will destroy the
current client on one thread and create a new client on another
thread. Although surface flinger ensures that these events happen in the
expected sequence (delete then create), the requests sometimes land in
the hardware composer service in inverted order, causing the creation
request to fail with an error.

Instead of failing with an error, block for a brief period (1 second)
until the existing client is removed, then proceed to initialize the new
client. This gives us enough time to ensure an inverted
creation/destruction order doesn't cause client creation to fail, while
avoiding a deadlock if the existing client is never destroyed.

Bug: 62925812

Test: - Transitioned to/from vr flinger hundreds of times, and confirmed
I no longer see sporadic composer client creation failure due to an
already existing client.

- Ran the vts graphics composer tests and confirmed they all pass.

Change-Id: I40be1fb0cb3d42ddb5a9fc159188886e9f5b6267
---
 graphics/composer/2.1/default/Hwc.cpp | 23 ++++++++++++++++++++++-
 graphics/composer/2.1/default/Hwc.h   |  4 +++-
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/graphics/composer/2.1/default/Hwc.cpp b/graphics/composer/2.1/default/Hwc.cpp
index 8ca0eb3351..862dff138e 100644
--- a/graphics/composer/2.1/default/Hwc.cpp
+++ b/graphics/composer/2.1/default/Hwc.cpp
@@ -18,6 +18,7 @@
 
 #include "Hwc.h"
 
+#include <chrono>
 #include <type_traits>
 #include <log/log.h>
 
@@ -25,6 +26,8 @@
 #include "hardware/hwcomposer.h"
 #include "hwc2on1adapter/HWC2On1Adapter.h"
 
+using namespace std::chrono_literals;
+
 namespace android {
 namespace hardware {
 namespace graphics {
@@ -218,7 +221,24 @@ Return<void> HwcHal::createClient(createClient_cb hidl_cb)
     sp<ComposerClient> client;
 
     {
-        std::lock_guard<std::mutex> lock(mClientMutex);
+        std::unique_lock<std::mutex> lock(mClientMutex);
+
+        if (mClient != nullptr) {
+            // In surface flinger we delete a composer client on one thread and
+            // then create a new client on another thread. Although surface
+            // flinger ensures the calls are made in that sequence (destroy and
+            // then create), sometimes the calls land in the composer service
+            // inverted (create and then destroy). Wait for a brief period to
+            // see if the existing client is destroyed.
+            ALOGI("HwcHal::createClient: Client already exists. Waiting for"
+                    " it to be destroyed.");
+            mClientDestroyedWait.wait_for(lock, 1s,
+                    [this] { return mClient == nullptr; });
+            std::string doneMsg = mClient == nullptr ?
+                    "Existing client was destroyed." :
+                    "Existing client was never destroyed!";
+            ALOGI("HwcHal::createClient: Done waiting. %s", doneMsg.c_str());
+        }
 
         // only one client is allowed
         if (mClient == nullptr) {
@@ -245,6 +265,7 @@ void HwcHal::removeClient()
 {
     std::lock_guard<std::mutex> lock(mClientMutex);
     mClient = nullptr;
+    mClientDestroyedWait.notify_all();
 }
 
 void HwcHal::hotplugHook(hwc2_callback_data_t callbackData,
diff --git a/graphics/composer/2.1/default/Hwc.h b/graphics/composer/2.1/default/Hwc.h
index b45389ad82..756132759c 100644
--- a/graphics/composer/2.1/default/Hwc.h
+++ b/graphics/composer/2.1/default/Hwc.h
@@ -17,8 +17,9 @@
 #ifndef ANDROID_HARDWARE_GRAPHICS_COMPOSER_V2_1_HWC_H
 #define ANDROID_HARDWARE_GRAPHICS_COMPOSER_V2_1_HWC_H
 
-#include <mutex>
+#include <condition_variable>
 #include <memory>
+#include <mutex>
 #include <unordered_set>
 #include <vector>
 
@@ -211,6 +212,7 @@ private:
     } mDispatch;
 
     std::mutex mClientMutex;
+    std::condition_variable mClientDestroyedWait;
     wp<ComposerClient> mClient;
 
     // If the HWC implementation version is < 2.0, use an adapter to interface