VCD: Migrate to use SetCallbacksAssociated

This addresses a subtle race condition in
CameraModuleCallbacks::SetCallbacks().

The key sequence of events,
1. CameraHalClient receives a pending remote of CameraModule
2. CameraHalClient calls CameraModule::GetNumberOfCameras()
3. CameraHalClient calls CameraModule::SetCallbacks() to pass a pending
   remote of CameraModuleCallbacks.
4. CameraModule receiver would fire
   CameraModuleCallbacks::CameraDeviceStatusChange()
5. CameraModule receiver then fires CameraModule::SetCallbacksCallback

The expectation was that 4 would be received before 5. However, since
CameraModule and CameraModuleCallbacks run on different message pipes, 4
can actually be received after 5, not meeting the expectation.

So here we make CameraModuleCallbacks an associated interface of
CameraModule.
Please see crrev.com/c/2434712 for the implementation on CameraModule.

Bug: b/169324225
Test: With the following,
    1. Open Chrome Camera App (CCA)
    2. restart cros-camera
    3. Verify CameraDeviceStatusChange is received before
       SetCallbacksCallback
    4. Repeat and passes 20/20 times.
    Additionally,
    1. Build capture_unittests and scp to DUT
    2. tast run dut 'camera.CaptureUnittests'

Change-Id: I79f6006b7827d2e556b01e7d57df43916ca7913e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2470898Reviewed-by: Wei Lee <wtlee@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841019}

media/capture/video/chromeos/camera_device_delegate_unittest.cc

