Return Sensor pipe in SensorInitParams

This patch modifies the GetSensor method so that the Sensor interface
pointer is returned in the SensorInitParams struct instead of being
passed to the GetSensor method as an interface request. This prevents
a class of errors where an early connection error on the Sensor pipe
can race with the reply to the GetSensor method as they are sent on
different FIFOs.

Bug: 781767
Change-Id: I79e47417da4b0bda49ddb44070e123009e46b893
Reviewed-on: https://chromium-review.googlesource.com/757283
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Tim Volodine <timvolodine@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515333}

content/browser/device_sensors/device_sensor_browsertest.cc

@@ -169,7 +169,6 @@ class FakeSensorProvider : public device::mojom::SensorProvider {
 
   // device::mojom::sensorProvider:
   void GetSensor(device::mojom::SensorType type,
-                 device::mojom::SensorRequest sensor_request,
                  GetSensorCallback callback) override {
     std::unique_ptr<FakeSensor> sensor;
     device::SensorReading reading;
@@ -228,17 +227,23 @@ class FakeSensorProvider : public device::mojom::SensorProvider {
 
     if (sensor) {
       sensor->set_reading(reading);
+
       auto init_params = device::mojom::SensorInitParams::New();
+      init_params->client_request = sensor->GetClient();
       init_params->memory = sensor->GetSharedBufferHandle();
       init_params->buffer_offset = sensor->GetBufferOffset();
       init_params->default_configuration = sensor->GetDefaultConfiguration();
       init_params->maximum_frequency = sensor->GetMaximumSupportedFrequency();
       init_params->minimum_frequency = sensor->GetMinimumSupportedFrequency();
 
-      std::move(callback).Run(std::move(init_params), sensor->GetClient());
-      mojo::MakeStrongBinding(std::move(sensor), std::move(sensor_request));
+      device::mojom::SensorPtr sensor_ptr;
+      mojo::MakeStrongBinding(std::move(sensor),
+                              mojo::MakeRequest(&sensor_ptr));
+      init_params->sensor = std::move(sensor_ptr);
+
+      std::move(callback).Run(std::move(init_params));
     } else {
-      std::move(callback).Run(nullptr, nullptr);
+      std::move(callback).Run(nullptr);
     }
   }
 

content/browser/generic_sensor/sensor_provider_proxy_impl.cc

@@ -36,13 +36,12 @@ void SensorProviderProxyImpl::Bind(
 
 void SensorProviderProxyImpl::GetSensor(
     device::mojom::SensorType type,
-    device::mojom::SensorRequest sensor_request,
     GetSensorCallback callback) {
   ServiceManagerConnection* connection =
       ServiceManagerConnection::GetForProcess();
 
   if (!connection || !CheckPermission(type)) {
-    std::move(callback).Run(nullptr, nullptr);
+    std::move(callback).Run(nullptr);
     return;
   }
 
@@ -53,8 +52,7 @@ void SensorProviderProxyImpl::GetSensor(
         base::BindOnce(&device::mojom::SensorProviderPtr::reset,
                        base::Unretained(&sensor_provider_)));
   }
-  sensor_provider_->GetSensor(type, std::move(sensor_request),
-                              std::move(callback));
+  sensor_provider_->GetSensor(type, std::move(callback));
 }
 
 bool SensorProviderProxyImpl::CheckPermission(

content/browser/generic_sensor/sensor_provider_proxy_impl.h

@@ -28,7 +28,6 @@ class SensorProviderProxyImpl final : public device::mojom::SensorProvider {
  private:
   // SensorProvider implementation.
   void GetSensor(device::mojom::SensorType type,
-                 device::mojom::SensorRequest sensor_request,
                  GetSensorCallback callback) override;
 
   bool CheckPermission(device::mojom::SensorType type) const;

content/browser/generic_sensor_browsertest.cc

@@ -139,21 +139,25 @@ class FakeSensorProvider : public device::mojom::SensorProvider {
 
   // device::mojom::sensorProvider implementation.
   void GetSensor(device::mojom::SensorType type,
-                 device::mojom::SensorRequest sensor_request,
                  GetSensorCallback callback) override {
     switch (type) {
       case device::mojom::SensorType::AMBIENT_LIGHT: {
         auto sensor = std::make_unique<FakeAmbientLightSensor>();
 
         auto init_params = device::mojom::SensorInitParams::New();
+        init_params->client_request = sensor->GetClient();
         init_params->memory = sensor->GetSharedBufferHandle();
         init_params->buffer_offset = sensor->GetBufferOffset();
         init_params->default_configuration = sensor->GetDefaultConfiguration();
         init_params->maximum_frequency = sensor->GetMaximumSupportedFrequency();
         init_params->minimum_frequency = sensor->GetMinimumSupportedFrequency();
 
-        std::move(callback).Run(std::move(init_params), sensor->GetClient());
-        mojo::MakeStrongBinding(std::move(sensor), std::move(sensor_request));
+        device::mojom::SensorPtr sensor_ptr;
+        mojo::MakeStrongBinding(std::move(sensor),
+                                mojo::MakeRequest(&sensor_ptr));
+        init_params->sensor = std::move(sensor_ptr);
+
+        std::move(callback).Run(std::move(init_params));
         break;
       }
       default:

content/renderer/device_sensors/device_sensor_event_pump.h

@@ -101,8 +101,7 @@ class CONTENT_EXPORT DeviceSensorEventPump
     }
 
     // Mojo callback for SensorProvider::GetSensor().
-    void OnSensorCreated(device::mojom::SensorInitParamsPtr params,
-                         device::mojom::SensorClientRequest client_request) {
+    void OnSensorCreated(device::mojom::SensorInitParamsPtr params) {
       if (!params) {
         HandleSensorError();
         event_pump->DidStartIfPossible();
@@ -117,8 +116,8 @@ class CONTENT_EXPORT DeviceSensorEventPump
       mode = params->mode;
       default_config = params->default_configuration;
 
-      DCHECK(sensor.is_bound());
-      client_binding.Bind(std::move(client_request));
+      sensor = std::move(params->sensor);
+      client_binding.Bind(std::move(params->client_request));
 
       shared_buffer_handle = std::move(params->memory);
       DCHECK(!shared_buffer);
@@ -138,6 +137,8 @@ class CONTENT_EXPORT DeviceSensorEventPump
 
       default_config.set_frequency(kDefaultPumpFrequencyHz);
 
+      sensor.set_connection_error_handler(
+          base::Bind(&SensorEntry::HandleSensorError, base::Unretained(this)));
       sensor->ConfigureReadingChangeNotifications(false /* disabled */);
       sensor->AddConfiguration(
           default_config, base::Bind(&SensorEntry::OnSensorAddConfiguration,
@@ -189,12 +190,9 @@ class CONTENT_EXPORT DeviceSensorEventPump
   friend struct SensorEntry;
 
   void GetSensor(SensorEntry* sensor_entry) {
-    auto request = mojo::MakeRequest(&sensor_entry->sensor);
-    sensor_provider_->GetSensor(sensor_entry->type, std::move(request),
+    sensor_provider_->GetSensor(sensor_entry->type,
                                 base::Bind(&SensorEntry::OnSensorCreated,
                                            base::Unretained(sensor_entry)));
-    sensor_entry->sensor.set_connection_error_handler(base::Bind(
-        &SensorEntry::HandleSensorError, base::Unretained(sensor_entry)));
   }
 
   virtual void DidStartIfPossible() {