Simplify smbfs mounting by using chromeos::disks::MountPoint

MountPoint uses a simple callback for result, instead of having to
register an observer and waiting for the appropriate event. The strong
ownership semantics of MountPoint also mean that explicit unmount are no
longer necessary.

Bug: 939235
Change-Id: I8478e414b8b7cc1c462985f272dea108547f67c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2006895Reviewed-by: Sergei Datsenko <dats@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733489}

chrome/browser/chromeos/smb_client/smbfs_share_unittest.cc

@@ -15,6 +15,7 @@
 #include "chromeos/components/smbfs/smbfs_host.h"
 #include "chromeos/components/smbfs/smbfs_mounter.h"
 #include "chromeos/disks/disk_mount_manager.h"
+#include "chromeos/disks/mount_point.h"
 #include "content/public/test/browser_task_environment.h"
 #include "storage/browser/file_system/external_mount_points.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -100,7 +101,9 @@ class SmbFsShareTest : public testing::Test {
       mojo::Receiver<smbfs::mojom::SmbFs>* smbfs_receiver,
       mojo::Remote<smbfs::mojom::SmbFsDelegate>* delegate) {
     return std::make_unique<smbfs::SmbFsHost>(
-        base::FilePath(kMountPath), share, disk_mount_manager_,
+        std::make_unique<chromeos::disks::MountPoint>(
+            base::FilePath(kMountPath), disk_mount_manager_),
+        share,
         mojo::Remote<smbfs::mojom::SmbFs>(
             smbfs_receiver->BindNewPipeAndPassRemote()),
         delegate->BindNewPipeAndPassReceiver());

chromeos/components/smbfs/smbfs_host.cc

@@ -38,28 +38,24 @@ class SmbFsDelegateImpl : public mojom::SmbFsDelegate {
 SmbFsHost::Delegate::~Delegate() = default;
 
 SmbFsHost::SmbFsHost(
-    const base::FilePath& mount_path,
+    std::unique_ptr<chromeos::disks::MountPoint> mount_point,
     Delegate* delegate,
-    chromeos::disks::DiskMountManager* disk_mount_manager,
     mojo::Remote<mojom::SmbFs> smbfs_remote,
     mojo::PendingReceiver<mojom::SmbFsDelegate> delegate_receiver)
-    : mount_path_(mount_path),
+    : mount_point_(std::move(mount_point)),
       delegate_(delegate),
-      disk_mount_manager_(disk_mount_manager),
       smbfs_(std::move(smbfs_remote)),
       delegate_impl_(std::make_unique<SmbFsDelegateImpl>(
           std::move(delegate_receiver),
           base::BindOnce(&SmbFsHost::OnDisconnect, base::Unretained(this)))) {
-  DCHECK(!mount_path.empty());
+  DCHECK(mount_point_);
   DCHECK(delegate);
 
   smbfs_.set_disconnect_handler(
       base::BindOnce(&SmbFsHost::OnDisconnect, base::Unretained(this)));
 }
 
-SmbFsHost::~SmbFsHost() {
-  disk_mount_manager_->UnmountPath(mount_path_.value(), base::DoNothing());
-}
+SmbFsHost::~SmbFsHost() = default;
 
 void SmbFsHost::OnDisconnect() {
   // Ensure only one disconnection event occurs.

chromeos/components/smbfs/smbfs_host.h

@@ -11,15 +11,10 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "chromeos/components/smbfs/mojom/smbfs.mojom.h"
+#include "chromeos/disks/mount_point.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
 
-namespace chromeos {
-namespace disks {
-class DiskMountManager;
-}  // namespace disks
-}  // namespace chromeos
-
 namespace smbfs {
 
 // SmbFsHost is a connection to a running instance of smbfs. It exposes methods
@@ -36,23 +31,23 @@ class COMPONENT_EXPORT(SMBFS) SmbFsHost {
     virtual void OnDisconnected() = 0;
   };
 
-  SmbFsHost(const base::FilePath& mount_path,
+  SmbFsHost(std::unique_ptr<chromeos::disks::MountPoint> mount_point,
             Delegate* delegate,
-            chromeos::disks::DiskMountManager* disk_mount_manager,
             mojo::Remote<mojom::SmbFs> smbfs_remote,
             mojo::PendingReceiver<mojom::SmbFsDelegate> delegate_receiver);
   ~SmbFsHost();
 
   // Returns the path where SmbFS is mounted.
-  const base::FilePath& mount_path() const { return mount_path_; }
+  const base::FilePath& mount_path() const {
+    return mount_point_->mount_path();
+  }
 
  private:
   // Mojo disconnection handler.
   void OnDisconnect();
 
-  const base::FilePath mount_path_;
+  const std::unique_ptr<chromeos::disks::MountPoint> mount_point_;
   Delegate* const delegate_;
-  chromeos::disks::DiskMountManager* const disk_mount_manager_;
 
   mojo::Remote<mojom::SmbFs> smbfs_;
   std::unique_ptr<mojom::SmbFsDelegate> delegate_impl_;

chromeos/components/smbfs/smbfs_host_unittest.cc

@@ -49,8 +49,10 @@ TEST_F(SmbFsHostTest, DisconnectDelegate) {
       .WillOnce(base::test::RunOnceCallback<1>(chromeos::MOUNT_ERROR_NONE));
 
   std::unique_ptr<SmbFsHost> host = std::make_unique<SmbFsHost>(
-      base::FilePath(kMountPath), &mock_delegate_, &mock_disk_mount_manager_,
-      std::move(smbfs_remote_), std::move(delegate_pending_receiver_));
+      std::make_unique<chromeos::disks::MountPoint>(base::FilePath(kMountPath),
+                                                    &mock_disk_mount_manager_),
+      &mock_delegate_, std::move(smbfs_remote_),
+      std::move(delegate_pending_receiver_));
   delegate_remote_.reset();
 
   run_loop.Run();
@@ -64,8 +66,10 @@ TEST_F(SmbFsHostTest, DisconnectSmbFs) {
       .WillOnce(base::test::RunOnceCallback<1>(chromeos::MOUNT_ERROR_NONE));
 
   std::unique_ptr<SmbFsHost> host = std::make_unique<SmbFsHost>(
-      base::FilePath(kMountPath), &mock_delegate_, &mock_disk_mount_manager_,
-      std::move(smbfs_remote_), std::move(delegate_pending_receiver_));
+      std::make_unique<chromeos::disks::MountPoint>(base::FilePath(kMountPath),
+                                                    &mock_disk_mount_manager_),
+      &mock_delegate_, std::move(smbfs_remote_),
+      std::move(delegate_pending_receiver_));
   smbfs_pending_receiver_.reset();
 
   run_loop.Run();
@@ -78,8 +82,10 @@ TEST_F(SmbFsHostTest, UnmountOnDestruction) {
 
   base::RunLoop run_loop;
   std::unique_ptr<SmbFsHost> host = std::make_unique<SmbFsHost>(
-      base::FilePath(kMountPath), &mock_delegate_, &mock_disk_mount_manager_,
-      std::move(smbfs_remote_), std::move(delegate_pending_receiver_));
+      std::make_unique<chromeos::disks::MountPoint>(base::FilePath(kMountPath),
+                                                    &mock_disk_mount_manager_),
+      &mock_delegate_, std::move(smbfs_remote_),
+      std::move(delegate_pending_receiver_));
   run_loop.RunUntilIdle();
   host.reset();
 }

chromeos/components/smbfs/smbfs_mounter.cc

@@ -4,6 +4,8 @@
 
 #include "chromeos/components/smbfs/smbfs_mounter.h"
 
+#include <utility>
+
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/strings/strcat.h"
@@ -50,10 +52,6 @@ SmbFsMounter::~SmbFsMounter() {
     mojo_bootstrap::PendingConnectionManager::Get()
         .CancelExpectedOpenIpcChannel(token_);
   }
-  if (disk_mount_manager_) {
-    // Can be nullptr in unit tests.
-    disk_mount_manager_->RemoveObserver(this);
-  }
 }
 
 void SmbFsMounter::Mount(SmbFsMounter::DoneCallback callback) {
@@ -73,42 +71,33 @@ void SmbFsMounter::Mount(SmbFsMounter::DoneCallback callback) {
   bootstrap_.set_disconnect_handler(
       base::BindOnce(&SmbFsMounter::OnMojoDisconnect, base::Unretained(this)));
 
-  disk_mount_manager_->AddObserver(this);
-  disk_mount_manager_->MountPath(mount_url_, "", mount_dir_name_, {},
-                                 chromeos::MOUNT_TYPE_NETWORK_STORAGE,
-                                 chromeos::MOUNT_ACCESS_MODE_READ_WRITE);
+  chromeos::disks::MountPoint::Mount(
+      disk_mount_manager_, mount_url_, "" /* source_format */, mount_dir_name_,
+      {} /* mount_options */, chromeos::MOUNT_TYPE_NETWORK_STORAGE,
+      chromeos::MOUNT_ACCESS_MODE_READ_WRITE,
+      base::BindOnce(&SmbFsMounter::OnMountDone, weak_factory_.GetWeakPtr()));
   mount_timer_.Start(
       FROM_HERE, kMountTimeout,
       base::BindOnce(&SmbFsMounter::OnMountTimeout, base::Unretained(this)));
 }
 
-void SmbFsMounter::OnMountEvent(
-    chromeos::disks::DiskMountManager::MountEvent event,
+void SmbFsMounter::OnMountDone(
     chromeos::MountError error_code,
-    const chromeos::disks::DiskMountManager::MountPointInfo& mount_info) {
+    std::unique_ptr<chromeos::disks::MountPoint> mount_point) {
   if (!callback_) {
     // This can happen if the mount timeout expires and the callback is already
     // run with a timeout error.
     return;
   }
 
-  if (mount_url_.empty() ||
-      mount_info.mount_type != chromeos::MOUNT_TYPE_NETWORK_STORAGE ||
-      mount_info.source_path != mount_url_ ||
-      event != chromeos::disks::DiskMountManager::MOUNTING) {
-    return;
-  }
-
-  disk_mount_manager_->RemoveObserver(this);
-
   if (error_code != chromeos::MOUNT_ERROR_NONE) {
     LOG(WARNING) << "smbfs mount error: " << error_code;
     ProcessMountError(mojom::MountError::kUnknown);
     return;
   }
 
-  DCHECK(!mount_info.mount_path.empty());
-  mount_path_ = mount_info.mount_path;
+  DCHECK(mount_point);
+  mount_point_ = std::move(mount_point);
 
   mojom::MountOptionsPtr mount_options = mojom::MountOptions::New();
   mount_options->share_path = share_path_;
@@ -170,10 +159,11 @@ void SmbFsMounter::OnMountShare(
     return;
   }
 
-  std::unique_ptr<SmbFsHost> host = std::make_unique<SmbFsHost>(
-      base::FilePath(mount_path_), delegate_, disk_mount_manager_,
-      mojo::Remote<mojom::SmbFs>(std::move(smbfs)),
-      std::move(delegate_receiver));
+  DCHECK(mount_point_);
+  std::unique_ptr<SmbFsHost> host =
+      std::make_unique<SmbFsHost>(std::move(mount_point_), delegate_,
+                                  mojo::Remote<mojom::SmbFs>(std::move(smbfs)),
+                                  std::move(delegate_receiver));
   std::move(callback_).Run(mojom::MountError::kOk, std::move(host));
 }
 
@@ -196,15 +186,7 @@ void SmbFsMounter::OnMountTimeout() {
 }
 
 void SmbFsMounter::ProcessMountError(mojom::MountError mount_error) {
-  if (!mount_path_.empty()) {
-    disk_mount_manager_->UnmountPath(
-        mount_path_, base::BindOnce([](chromeos::MountError error_code) {
-          LOG_IF(WARNING, error_code != chromeos::MOUNT_ERROR_NONE)
-              << "Error unmounting smbfs on setup failure: " << error_code;
-        }));
-    mount_path_ = {};
-  }
-
+  mount_point_.reset();
   std::move(callback_).Run(mount_error, nullptr);
 }
 

chromeos/components/smbfs/smbfs_mounter.h

@@ -12,11 +12,13 @@
 #include "base/component_export.h"
 #include "base/files/scoped_file.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
 #include "base/unguessable_token.h"
 #include "chromeos/components/smbfs/mojom/smbfs.mojom.h"
 #include "chromeos/components/smbfs/smbfs_host.h"
 #include "chromeos/disks/disk_mount_manager.h"
+#include "chromeos/disks/mount_point.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/system/invitation.h"
 
@@ -25,8 +27,7 @@ namespace smbfs {
 // SmbFsMounter is a helper class that is used to mount an instance of smbfs. It
 // performs all the actions necessary to start smbfs and initiate a connection
 // to the SMB server.
-class COMPONENT_EXPORT(SMBFS) SmbFsMounter
-    : public chromeos::disks::DiskMountManager::Observer {
+class COMPONENT_EXPORT(SMBFS) SmbFsMounter {
  public:
   using DoneCallback =
       base::OnceCallback<void(mojom::MountError, std::unique_ptr<SmbFsHost>)>;
@@ -50,7 +51,7 @@ class COMPONENT_EXPORT(SMBFS) SmbFsMounter
                const MountOptions& options,
                SmbFsHost::Delegate* delegate,
                chromeos::disks::DiskMountManager* disk_mount_manager);
-  ~SmbFsMounter() override;
+  virtual ~SmbFsMounter();
 
   // Initiate the filesystem mount request, and run |callback| when completed.
   // |callback| is guaranteed not to run after |this| is destroyed.
@@ -61,11 +62,9 @@ class COMPONENT_EXPORT(SMBFS) SmbFsMounter
   SmbFsMounter();
 
  private:
-  // DiskMountManager::Observer overrides.
-  void OnMountEvent(chromeos::disks::DiskMountManager::MountEvent event,
-                    chromeos::MountError error_code,
-                    const chromeos::disks::DiskMountManager::MountPointInfo&
-                        mount_info) override;
+  // Callback for MountPoint::Mount().
+  void OnMountDone(chromeos::MountError error_code,
+                   std::unique_ptr<chromeos::disks::MountPoint> mount_point);
 
   // Callback for receiving a Mojo bootstrap channel.
   void OnIpcChannel(base::ScopedFD mojo_fd);
@@ -97,10 +96,12 @@ class COMPONENT_EXPORT(SMBFS) SmbFsMounter
   base::OneShotTimer mount_timer_;
   DoneCallback callback_;
 
-  std::string mount_path_;
+  std::unique_ptr<chromeos::disks::MountPoint> mount_point_;
   mojo::OutgoingInvitation bootstrap_invitation_;
   mojo::Remote<mojom::SmbFsBootstrap> bootstrap_;
 
+  base::WeakPtrFactory<SmbFsMounter> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(SmbFsMounter);
 };