From a8bf1c6e6d210833070152adf130f4d988fc255e Mon Sep 17 00:00:00 2001
From: Jayant Chowdhary <jchowdhary@google.com>
Date: Thu, 26 Sep 2019 08:50:17 -0700
Subject: [PATCH] camera2ndk: ~ACameraCaptureSession shouldn't hold device lock
 in ACameraDevice_close().

We call disconnectLocked before stopping CameraDevice's looper, in order to avoid this situation:

1) Its possible that an OnResultReceived callback is received, and posts an
   AMessage with sp<ACameraCaptureSession> on CameraDevice's looper.

2) Before the looper message is processsed, ACameraDevice_close() is
   called by the client, which results in the looper being stopped and
   cleared with device lock held.

3) When the looper is getting cleared, the AMessage containg the
   ACameraCaptureSession pointer is destructed leading to
   ~ACameraCaptureSession running without knowing that the device is
   being closed, as a result it tries to hold device lock, resulting in
   a deadlock.

Bug: 141603005

Test: CTS native tests
Test: use camera2 vndk client for extended periods of time

Change-Id: Ia0d47fc2975981055cd1f2103c1cbe8d76642fe4
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>
---
 camera/ndk/impl/ACameraDevice.cpp            | 24 ++++++++------------
 camera/ndk/impl/ACameraDevice.h              |  3 ++-
 camera/ndk/ndk_vendor/impl/ACameraDevice.cpp | 24 ++++++++------------
 camera/ndk/ndk_vendor/impl/ACameraDevice.h   |  3 ++-
 4 files changed, 22 insertions(+), 32 deletions(-)

diff --git a/camera/ndk/impl/ACameraDevice.cpp b/camera/ndk/impl/ACameraDevice.cpp
index d24cb814a3..46a8dae317 100644
--- a/camera/ndk/impl/ACameraDevice.cpp
+++ b/camera/ndk/impl/ACameraDevice.cpp
@@ -29,7 +29,7 @@
 #include "ACameraCaptureSession.inc"
 
 ACameraDevice::~ACameraDevice() {
-    mDevice->stopLooper();
+    mDevice->stopLooperAndDisconnect();
 }
 
 namespace android {
@@ -112,19 +112,7 @@ CameraDevice::CameraDevice(
     }
 }
 
-// Device close implementaiton
-CameraDevice::~CameraDevice() {
-    sp<ACameraCaptureSession> session = mCurrentSession.promote();
-    {
-        Mutex::Autolock _l(mDeviceLock);
-        if (!isClosed()) {
-            disconnectLocked(session);
-        }
-        LOG_ALWAYS_FATAL_IF(mCbLooper != nullptr,
-                "CameraDevice looper should've been stopped before ~CameraDevice");
-        mCurrentSession = nullptr;
-    }
-}
+CameraDevice::~CameraDevice() { }
 
 void
 CameraDevice::postSessionMsgAndCleanup(sp<AMessage>& msg) {
@@ -892,8 +880,14 @@ CameraDevice::onCaptureErrorLocked(
     return;
 }
 
-void CameraDevice::stopLooper() {
+void CameraDevice::stopLooperAndDisconnect() {
     Mutex::Autolock _l(mDeviceLock);
+    sp<ACameraCaptureSession> session = mCurrentSession.promote();
+    if (!isClosed()) {
+        disconnectLocked(session);
+    }
+    mCurrentSession = nullptr;
+
     if (mCbLooper != nullptr) {
       mCbLooper->unregisterHandler(mHandler->id());
       mCbLooper->stop();
diff --git a/camera/ndk/impl/ACameraDevice.h b/camera/ndk/impl/ACameraDevice.h
index 7a35bf0e81..6c2ceb300e 100644
--- a/camera/ndk/impl/ACameraDevice.h
+++ b/camera/ndk/impl/ACameraDevice.h
@@ -40,6 +40,7 @@
 
 #include <camera/NdkCameraManager.h>
 #include <camera/NdkCameraCaptureSession.h>
+
 #include "ACameraMetadata.h"
 
 namespace android {
@@ -110,7 +111,7 @@ class CameraDevice final : public RefBase {
     inline ACameraDevice* getWrapper() const { return mWrapper; };
 
     // Stop the looper thread and unregister the handler
-    void stopLooper();
+    void stopLooperAndDisconnect();
 
   private:
     friend ACameraCaptureSession;
diff --git a/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp b/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp
index 35c83551c2..e511a3f81d 100644
--- a/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp
+++ b/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp
@@ -45,7 +45,7 @@
 using namespace android;
 
 ACameraDevice::~ACameraDevice() {
-    mDevice->stopLooper();
+    mDevice->stopLooperAndDisconnect();
 }
 
 namespace android {
@@ -125,19 +125,7 @@ CameraDevice::CameraDevice(
     }
 }
 
-// Device close implementaiton
-CameraDevice::~CameraDevice() {
-    sp<ACameraCaptureSession> session = mCurrentSession.promote();
-    {
-        Mutex::Autolock _l(mDeviceLock);
-        if (!isClosed()) {
-            disconnectLocked(session);
-        }
-        mCurrentSession = nullptr;
-        LOG_ALWAYS_FATAL_IF(mCbLooper != nullptr,
-            "CameraDevice looper should've been stopped before ~CameraDevice");
-    }
-}
+CameraDevice::~CameraDevice() { }
 
 void
 CameraDevice::postSessionMsgAndCleanup(sp<AMessage>& msg) {
@@ -1388,6 +1376,7 @@ CameraDevice::checkAndFireSequenceCompleteLocked() {
             // before cbh goes out of scope and causing we call the session
             // destructor while holding device lock
             cbh.mSession.clear();
+
             postSessionMsgAndCleanup(msg);
         }
 
@@ -1400,8 +1389,13 @@ CameraDevice::checkAndFireSequenceCompleteLocked() {
     }
 }
 
-void CameraDevice::stopLooper() {
+void CameraDevice::stopLooperAndDisconnect() {
     Mutex::Autolock _l(mDeviceLock);
+    sp<ACameraCaptureSession> session = mCurrentSession.promote();
+    if (!isClosed()) {
+        disconnectLocked(session);
+    }
+    mCurrentSession = nullptr;
     if (mCbLooper != nullptr) {
       mCbLooper->unregisterHandler(mHandler->id());
       mCbLooper->stop();
diff --git a/camera/ndk/ndk_vendor/impl/ACameraDevice.h b/camera/ndk/ndk_vendor/impl/ACameraDevice.h
index 3328a85686..0b882eebe5 100644
--- a/camera/ndk/ndk_vendor/impl/ACameraDevice.h
+++ b/camera/ndk/ndk_vendor/impl/ACameraDevice.h
@@ -36,6 +36,7 @@
 
 #include <camera/NdkCameraManager.h>
 #include <camera/NdkCameraCaptureSession.h>
+
 #include "ACameraMetadata.h"
 #include "utils.h"
 
@@ -134,7 +135,7 @@ class CameraDevice final : public RefBase {
     inline ACameraDevice* getWrapper() const { return mWrapper; };
 
     // Stop the looper thread and unregister the handler
-    void stopLooper();
+    void stopLooperAndDisconnect();
 
   private:
     friend ACameraCaptureSession;