Create single instance of SerialPortManagerImpl for Device Service

As a follow-up to r620271 which assigned each serial port a token this
change makes it so that the Device Service serves all requests for the
SerialPortManager interface from the same SerialPortManagerImpl. This
means that the tokens assigned to each port are consistent between
multiple clients of this interface. Otherwise each gets its own mapping
from port path to token, which defeats the point.

Sensor tests have been updated to not inherit from DeviceServiceTestBase
as they do not depend on it and don't initialize it properly.

Bug: 908833
Change-Id: Id7ea9ab6368891fe384286fd18c82ca40c842ced
Reviewed-on: https://chromium-review.googlesource.com/c/1416535Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625752}

services/device/device_service.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
+#include "base/task/post_task.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "mojo/public/cpp/system/message_pipe.h"
@@ -116,6 +117,11 @@ DeviceService::~DeviceService() {
 #if !defined(OS_ANDROID)
   device::BatteryStatusService::GetInstance()->Shutdown();
 #endif
+#if (defined(OS_LINUX) && defined(USE_UDEV)) || defined(OS_WIN) || \
+    defined(OS_MACOSX)
+  serial_port_manager_task_runner_->DeleteSoon(FROM_HERE,
+                                               std::move(serial_port_manager_));
+#endif
 }
 
 void DeviceService::OnStart() {
@@ -141,8 +147,6 @@ void DeviceService::OnStart() {
       &DeviceService::BindTimeZoneMonitorRequest, base::Unretained(this)));
   registry_.AddInterface<mojom::WakeLockProvider>(base::Bind(
       &DeviceService::BindWakeLockProviderRequest, base::Unretained(this)));
