commit 44d5f626b449430f97243e2bb8b4ec41dfae6ff7
author Jayant Chowdhary <jchowdhary@google.com> Wed Sep 20 03:11:41 2023 +0000
committer Jayant Chowdhary <jchowdhary@google.com> Thu Sep 21 00:09:46 2023 +0000
tree 57e03a763bc999b081126fabe26b0e72e5d99717
parent 35b1ff9298facd6aaa38af7deb86837585b41901

Fix deadlock when notifyPhysicalCameraDevice is called while waitUntilDrained hasn't completed

The following situation is possible :

thread 1: handling waitUntilIdle from client holds Camera3Device::mInterfaceLock and waits for the
    HAL to drain all results.
    android::Camera3Device::waitUntilStateThenRelock()
    android::Camera3Device::waitUntilDrainedLocked()
    android::Camera3Device::waitUntilDrained()
    android::CameraDeviceClient::waitUntilIdle()

thread 2: handling processCaptureResult callback from the HAL waits on Camera3Device::mInterfaceLock
    NonPI::MutexLockWithTimeout()
    android::Camera3Device::setRotateAndCropAutoBehavior()
    android::Camera2ClientBase<android::CameraDeviceClientBase>::notifyPhysicalCameraChange()
    android::camera3::processCaptureResult()

setRotateAndCropAutoBehaviour() in this case, is an effect of a HAL
callback and shouldn't hold mInterfaceLock. Since mLock already protects
the rotate and crop state we can do without holding mInterfaceLock when
called from processCaptureResult.

Bug: 298705937
Bug: 299348355

Test: GCA (basic validity)
Test: Camera CTS
Test: trigger notifyPhysicalCameraDevice from processCaptureResult while
      Camera3Device::waitUntilDrained() is still executing, no deadlock
      observed.

Change-Id: If6a89ebc7d38510eece00dfbee62af01b5b5b065
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>

services/camera/libcameraservice/device3/Camera3Device.cpp

diff --git a/services/camera/libcameraservice/device3/Camera3Device.cpp b/services/camera/libcameraservice/device3/Camera3Device.cpp
index 71e49fd..312f5ba 100644
--- a/services/camera/libcameraservice/device3/Camera3Device.cpp
+++ b/services/camera/libcameraservice/device3/Camera3Device.cpp
@@ -67,6 +67,7 @@
 #include "utils/TraceHFR.h"
 
 #include <algorithm>
+#include <optional>
 #include <tuple>
 
 using namespace android::camera3;
@@ -5396,9 +5397,13 @@
 }
 
 status_t Camera3Device::setRotateAndCropAutoBehavior(
-    camera_metadata_enum_android_scaler_rotate_and_crop_t rotateAndCropValue) {
+    camera_metadata_enum_android_scaler_rotate_and_crop_t rotateAndCropValue, bool fromHal) {
     ATRACE_CALL();
-    Mutex::Autolock il(mInterfaceLock);
+    // We shouldn't hold mInterfaceLock when called as an effect of a HAL
+    // callback since this can lead to a deadlock : b/299348355.
+    // mLock still protects state.
+    std::optional<Mutex::Autolock> maybeMutex =
+        fromHal ? std::nullopt : std::optional<Mutex::Autolock>(mInterfaceLock);
     Mutex::Autolock l(mLock);
     if (mRequestThread == nullptr) {
         return INVALID_OPERATION;

services/camera/libcameraservice/device3/Camera3Device.h

diff --git a/services/camera/libcameraservice/device3/Camera3Device.h b/services/camera/libcameraservice/device3/Camera3Device.h
index aa1d55a..9180a10 100644
--- a/services/camera/libcameraservice/device3/Camera3Device.h
+++ b/services/camera/libcameraservice/device3/Camera3Device.h
@@ -275,7 +275,7 @@
      * and defaults to NONE.
      */
     status_t setRotateAndCropAutoBehavior(
-            camera_metadata_enum_android_scaler_rotate_and_crop_t rotateAndCropValue);
+            camera_metadata_enum_android_scaler_rotate_and_crop_t rotateAndCropValue, bool fromHal);
 
     /**
      * Set the current behavior for the AUTOFRAMING control when in AUTO.