Fix race in DiskMountManager mount events

This CL fixes a race that can occur in the DiskMountManager when disks
are added. Since we wait for the asynchronous GetDeviceProperties dbus
call to complete before updating the disk map, if the disk mount event
is received too soon, we notify observers of the mount even though the
disk entry is not yet created. As a result, the CrosUsbDetector cannot
correctly track these entries and settings may fail to prompt the user
when trying to share USB drives. The notification that a new removable
storage device is added also does not appear.

We fix this by deferring sending the event until GetDeviceProperties
returns. I observed this race being triggered consistently on an IronKey
D300s drive, which initially presents itself as a CD drive.

Bug: 1146325
Change-Id: I2a6b6c352b36c1e06a7ddaa49d5b987e7458c8b8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2592575Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837848}

chromeos/disks/disk_mount_manager.cc

@@ -456,6 +456,12 @@ class DiskMountManagerImpl : public DiskMountManager,
 
   // CrosDisksClient::Observer override.
   void OnMountCompleted(const MountEntry& entry) override {
+    auto iter = deferred_mount_events_.find(entry.source_path());
+    if (iter != deferred_mount_events_.end()) {
+      iter->second.push_back(entry);
+      return;
+    }
+
     MountCondition mount_condition = MOUNT_CONDITION_NONE;
     if (entry.mount_type() == MOUNT_TYPE_DEVICE) {
       if (entry.error_code() == MOUNT_ERROR_UNKNOWN_FILESYSTEM) {
@@ -813,10 +819,24 @@ class DiskMountManagerImpl : public DiskMountManager,
                              device_label);
   }
 
+  // Fire observer mount events that were deferred due to an in-progress
+  // GetDeviceProperties() call.
+  void RunDeferredMountEvents(const std::string& device_path) {
+    auto mount_events_iter = deferred_mount_events_.find(device_path);
+    if (mount_events_iter == deferred_mount_events_.end())
+      return;
+    std::vector<MountEntry> entries = std::move(mount_events_iter->second);
+    deferred_mount_events_.erase(mount_events_iter);
+    for (const MountEntry& entry : entries)
+      OnMountCompleted(entry);
+  }
+
   // Callback for GetDeviceProperties.
   void OnGetDeviceProperties(const DiskInfo& disk_info) {
-    if (disk_info.is_virtual())
+    if (disk_info.is_virtual()) {
+      RunDeferredMountEvents(disk_info.device_path());
       return;
+    }
 
     DVLOG(1) << "Found disk " << disk_info.device_path();
     // Delete previous disk info for this path:
@@ -846,6 +866,7 @@ class DiskMountManagerImpl : public DiskMountManager,
     disks_.insert(
         std::make_pair(disk_info.device_path(), base::WrapUnique(disk)));
     NotifyDiskStatusUpdate(is_new ? DISK_ADDED : DISK_CHANGED, *disk);
+    RunDeferredMountEvents(disk_info.device_path());
   }
 
   // Part of EnsureMountInfoRefreshed(). Called after the list of devices are
@@ -915,6 +936,9 @@ class DiskMountManagerImpl : public DiskMountManager,
     std::string device_path = device_path_arg;
     switch (event) {
       case CROS_DISKS_DISK_ADDED: {
+        // Ensure we have an entry indicating we're waiting for
+        // GetDeviceProperties() to complete.
+        deferred_mount_events_[device_path];
         cros_disks_client_->GetDeviceProperties(
             device_path,
             base::BindOnce(&DiskMountManagerImpl::OnGetDeviceProperties,
@@ -1027,6 +1051,12 @@ class DiskMountManagerImpl : public DiskMountManager,
 
   DiskMountManager::MountPointMap mount_points_;
 
+  // A map entry with a key of the device path will be created upon calling
+  // GetDeviceProperties(), for deferring mount events, and removed once it has
+  // completed. This prevents a race resulting in mount events being fired with
+  // the corresponding Disk entry unexpectedly missing.
+  std::map<std::string, std::vector<MountEntry>> deferred_mount_events_;
+
   bool already_refreshed_;
   std::vector<EnsureMountInfoRefreshedCallback> refresh_callbacks_;
 

chromeos/disks/disk_mount_manager_unittest.cc

@@ -1667,6 +1667,42 @@ TEST_F(DiskMountManagerTest, Mount_RemountPreservesFirstMount) {
       manager->FindDiskBySourcePath(kDevice1SourcePath)->is_first_mount());
 }
 
+TEST_F(DiskMountManagerTest, Mount_DefersDuringGetDeviceProperties) {
+  DiskMountManager* manager = DiskMountManager::GetInstance();
+
+  // When a disk is added, we call GetDeviceProperties() before updating our
+  // DiskMap. If the disk is mounted before this asynchronous call returns, we
+  // defer sending the mount event so that clients are able to access the disk
+  // information immediately.
+
+  fake_cros_disks_client_->NotifyMountEvent(CROS_DISKS_DISK_REMOVED,
+                                            kDevice1SourcePath);
+  EXPECT_EQ(nullptr, manager->FindDiskBySourcePath(kDevice1SourcePath));
+
+  std::unique_ptr<dbus::Response> response = dbus::Response::CreateEmpty();
+  DiskInfo disk_info(kDevice1SourcePath, response.get());
+  fake_cros_disks_client_->set_next_get_device_properties_disk_info(&disk_info);
+  fake_cros_disks_client_->NotifyMountEvent(CROS_DISKS_DISK_ADDED,
+                                            kDevice1SourcePath);
+  fake_cros_disks_client_->NotifyMountCompleted(
+      chromeos::MOUNT_ERROR_NONE, kDevice1SourcePath,
+      chromeos::MOUNT_TYPE_DEVICE, kDevice1MountPath);
+
+  // The mount event will not have fired yet as we are still waiting for
+  // GetDeviceProperties() to return.
+  EXPECT_EQ(0u,
+            observer_->CountMountEvents(DiskMountManager::MOUNTING,
+                                        MOUNT_ERROR_NONE, kDevice1MountPath));
+  base::RunLoop().RunUntilIdle();
+
+  // We have fired 3 events: disk removed -> disk added -> mounting
+  const MountEvent& mount_event = observer_->GetMountEvent(2);
+  EXPECT_EQ(DiskMountManager::MOUNTING, mount_event.event);
+  EXPECT_EQ(kDevice1MountPath, mount_event.mount_point.mount_path);
+  // The test OnMountEvent() finds the matching disk when it is called.
+  EXPECT_NE(nullptr, mount_event.disk);
+}
+
 }  // namespace
 
 }  // namespace disks