-  registry_.AddInterface<mojom::SerialPortManager>(base::Bind(
-      &DeviceService::BindSerialPortManagerRequest, base::Unretained(this)));
   registry_.AddInterface<mojom::UsbDeviceManager>(base::Bind(
       &DeviceService::BindUsbDeviceManagerRequest, base::Unretained(this)));
   registry_.AddInterface<mojom::UsbDeviceManagerTest>(base::Bind(
@@ -167,6 +171,20 @@ void DeviceService::OnStart() {
       &DeviceService::BindVibrationManagerRequest, base::Unretained(this)));
 #endif
 
+#if (defined(OS_LINUX) && defined(USE_UDEV)) || defined(OS_WIN) || \
+    defined(OS_MACOSX)
+  // SerialPortManagerImpl must live on a thread that is allowed to do
+  // blocking IO.
+  serial_port_manager_ = std::make_unique<SerialPortManagerImpl>(
+      io_task_runner_, base::ThreadTaskRunnerHandle::Get());
+  serial_port_manager_task_runner_ = base::CreateSequencedTaskRunnerWithTraits(
+      {base::MayBlock(), base::TaskPriority::BEST_EFFORT});
+  registry_.AddInterface<mojom::SerialPortManager>(
+      base::BindRepeating(&SerialPortManagerImpl::Bind,
+                          base::Unretained(serial_port_manager_.get())),
+      serial_port_manager_task_runner_);
+#endif
+
 #if defined(OS_CHROMEOS)
   registry_.AddInterface<mojom::BluetoothSystemFactory>(
       base::BindRepeating(&DeviceService::BindBluetoothSystemFactoryRequest,
@@ -305,15 +323,6 @@ void DeviceService::BindWakeLockProviderRequest(
                            wake_lock_context_callback_);
 }
 
-void DeviceService::BindSerialPortManagerRequest(
-    mojom::SerialPortManagerRequest request) {
-#if (defined(OS_LINUX) && defined(USE_UDEV)) || defined(OS_WIN) || \
-    defined(OS_MACOSX)
-  if (io_task_runner_)
-    SerialPortManagerImpl::Create(std::move(request), io_task_runner_);
-#endif
-}
-
 void DeviceService::BindUsbDeviceManagerRequest(
     mojom::UsbDeviceManagerRequest request) {
   if (!usb_device_manager_)

services/device/device_service.h

@@ -66,6 +66,7 @@ namespace device {
 
 #if !defined(OS_ANDROID)
 class HidManagerImpl;
+class SerialPortManagerImpl;
 #endif
 
 class DeviceService;
@@ -161,8 +162,6 @@ class DeviceService : public service_manager::Service {
 
   void BindWakeLockProviderRequest(mojom::WakeLockProviderRequest request);
 
-  void BindSerialPortManagerRequest(mojom::SerialPortManagerRequest request);
-
   void BindUsbDeviceManagerRequest(mojom::UsbDeviceManagerRequest request);
 
   void BindUsbDeviceManagerTestRequest(
@@ -198,6 +197,15 @@ class DeviceService : public service_manager::Service {
   std::unique_ptr<HidManagerImpl> hid_manager_;
 #endif
 
+#if (defined(OS_LINUX) && defined(USE_UDEV)) || defined(OS_WIN) || \
+    defined(OS_MACOSX)
+  // Requests for the SerialPortManager interface must be bound to
+  // |serial_port_manager_| on |serial_port_manager_task_runner_| and it will
+  // be destroyed on that sequence.
+  std::unique_ptr<SerialPortManagerImpl> serial_port_manager_;
+  scoped_refptr<base::SequencedTaskRunner> serial_port_manager_task_runner_;
+#endif
+
 #if defined(OS_CHROMEOS)
   std::unique_ptr<MtpDeviceManager> mtp_device_manager_;
 #endif

services/device/generic_sensor/absolute_orientation_euler_angles_fusion_algorithm_using_accelerometer_and_magnetometer_unittest.cc

@@ -5,7 +5,7 @@
 #include <cmath>
 
 #include "base/memory/ref_counted.h"
-#include "services/device/device_service_test_base.h"
+#include "base/test/scoped_task_environment.h"
 #include "services/device/generic_sensor/absolute_orientation_euler_angles_fusion_algorithm_using_accelerometer_and_magnetometer.h"
 #include "services/device/generic_sensor/fake_platform_sensor_fusion.h"
 #include "services/device/generic_sensor/generic_sensor_consts.h"
@@ -18,7 +18,7 @@ namespace {
 
 class
     AbsoluteOrientationEulerAnglesFusionAlgorithmUsingAccelerometerAndMagnetometerTest
-    : public DeviceServiceTestBase {
+    : public testing::Test {
  public:
   AbsoluteOrientationEulerAnglesFusionAlgorithmUsingAccelerometerAndMagnetometerTest() {
     auto fusion_algorithm = std::make_unique<
@@ -67,6 +67,7 @@ class
   }
 
  protected:
+  base::test::ScopedTaskEnvironment task_environment_;
   scoped_refptr<FakePlatformSensorFusion> fake_fusion_sensor_;
   AbsoluteOrientationEulerAnglesFusionAlgorithmUsingAccelerometerAndMagnetometer*
       fusion_algorithm_;

services/device/generic_sensor/linear_acceleration_fusion_algorithm_using_accelerometer_unittest.cc

@@ -4,7 +4,7 @@
 
 #include "services/device/generic_sensor/linear_acceleration_fusion_algorithm_using_accelerometer.h"
 #include "base/memory/ref_counted.h"
-#include "services/device/device_service_test_base.h"
+#include "base/test/scoped_task_environment.h"
 #include "services/device/generic_sensor/fake_platform_sensor_fusion.h"
 #include "services/device/generic_sensor/generic_sensor_consts.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -14,7 +14,7 @@ namespace device {
 namespace {
 
 class LinearAccelerationFusionAlgorithmUsingAccelerometerTest
-    : public DeviceServiceTestBase {
+    : public testing::Test {
  public:
   LinearAccelerationFusionAlgorithmUsingAccelerometerTest() {
     auto fusion_algorithm =
@@ -106,6 +106,7 @@ class LinearAccelerationFusionAlgorithmUsingAccelerometerTest
   }
 
  protected:
+  base::test::ScopedTaskEnvironment task_environment_;
   scoped_refptr<FakePlatformSensorFusion> fake_fusion_sensor_;
   LinearAccelerationFusionAlgorithmUsingAccelerometer* fusion_algorithm_;
 };

services/device/generic_sensor/orientation_euler_angles_fusion_algorithm_using_quaternion_unittest.cc

@@ -4,7 +4,7 @@
 
 #include "services/device/generic_sensor/orientation_euler_angles_fusion_algorithm_using_quaternion.h"
 #include "base/memory/ref_counted.h"
-#include "services/device/device_service_test_base.h"
+#include "base/test/scoped_task_environment.h"
 #include "services/device/generic_sensor/fake_platform_sensor_fusion.h"
 #include "services/device/generic_sensor/generic_sensor_consts.h"
 #include "services/device/generic_sensor/orientation_test_data.h"
@@ -13,7 +13,7 @@
 namespace device {
 
 class OrientationEulerAnglesFusionAlgorithmUsingQuaternionTest
-    : public DeviceServiceTestBase {
+    : public testing::Test {
  public:
   OrientationEulerAnglesFusionAlgorithmUsingQuaternionTest() {
     auto fusion_algorithm =
@@ -26,6 +26,7 @@ class OrientationEulerAnglesFusionAlgorithmUsingQuaternionTest
   }
 
  protected:
+  base::test::ScopedTaskEnvironment task_environment_;
   scoped_refptr<FakePlatformSensorFusion> fake_fusion_sensor_;
   OrientationEulerAnglesFusionAlgorithmUsingQuaternion* fusion_algorithm_;
 };

services/device/generic_sensor/orientation_quaternion_fusion_algorithm_using_euler_angles_unittest.cc

@@ -4,18 +4,18 @@
 
 #include <cmath>
 
-#include "services/device/generic_sensor/orientation_quaternion_fusion_algorithm_using_euler_angles.h"
 #include "base/memory/ref_counted.h"
-#include "services/device/device_service_test_base.h"
+#include "base/test/scoped_task_environment.h"
 #include "services/device/generic_sensor/fake_platform_sensor_fusion.h"
 #include "services/device/generic_sensor/generic_sensor_consts.h"
+#include "services/device/generic_sensor/orientation_quaternion_fusion_algorithm_using_euler_angles.h"
 #include "services/device/generic_sensor/orientation_test_data.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace device {
 
 class OrientationQuaternionFusionAlgorithmUsingEulerAnglesTest
-    : public DeviceServiceTestBase {
+    : public testing::Test {
  public:
   OrientationQuaternionFusionAlgorithmUsingEulerAnglesTest() {
     auto fusion_algorithm =
@@ -28,6 +28,7 @@ class OrientationQuaternionFusionAlgorithmUsingEulerAnglesTest
   }
 
  protected:
+  base::test::ScopedTaskEnvironment task_environment_;
   scoped_refptr<FakePlatformSensorFusion> fake_fusion_sensor_;
   OrientationQuaternionFusionAlgorithmUsingEulerAngles* fusion_algorithm_;
 };

services/device/generic_sensor/platform_sensor_and_provider_unittest.cc

@@ -6,7 +6,7 @@
 
 #include "base/macros.h"
 #include "base/memory/singleton.h"
-#include "services/device/device_service_test_base.h"
+#include "base/test/scoped_task_environment.h"
 #include "services/device/generic_sensor/fake_platform_sensor_and_provider.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -16,7 +16,7 @@ using ::testing::NiceMock;
 
 namespace device {
 
-class PlatformSensorProviderTest : public DeviceServiceTestBase {
+class PlatformSensorProviderTest : public testing::Test {
  public:
   PlatformSensorProviderTest() {
     provider_ = std::make_unique<FakePlatformSensorProvider>();
@@ -27,6 +27,8 @@ class PlatformSensorProviderTest : public DeviceServiceTestBase {
   std::unique_ptr<FakePlatformSensorProvider> provider_;
 
  private:
+  base::test::ScopedTaskEnvironment task_environment_;
+
   DISALLOW_COPY_AND_ASSIGN(PlatformSensorProviderTest);
 };
 

services/device/generic_sensor/platform_sensor_fusion_unittest.cc

@@ -7,8 +7,8 @@
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/test/scoped_task_environment.h"
 #include "mojo/public/cpp/bindings/binding.h"
-#include "services/device/device_service_test_base.h"
 #include "services/device/generic_sensor/absolute_orientation_euler_angles_fusion_algorithm_using_accelerometer_and_magnetometer.h"
 #include "services/device/generic_sensor/fake_platform_sensor_and_provider.h"
 #include "services/device/generic_sensor/linear_acceleration_fusion_algorithm_using_accelerometer.h"
@@ -25,7 +25,7 @@ namespace device {
 
 using mojom::SensorType;
 
-class PlatformSensorFusionTest : public DeviceServiceTestBase {
+class PlatformSensorFusionTest : public testing::Test {
  public:
   PlatformSensorFusionTest() {
     provider_ = std::make_unique<FakePlatformSensorProvider>();
@@ -90,6 +90,7 @@ class PlatformSensorFusionTest : public DeviceServiceTestBase {
     EXPECT_TRUE(platform_sensor_fusion_callback_called_);
   }
 
+  base::test::ScopedTaskEnvironment task_environment_;
   std::unique_ptr<FakePlatformSensorProvider> provider_;
   bool accelerometer_callback_called_ = false;
   scoped_refptr<FakePlatformSensor> accelerometer_;

services/device/generic_sensor/relative_orientation_euler_angles_fusion_algorithm_using_accelerometer_and_gyroscope_unittest.cc

@@ -6,7 +6,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/numerics/math_constants.h"
-#include "services/device/device_service_test_base.h"
+#include "base/test/scoped_task_environment.h"
 #include "services/device/generic_sensor/fake_platform_sensor_fusion.h"
 #include "services/device/generic_sensor/generic_sensor_consts.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -17,7 +17,7 @@ namespace {
 
 class
     RelativeOrientationEulerAnglesFusionAlgorithmUsingAccelerometerAndGyroscopeTest
-    : public DeviceServiceTestBase {
+    : public testing::Test {
  public:
   RelativeOrientationEulerAnglesFusionAlgorithmUsingAccelerometerAndGyroscopeTest() {
     auto fusion_algorithm = std::make_unique<
@@ -69,6 +69,7 @@ class
   }
 
  protected:
+  base::test::ScopedTaskEnvironment task_environment_;
   scoped_refptr<FakePlatformSensorFusion> fake_fusion_sensor_;
   RelativeOrientationEulerAnglesFusionAlgorithmUsingAccelerometerAndGyroscope*
       fusion_algorithm_;

services/device/generic_sensor/relative_orientation_euler_angles_fusion_algorithm_using_accelerometer_unittest.cc

@@ -5,7 +5,7 @@
 #include <cmath>
 
 #include "base/memory/ref_counted.h"
-#include "services/device/device_service_test_base.h"
+#include "base/test/scoped_task_environment.h"
 #include "services/device/generic_sensor/fake_platform_sensor_fusion.h"
 #include "services/device/generic_sensor/generic_sensor_consts.h"
 #include "services/device/generic_sensor/relative_orientation_euler_angles_fusion_algorithm_using_accelerometer.h"
@@ -16,7 +16,7 @@ namespace device {
 namespace {
 
 class RelativeOrientationEulerAnglesFusionAlgorithmUsingAccelerometerTest
-    : public DeviceServiceTestBase {
+    : public testing::Test {
  public:
   RelativeOrientationEulerAnglesFusionAlgorithmUsingAccelerometerTest() {
     auto fusion_algorithm = std::make_unique<
@@ -55,6 +55,7 @@ class RelativeOrientationEulerAnglesFusionAlgorithmUsingAccelerometerTest
   }
 
  protected:
+  base::test::ScopedTaskEnvironment task_environment_;
   scoped_refptr<FakePlatformSensorFusion> fake_fusion_sensor_;
   RelativeOrientationEulerAnglesFusionAlgorithmUsingAccelerometer*
       fusion_algorithm_;

services/device/serial/serial_port_manager_impl.cc

@@ -8,52 +8,24 @@
 #include <utility>
 
 #include "base/sequenced_task_runner.h"
-#include "base/task/post_task.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "services/device/serial/serial_device_enumerator.h"
 #include "services/device/serial/serial_port_impl.h"
 
 namespace device {
 
-namespace {
-
-void CreateAndBindOnBlockableRunner(
-    mojom::SerialPortManagerRequest request,
-    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) {
-  mojo::MakeStrongBinding(
-      std::make_unique<SerialPortManagerImpl>(std::move(io_task_runner),
-                                              std::move(ui_task_runner)),
-      std::move(request));
-}
-
-}  // namespace
-
-// static
-void SerialPortManagerImpl::Create(
-    mojom::SerialPortManagerRequest request,
-    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner) {
-  // SerialPortManagerImpl must live on a thread that is allowed to do
-  // blocking IO.
-  scoped_refptr<base::SequencedTaskRunner> blockable_sequence_runner =
-      base::CreateSequencedTaskRunnerWithTraits(
-          {base::MayBlock(), base::TaskPriority::BEST_EFFORT});
-  blockable_sequence_runner->PostTask(
-      FROM_HERE, base::BindOnce(&CreateAndBindOnBlockableRunner,
-                                std::move(request), std::move(io_task_runner),
-                                base::ThreadTaskRunnerHandle::Get()));
-}
-
-// SerialPortManagerImpl must be created in a blockable runner.
 SerialPortManagerImpl::SerialPortManagerImpl(
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
-    : enumerator_(SerialDeviceEnumerator::Create()),
-      io_task_runner_(std::move(io_task_runner)),
+    : io_task_runner_(std::move(io_task_runner)),
       ui_task_runner_(std::move(ui_task_runner)) {}
 
 SerialPortManagerImpl::~SerialPortManagerImpl() = default;
 
+void SerialPortManagerImpl::Bind(mojom::SerialPortManagerRequest request) {
+  bindings_.AddBinding(this, std::move(request));
+}
+
 void SerialPortManagerImpl::SetSerialEnumeratorForTesting(
     std::unique_ptr<SerialDeviceEnumerator> fake_enumerator) {
   DCHECK(fake_enumerator);
@@ -61,13 +33,15 @@ void SerialPortManagerImpl::SetSerialEnumeratorForTesting(
 }
 
 void SerialPortManagerImpl::GetDevices(GetDevicesCallback callback) {
-  DCHECK(enumerator_);
+  if (!enumerator_)
+    enumerator_ = SerialDeviceEnumerator::Create();
   std::move(callback).Run(enumerator_->GetDevices());
 }
 
 void SerialPortManagerImpl::GetPort(const base::UnguessableToken& token,
                                     mojom::SerialPortRequest request) {
-  DCHECK(enumerator_);
+  if (!enumerator_)
+    enumerator_ = SerialDeviceEnumerator::Create();
   base::Optional<base::FilePath> path = enumerator_->GetPathFromToken(token);
   if (path) {
     io_task_runner_->PostTask(

services/device/serial/serial_port_manager_impl.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 #include "services/device/public/mojom/serial.mojom.h"
 
 namespace base {
@@ -25,15 +26,12 @@ class SerialDeviceEnumerator;
 // crbug.com/748505
 class SerialPortManagerImpl : public mojom::SerialPortManager {
  public:
-  static void Create(
-      mojom::SerialPortManagerRequest request,
-      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
-
   SerialPortManagerImpl(
       scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
       scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
   ~SerialPortManagerImpl() override;
 
+  void Bind(mojom::SerialPortManagerRequest request);
   void SetSerialEnumeratorForTesting(
       std::unique_ptr<SerialDeviceEnumerator> fake_enumerator);
 
@@ -48,6 +46,8 @@ class SerialPortManagerImpl : public mojom::SerialPortManager {
   scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
   scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
 
+  mojo::BindingSet<SerialPortManager> bindings_;
+
   DISALLOW_COPY_AND_ASSIGN(SerialPortManagerImpl);
 };