VCD: Guard all use cases of |camera_info_| in CameraHalDelegate

There are some use cases which are not guarded by the lock, which might
cause race condition since |camera_info_| might be accessed from
different threads.

In addition, when VendorTagOpsDelegate is reset, we should have a way
to let caller know that the initialization state is reset so that we
won't wait forever.

Bug: b/174126439
Test: Build successfully
Change-Id: I270c0989f04896b1fc3df28aa2243dce799b93ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2602964Reviewed-by: Shik Chen <shik@chromium.org>
Commit-Queue: Shik Chen <shik@chromium.org>
Auto-Submit: Wei Lee <wtlee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841431}

media/capture/video/chromeos/camera_hal_delegate.cc

@@ -221,12 +221,10 @@ std::unique_ptr<VideoCaptureDevice> CameraHalDelegate::CreateDevice(
 }
 
 void CameraHalDelegate::GetSupportedFormats(
-    int camera_id,
+    const cros::mojom::CameraInfoPtr& camera_info,
     VideoCaptureFormats* supported_formats) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  const cros::mojom::CameraInfoPtr& camera_info = camera_info_[camera_id];
-
   base::flat_set<int32_t> candidate_fps_set =
       GetAvailableFramerates(camera_info);
 
@@ -371,7 +369,8 @@ void CameraHalDelegate::GetDevicesInfo(
       desc.set_control_support(GetControlSupport(camera_info));
       device_id_to_camera_id_[desc.device_id] = camera_id;
       devices_info.emplace_back(desc);
-      GetSupportedFormats(camera_id, &devices_info.back().supported_formats);
+      GetSupportedFormats(camera_info_[camera_id],
+                          &devices_info.back().supported_formats);
     }
   }
 
@@ -490,6 +489,7 @@ void CameraHalDelegate::ResetMojoInterfaceOnIpcThread() {
   external_camera_info_updated_.Signal();
 
   // Clear all cached camera info, especially external cameras.
+  base::AutoLock lock(camera_info_lock_);
   camera_info_.clear();
   pending_external_camera_info_.clear();
 }