@@ -354,7 +354,7 @@ class CameraDeviceDelegateTest : public ::testing::Test {
         .Times(1)
         .WillOnce(
             Invoke(this, &CameraDeviceDelegateTest::GetNumberOfFakeCameras));
-    EXPECT_CALL(mock_camera_module_, DoSetCallbacks(_, _)).Times(1);
+    EXPECT_CALL(mock_camera_module_, DoSetCallbacksAssociated(_, _)).Times(1);
     EXPECT_CALL(mock_camera_module_, DoGetVendorTagOps(_, _))
         .Times(1)
         .WillOnce(Invoke(this, &CameraDeviceDelegateTest::GetFakeVendorTagOps));

media/capture/video/chromeos/camera_hal_delegate.cc

@@ -533,8 +533,8 @@ void CameraHalDelegate::OnGotNumberOfCamerasOnIpcThread(int32_t num_cameras) {
   // Per camera HAL v3 specification SetCallbacks() should be called after the
   // first time GetNumberOfCameras() is called, and before other CameraModule
   // functions are called.
-  camera_module_->SetCallbacks(
-      camera_module_callbacks_.BindNewPipeAndPassRemote(),
+  camera_module_->SetCallbacksAssociated(
+      camera_module_callbacks_.BindNewEndpointAndPassRemote(),
       base::BindOnce(&CameraHalDelegate::OnSetCallbacksOnIpcThread, this));
 
   camera_module_->GetVendorTagOps(

media/capture/video/chromeos/camera_hal_delegate.h

@@ -20,9 +20,10 @@
 #include "media/capture/video/chromeos/vendor_tag_ops_delegate.h"
 #include "media/capture/video/video_capture_device_factory.h"
 #include "media/capture/video_capture_types.h"
+#include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "mojo/public/cpp/bindings/pending_associated_receiver.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
-#include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
 namespace media {
@@ -196,7 +197,8 @@ class CAPTURE_EXPORT CameraHalDelegate final
 
   // The Mojo receiver serving the camera module callbacks.  Bound to
   // |ipc_task_runner_|.
-  mojo::Receiver<cros::mojom::CameraModuleCallbacks> camera_module_callbacks_;
+  mojo::AssociatedReceiver<cros::mojom::CameraModuleCallbacks>
+      camera_module_callbacks_;
 
   // An internal delegate to handle VendorTagOps mojo connection and query
   // information of vendor tags.  Bound to |ipc_task_runner_|.

media/capture/video/chromeos/camera_hal_delegate_unittest.cc

@@ -167,8 +167,9 @@ TEST_F(CameraHalDelegateTest, GetBuiltinCameraInfo) {
       };
 
   auto set_callbacks_cb =
-      [&](mojo::PendingRemote<cros::mojom::CameraModuleCallbacks>& callbacks,
-          cros::mojom::CameraModule::SetCallbacksCallback&) {
+      [&](mojo::PendingAssociatedRemote<cros::mojom::CameraModuleCallbacks>&
+              callbacks,
+          cros::mojom::CameraModule::SetCallbacksAssociatedCallback&) {
         mock_camera_module_.NotifyCameraDeviceChange(
             2, cros::mojom::CameraDeviceStatus::CAMERA_DEVICE_STATUS_PRESENT);
       };
@@ -176,10 +177,12 @@ TEST_F(CameraHalDelegateTest, GetBuiltinCameraInfo) {
   EXPECT_CALL(mock_camera_module_, DoGetNumberOfCameras(_))
       .Times(1)
       .WillOnce(Invoke(get_number_of_cameras_cb));
-  EXPECT_CALL(mock_camera_module_,
-              DoSetCallbacks(
-                  A<mojo::PendingRemote<cros::mojom::CameraModuleCallbacks>&>(),
-                  A<cros::mojom::CameraModule::SetCallbacksCallback&>()))
+  EXPECT_CALL(
+      mock_camera_module_,
+      DoSetCallbacksAssociated(
+          A<mojo::PendingAssociatedRemote<
+              cros::mojom::CameraModuleCallbacks>&>(),
+          A<cros::mojom::CameraModule::SetCallbacksAssociatedCallback&>()))
       .Times(1)
       .WillOnce(Invoke(set_callbacks_cb));
   EXPECT_CALL(mock_camera_module_,

media/capture/video/chromeos/mock_camera_module.cc

@@ -42,9 +42,7 @@ void MockCameraModule::GetCameraInfo(int32_t camera_id,
 void MockCameraModule::SetCallbacks(
     mojo::PendingRemote<cros::mojom::CameraModuleCallbacks> callbacks,
     SetCallbacksCallback callback) {
-  DoSetCallbacks(callbacks, callback);
-  callbacks_.Bind(std::move(callbacks));
-  std::move(callback).Run(0);
+  // Method deprecated and not expected to be called.
 }
 
 void MockCameraModule::Init(InitCallback callback) {
@@ -66,6 +64,13 @@ void MockCameraModule::GetVendorTagOps(
   std::move(callback).Run();
 }
 
+void MockCameraModule::SetCallbacksAssociated(
+    mojo::PendingAssociatedRemote<cros::mojom::CameraModuleCallbacks> callbacks,
+    SetCallbacksAssociatedCallback callback) {
+  DoSetCallbacksAssociated(callbacks, callback);
+  callbacks_.Bind(std::move(callbacks));
+  std::move(callback).Run(0);
+}
 void MockCameraModule::NotifyCameraDeviceChange(
     int camera_id,
     cros::mojom::CameraDeviceStatus status) {

media/capture/video/chromeos/mock_camera_module.h

@@ -11,6 +11,8 @@
 #include "base/threading/thread.h"
 #include "media/capture/video/chromeos/mojom/camera3.mojom.h"
 #include "media/capture/video/chromeos/mojom/camera_common.mojom.h"
+#include "mojo/public/cpp/bindings/associated_remote.h"
+#include "mojo/public/cpp/bindings/pending_associated_remote.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
@@ -72,6 +74,14 @@ class MockCameraModule : public cros::mojom::CameraModule {
                         vendor_tag_ops_receiver,
                     GetVendorTagOpsCallback& callback));
 
+  void SetCallbacksAssociated(mojo::PendingAssociatedRemote<
+                                  cros::mojom::CameraModuleCallbacks> callbacks,
+                              SetCallbacksAssociatedCallback callback) override;
+  MOCK_METHOD2(DoSetCallbacksAssociated,
+               void(mojo::PendingAssociatedRemote<
+                        cros::mojom::CameraModuleCallbacks>& callbacks,
+                    SetCallbacksAssociatedCallback& callback));
+
   void NotifyCameraDeviceChange(int camera_id,
                                 cros::mojom::CameraDeviceStatus status);
 
@@ -89,7 +99,7 @@ class MockCameraModule : public cros::mojom::CameraModule {
 
   base::Thread mock_module_thread_;
   mojo::Receiver<cros::mojom::CameraModule> receiver_{this};
-  mojo::Remote<cros::mojom::CameraModuleCallbacks> callbacks_;
+  mojo::AssociatedRemote<cros::mojom::CameraModuleCallbacks> callbacks_;
 
   DISALLOW_COPY_AND_ASSIGN(MockCameraModule);
 };

media/capture/video/chromeos/mojom/camera_common.mojom

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Next min version: 3
+// Next min version: 4
 
 module cros.mojom;
 
@@ -101,6 +101,7 @@ interface CameraModule {
   // Gets various info about the camera specified by |camera_id|.
   GetCameraInfo@2(int32 camera_id) => (int32 result, CameraInfo? camera_info);
 
+  // [Deprecated in version 3]
   // Registers the CameraModuleCallbacks interface with the camera HAL.
   SetCallbacks@3(pending_remote<CameraModuleCallbacks> callbacks)
       => (int32 result);
@@ -122,4 +123,11 @@ interface CameraModule {
   [MinVersion=2]
   GetVendorTagOps@6(pending_receiver<VendorTagOps> vendor_tag_ops_request)
       => ();
+
+  // Registers the CameraModuleCallbacks associated interface with the camera
+  // HAL. TODO(b/169324225): Migrate all camera HAL clients to use this.
+  [MinVersion=3]
+  SetCallbacksAssociated@7(
+      pending_associated_remote<CameraModuleCallbacks> callbacks)
+      => (int32 result);
 };