cameraserver: fix deadlock scenario in torchModeStatusChanged callback.
The following scenario can occur:
T1: CameraService::connectDevice()
CameraService::connectDeviceHelper()
CameraProviderManager::openSession() ---> holds mInterfaceLock
.
.
. on the same thread before openSession execution completes
CameraProviderManager::ProviderInfo::torchModeStatusChange() callback from HAL
.
CameraService::onTorchStatusChanged()
CameraProviderManager::getSystemCameraKind tries to lock mInterfaceLock -> deadlock.
We now pass in system camera kind to onTorchStatusChanged in
CameraProviderManager::torchModeStatusChange() instead of calling getSystemCameraKind
This CL also removes CameraProviderManager::mStatusListenerMutex, since
it wasn't protecting any data structure.
Bug: 202198748
Test: camera CTS, GCA (basic validity)
Merged-In: Id95a2aa061b6cb4db4a25b1a2aa6a390f898af87
Change-Id: Id95a2aa061b6cb4db4a25b1a2aa6a390f898af87
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>
diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp
index b5de1b7..ce2bec0 100644
--- a/services/camera/libcameraservice/CameraService.cpp
+++ b/services/camera/libcameraservice/CameraService.cpp
@@ -506,6 +506,13 @@
onTorchStatusChangedLocked(cameraId, newStatus);
}
+
+void CameraService::onTorchStatusChanged(const String8& cameraId,
+ TorchModeStatus newStatus, SystemCameraKind systemCameraKind) {
+ Mutex::Autolock al(mTorchStatusMutex);
+ onTorchStatusChangedLocked(cameraId, newStatus, systemCameraKind);
+}
+
void CameraService::onTorchStatusChangedLocked(const String8& cameraId,
TorchModeStatus newStatus) {
ALOGI("%s: Torch status changed for cameraId=%s, newStatus=%d",
diff --git a/services/camera/libcameraservice/CameraService.h b/services/camera/libcameraservice/CameraService.h
index 35e13e7..69d44e2 100644
--- a/services/camera/libcameraservice/CameraService.h
+++ b/services/camera/libcameraservice/CameraService.h
@@ -109,8 +109,16 @@
virtual void onDeviceStatusChanged(const String8 &cameraId,
const String8 &physicalCameraId,
hardware::camera::common::V1_0::CameraDeviceStatus newHalStatus) override;
+ // This method may hold CameraProviderManager::mInterfaceMutex as a part
+ // of calling getSystemCameraKind() internally. Care should be taken not to
+ // directly / indirectly call this from callers who also hold
+ // mInterfaceMutex.
virtual void onTorchStatusChanged(const String8& cameraId,
hardware::camera::common::V1_0::TorchModeStatus newStatus) override;
+ // Does not hold CameraProviderManager::mInterfaceMutex.
+ virtual void onTorchStatusChanged(const String8& cameraId,
+ hardware::camera::common::V1_0::TorchModeStatus newStatus,
+ SystemCameraKind kind) override;
virtual void onNewProviderRegistered() override;
/////////////////////////////////////////////////////////////////////
diff --git a/services/camera/libcameraservice/common/CameraProviderManager.cpp b/services/camera/libcameraservice/common/CameraProviderManager.cpp
index 32d118d..fdbf32b 100644
--- a/services/camera/libcameraservice/common/CameraProviderManager.cpp
+++ b/services/camera/libcameraservice/common/CameraProviderManager.cpp
@@ -1836,16 +1836,19 @@
const hardware::hidl_string& cameraDeviceName,
TorchModeStatus newStatus) {
sp<StatusListener> listener;
+ SystemCameraKind systemCameraKind = SystemCameraKind::PUBLIC;
std::string id;
+ bool known = false;
{
- std::lock_guard<std::mutex> lock(mManager->mStatusListenerMutex);
- bool known = false;
+ // Hold mLock for accessing mDevices
+ std::lock_guard<std::mutex> lock(mLock);
for (auto& deviceInfo : mDevices) {
if (deviceInfo->mName == cameraDeviceName) {
ALOGI("Camera device %s torch status is now %s", cameraDeviceName.c_str(),
torchStatusToString(newStatus));
id = deviceInfo->mId;
known = true;
+ systemCameraKind = deviceInfo->mSystemCameraKind;
if (TorchModeStatus::AVAILABLE_ON != newStatus) {
mManager->removeRef(DeviceMode::TORCH, id);
}
@@ -1857,11 +1860,19 @@
mProviderName.c_str(), cameraDeviceName.c_str(), newStatus);
return hardware::Void();
}
+ // no lock needed since listener is set up only once during
+ // CameraProviderManager initialization and then never changed till it is
+ // destructed.
listener = mManager->getStatusListener();
- }
+ }
// Call without lock held to allow reentrancy into provider manager
+ // The problem with holding mLock here is that we
+ // might be limiting re-entrancy : CameraService::onTorchStatusChanged calls
+ // back into CameraProviderManager which might try to hold mLock again (eg:
+ // findDeviceInfo, which should be holding mLock while iterating through
+ // each provider's devices).
if (listener != nullptr) {
- listener->onTorchStatusChanged(String8(id.c_str()), newStatus);
+ listener->onTorchStatusChanged(String8(id.c_str()), newStatus, systemCameraKind);
}
return hardware::Void();
}
diff --git a/services/camera/libcameraservice/common/CameraProviderManager.h b/services/camera/libcameraservice/common/CameraProviderManager.h
index 25d3639..c9e0a33 100644
--- a/services/camera/libcameraservice/common/CameraProviderManager.h
+++ b/services/camera/libcameraservice/common/CameraProviderManager.h
@@ -143,6 +143,9 @@
const String8 &physicalCameraId,
hardware::camera::common::V1_0::CameraDeviceStatus newStatus) = 0;
virtual void onTorchStatusChanged(const String8 &cameraId,
+ hardware::camera::common::V1_0::TorchModeStatus newStatus,
+ SystemCameraKind kind) = 0;
+ virtual void onTorchStatusChanged(const String8 &cameraId,
hardware::camera::common::V1_0::TorchModeStatus newStatus) = 0;
virtual void onNewProviderRegistered() = 0;
};
@@ -325,8 +328,6 @@
// All private members, unless otherwise noted, expect mInterfaceMutex to be locked before use
mutable std::mutex mInterfaceMutex;
- // the status listener update callbacks will lock mStatusMutex
- mutable std::mutex mStatusListenerMutex;
wp<StatusListener> mListener;
ServiceInteractionProxy* mServiceProxy;
diff --git a/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp b/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp
index 855b5ab..face4a1 100644
--- a/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp
+++ b/services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp
@@ -253,6 +253,8 @@
hardware::camera::common::V1_0::CameraDeviceStatus) override {}
void onTorchStatusChanged(const String8 &,
hardware::camera::common::V1_0::TorchModeStatus) override {}
+ void onTorchStatusChanged(const String8 &,
+ hardware::camera::common::V1_0::TorchModeStatus, SystemCameraKind) override {}
void onNewProviderRegistered() override {}
};