@@ -523,6 +523,8 @@ void CameraHalDelegate::UpdateBuiltInCameraInfoOnIpcThread() {
 
 void CameraHalDelegate::OnGotNumberOfCamerasOnIpcThread(int32_t num_cameras) {
   DCHECK(ipc_task_runner_->BelongsToCurrentThread());
+
+  base::AutoLock lock(camera_info_lock_);
   if (num_cameras < 0) {
     builtin_camera_info_updated_.Signal();
     LOG(ERROR) << "Failed to get number of cameras: " << num_cameras;
@@ -544,6 +546,8 @@ void CameraHalDelegate::OnGotNumberOfCamerasOnIpcThread(int32_t num_cameras) {
 
 void CameraHalDelegate::OnSetCallbacksOnIpcThread(int32_t result) {
   DCHECK(ipc_task_runner_->BelongsToCurrentThread());
+
+  base::AutoLock lock(camera_info_lock_);
   if (result) {
     num_builtin_cameras_ = 0;
     builtin_camera_info_updated_.Signal();
@@ -595,6 +599,7 @@ void CameraHalDelegate::OnGotCameraInfoOnIpcThread(
     // |camera_info_| might contain some entries for external cameras as well,
     // we should check all built-in cameras explicitly.
     bool all_updated = [&]() {
+      camera_info_lock_.AssertAcquired();
       for (size_t i = 0; i < num_builtin_cameras_; i++) {
         if (camera_info_.find(i) == camera_info_.end()) {
           return false;

media/capture/video/chromeos/camera_hal_delegate.h

@@ -98,7 +98,7 @@ class CAPTURE_EXPORT CameraHalDelegate final
 
   void OnRegisteredCameraHalClient(int32_t result);
 
-  void GetSupportedFormats(int camera_id,
+  void GetSupportedFormats(const cros::mojom::CameraInfoPtr& camera_info,
                            VideoCaptureFormats* supported_formats);
 
   void SetCameraModuleOnIpcThread(
@@ -174,9 +174,10 @@ class CAPTURE_EXPORT CameraHalDelegate final
   // conditions. For external cameras, the |camera_info_| would be read nad
   // updated in CameraDeviceStatusChange, which is also protected by
   // |camera_info_lock_|.
-  size_t num_builtin_cameras_;
   base::Lock camera_info_lock_;
-  std::unordered_map<int, cros::mojom::CameraInfoPtr> camera_info_;
+  size_t num_builtin_cameras_ GUARDED_BY(camera_info_lock_);
+  std::unordered_map<int, cros::mojom::CameraInfoPtr> camera_info_
+      GUARDED_BY(camera_info_lock_);
 
   // A map from |VideoCaptureDeviceDescriptor.device_id| to camera id, which is
   // updated in GetDeviceDescriptors() and queried in

media/capture/video/chromeos/vendor_tag_ops_delegate.cc

@@ -13,7 +13,7 @@ namespace media {
 
 VendorTagOpsDelegate::VendorTagOpsDelegate(
     scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner)
-    : ipc_task_runner_(ipc_task_runner) {}
+    : ipc_task_runner_(ipc_task_runner), is_initializing_(false) {}
 
 VendorTagOpsDelegate::~VendorTagOpsDelegate() = default;
 
@@ -28,17 +28,29 @@ VendorTagOpsDelegate::MakeReceiver() {
 
 void VendorTagOpsDelegate::Initialize() {
   DCHECK(ipc_task_runner_->RunsTasksInCurrentSequence());
+
+  base::AutoLock lock(lock_);
+  is_initializing_ = true;
   vendor_tag_ops_->GetTagCount(base::BindOnce(
       &VendorTagOpsDelegate::OnGotTagCount, base::Unretained(this)));
 }
 
 void VendorTagOpsDelegate::Reset() {
   DCHECK(ipc_task_runner_->RunsTasksInCurrentSequence());
+
+  base::AutoLock lock(lock_);
   vendor_tag_ops_.reset();
   pending_info_.clear();
   name_map_.clear();
   tag_map_.clear();
   initialized_.Reset();
+  is_initializing_ = false;
+}
+
+void VendorTagOpsDelegate::StopInitialization() {
+  base::AutoLock lock(lock_);
+  initialized_.Signal();
+  is_initializing_ = false;
 }
 
 void VendorTagOpsDelegate::RemovePending(uint32_t tag) {
@@ -47,7 +59,7 @@ void VendorTagOpsDelegate::RemovePending(uint32_t tag) {
   DCHECK_EQ(removed, 1u);
   if (pending_info_.empty()) {
     DVLOG(1) << "VendorTagOpsDelegate initialized";
-    initialized_.Signal();
+    StopInitialization();
   }
 }
 
@@ -55,13 +67,13 @@ void VendorTagOpsDelegate::OnGotTagCount(int32_t tag_count) {
   DCHECK(ipc_task_runner_->RunsTasksInCurrentSequence());
   if (tag_count == -1) {
     LOG(ERROR) << "Failed to get tag count";
-    initialized_.Signal();
+    StopInitialization();
     return;
   }
 
   if (tag_count == 0) {
     // There is no vendor tag, we are done here.
-    initialized_.Signal();
+    StopInitialization();
     return;
   }
 
@@ -134,6 +146,13 @@ void VendorTagOpsDelegate::OnGotTagType(uint32_t tag, int32_t type) {
 
 const VendorTagInfo* VendorTagOpsDelegate::GetInfoByName(
     const std::string& full_name) {
+  {
+    base::AutoLock lock(lock_);
+    if (!is_initializing_ && !initialized_.IsSignaled()) {
+      LOG(WARNING) << "VendorTagOps is accessed before calling Initialize()";
+      return nullptr;
+    }
+  }
   initialized_.Wait();
   auto it = name_map_.find(full_name);
   if (it == name_map_.end()) {
@@ -144,6 +163,13 @@ const VendorTagInfo* VendorTagOpsDelegate::GetInfoByName(
 
 const VendorTagInfo* VendorTagOpsDelegate::GetInfoByTag(
     cros::mojom::CameraMetadataTag tag) {
+  {
+    base::AutoLock lock(lock_);
+    if (!is_initializing_ && !initialized_.IsSignaled()) {
+      LOG(WARNING) << "VendorTagOps is accessed before calling Initialize()";
+      return nullptr;
+    }
+  }
   initialized_.Wait();
   auto it = tag_map_.find(tag);
   if (it == tag_map_.end()) {

media/capture/video/chromeos/vendor_tag_ops_delegate.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/synchronization/lock.h"
 #include "media/capture/video/chromeos/mojom/camera_common.mojom.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
@@ -41,6 +42,7 @@ class VendorTagOpsDelegate {
   const VendorTagInfo* GetInfoByTag(cros::mojom::CameraMetadataTag tag);
 
  private:
+  void StopInitialization();
   void RemovePending(uint32_t tag);
 
   void OnGotTagCount(int32_t tag_count);
@@ -63,6 +65,9 @@ class VendorTagOpsDelegate {
   std::map<cros::mojom::CameraMetadataTag, VendorTagInfo> tag_map_;
 
   base::WaitableEvent initialized_;
+
+  base::Lock lock_;
+  bool is_initializing_ GUARDED_BY(lock_);
 };
 
 }  // namespace media