ArcFileSystemBridge: Refactor OpenFileToRead()

Refactor OpenFileToRead() to prepare for MTP support in ARCVM.

1. In OpenFileToRead(), the main work for MTP flow is refactored into
GenerateVirtualFileId() and OpenFileById().
2. Added 2 new FileSystemHost mojo call: GetVirtualFileId() and
HandleIdReleased().

BUG=b:158606367
TEST=As a regression test, in ARC++, a file can still be opened
with an Android app via MTP successfully.

Change-Id: Iea6cedd37903f94138428016b663e225d05138dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2444757Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Commit-Queue: Cherie Cheung <cherieccy@google.com>
Cr-Commit-Position: refs/heads/master@{#821663}

chrome/browser/chromeos/arc/fileapi/arc_file_system_bridge.cc

@@ -244,6 +244,12 @@ void ArcFileSystemBridge::GetFileSize(const std::string& url,
     std::move(callback).Run(-1);
     return;
   }
+
+  GetFileSizeInternal(url_decoded, std::move(callback));
+}
+
+void ArcFileSystemBridge::GetFileSizeInternal(const GURL& url_decoded,
+                                              GetFileSizeCallback callback) {
   scoped_refptr<storage::FileSystemContext> context =
       GetFileSystemContext(profile_, url_decoded);
   file_manager::util::FileSystemURLAndHandle file_system_url_and_handle =
@@ -297,6 +303,24 @@ void ArcFileSystemBridge::OnRootsChanged() {
     observer.OnRootsChanged();
 }
 
+void ArcFileSystemBridge::GetVirtualFileId(const std::string& url,
+                                           GetVirtualFileIdCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  GURL url_decoded = DecodeFromChromeContentProviderUrl(GURL(url));
+  if (url_decoded.is_empty() || !IsUrlAllowed(url_decoded)) {
+    LOG(ERROR) << "Invalid URL: " << url << " " << url_decoded;
+    std::move(callback).Run(base::nullopt);
+    return;
+  }
+
+  GetVirtualFileIdInternal(url_decoded, std::move(callback));
+}
+
+void ArcFileSystemBridge::HandleIdReleased(const std::string& id,
+                                           HandleIdReleasedCallback callback) {
+  std::move(callback).Run(HandleIdReleased(id));
+}
+
 void ArcFileSystemBridge::OpenFileToRead(const std::string& url,
                                          OpenFileToReadCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -313,10 +337,10 @@ void ArcFileSystemBridge::OpenFileToRead(const std::string& url,
   // If either DriveFS is not enabled/not mounted, or the URL doesn't represent
   // drivefs file (e.g., FSP, MTP), use VirtualFileProvider instead.
   if (fs_path.empty()) {
-    GetFileSize(url, base::BindOnce(
-                         &ArcFileSystemBridge::OpenFileToReadAfterGetFileSize,
-                         weak_ptr_factory_.GetWeakPtr(), url_decoded,
-                         std::move(callback)));
+    GetVirtualFileIdInternal(
+        url_decoded, base::BindOnce(&ArcFileSystemBridge::OpenFileById,
+                                    weak_ptr_factory_.GetWeakPtr(), url_decoded,
+                                    std::move(callback)));
     return;
   }
 
@@ -325,6 +349,15 @@ void ArcFileSystemBridge::OpenFileToRead(const std::string& url,
       base::BindOnce(&OpenDriveFSFileToRead, fs_path), std::move(callback));
 }
 
+void ArcFileSystemBridge::GetVirtualFileIdInternal(
+    const GURL& url_decoded,
+    GetVirtualFileIdCallback callback) {
+  GetFileSizeInternal(
+      url_decoded, base::BindOnce(&ArcFileSystemBridge::GenerateVirtualFileId,
+                                  weak_ptr_factory_.GetWeakPtr(), url_decoded,
+                                  std::move(callback)));
+}
+
 void ArcFileSystemBridge::SelectFiles(mojom::SelectFilesRequestPtr request,
                                       SelectFilesCallback callback) {
   select_files_handlers_manager_->SelectFiles(std::move(request),
@@ -352,39 +385,73 @@ void ArcFileSystemBridge::GetFileSelectorElements(
                                                           std::move(callback));
 }
 
-void ArcFileSystemBridge::OpenFileToReadAfterGetFileSize(
+void ArcFileSystemBridge::GenerateVirtualFileId(
     const GURL& url_decoded,
-    OpenFileToReadCallback callback,
+    GenerateVirtualFileIdCallback callback,
     int64_t size) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (size < 0) {
     LOG(ERROR) << "Failed to get file size " << url_decoded;
+    std::move(callback).Run(base::nullopt);
+    return;
+  }
+  chromeos::DBusThreadManager::Get()
+      ->GetVirtualFileProviderClient()
+      ->GenerateVirtualFileId(
+          size, base::BindOnce(&ArcFileSystemBridge::OnGenerateVirtualFileId,
+                               weak_ptr_factory_.GetWeakPtr(), url_decoded,
+                               std::move(callback)));
+}
+
+void ArcFileSystemBridge::OnGenerateVirtualFileId(
+    const GURL& url_decoded,
+    GenerateVirtualFileIdCallback callback,
+    const base::Optional<std::string>& id) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(id.has_value());
+  DCHECK_EQ(id_to_url_.count(id.value()), 0u);
+  id_to_url_[id.value()] = url_decoded;
+
+  std::move(callback).Run(std::move(id));
+}
+
+void ArcFileSystemBridge::OpenFileById(const GURL& url_decoded,
+                                       OpenFileToReadCallback callback,
+                                       const base::Optional<std::string>& id) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!id.has_value()) {
+    LOG(ERROR) << "Missing ID";
     std::move(callback).Run(mojo::ScopedHandle());
     return;
   }
-  chromeos::DBusThreadManager::Get()->GetVirtualFileProviderClient()->OpenFile(
-      size, base::BindOnce(&ArcFileSystemBridge::OnOpenFile,
-                           weak_ptr_factory_.GetWeakPtr(), url_decoded,
-                           std::move(callback)));
+
+  chromeos::DBusThreadManager::Get()
+      ->GetVirtualFileProviderClient()
+      ->OpenFileById(id.value(),
+                     base::BindOnce(&ArcFileSystemBridge::OnOpenFileById,
+                                    weak_ptr_factory_.GetWeakPtr(), url_decoded,
+                                    std::move(callback), id.value()));
 }
 
-void ArcFileSystemBridge::OnOpenFile(const GURL& url_decoded,
-                                     OpenFileToReadCallback callback,
-                                     const std::string& id,
-                                     base::ScopedFD fd) {
+void ArcFileSystemBridge::OnOpenFileById(const GURL& url_decoded,
+                                         OpenFileToReadCallback callback,
+                                         const std::string& id,
+                                         base::ScopedFD fd) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!fd.is_valid()) {
     LOG(ERROR) << "Invalid FD";
+    if (!HandleIdReleased(id))
+      LOG(ERROR) << "Cannot release ID: " << id;
     std::move(callback).Run(mojo::ScopedHandle());
     return;
   }
-  DCHECK_EQ(id_to_url_.count(id), 0u);
-  id_to_url_[id] = url_decoded;
 
   mojo::ScopedHandle wrapped_handle =
       mojo::WrapPlatformHandle(mojo::PlatformHandle(std::move(fd)));
   if (!wrapped_handle.is_valid()) {
     LOG(ERROR) << "Failed to wrap handle";
+    if (!HandleIdReleased(id))
+      LOG(ERROR) << "Cannot release ID: " << id;
     std::move(callback).Run(mojo::ScopedHandle());
     return;
   }

chrome/browser/chromeos/arc/fileapi/arc_file_system_bridge.h

@@ -89,6 +89,10 @@ class ArcFileSystemBridge
   void OnDocumentChanged(int64_t watcher_id,
                          storage::WatcherManager::ChangeType type) override;
   void OnRootsChanged() override;
+  void GetVirtualFileId(const std::string& url,
+                        GetVirtualFileIdCallback callback) override;
+  void HandleIdReleased(const std::string& id,
+                        HandleIdReleasedCallback callback) override;
   void OpenFileToRead(const std::string& url,
                       OpenFileToReadCallback callback) override;
   void SelectFiles(mojom::SelectFilesRequestPtr request,
@@ -103,16 +107,37 @@ class ArcFileSystemBridge
   void OnConnectionClosed() override;
 
  private:
+  using GenerateVirtualFileIdCallback =
+      base::OnceCallback<void(const base::Optional<std::string>& id)>;
+
+  // Used to implement GetFileSize().
+  void GetFileSizeInternal(const GURL& url_decoded,
+                           GetFileSizeCallback callback);
+
+  // Used to implement GetVirtualFileId().
+  void GetVirtualFileIdInternal(const GURL& url_decoded,
+                                GetVirtualFileIdCallback callback);
+
+  // Used to implement GetVirtualFileId().
+  void GenerateVirtualFileId(const GURL& url_decoded,
+                             GenerateVirtualFileIdCallback callback,
+                             int64_t size);
+
+  // Used to implement GetVirtualFileId().
+  void OnGenerateVirtualFileId(const GURL& url_decoded,
+                               GenerateVirtualFileIdCallback callback,
+                               const base::Optional<std::string>& id);
+
   // Used to implement OpenFileToRead().
-  void OpenFileToReadAfterGetFileSize(const GURL& url_decoded,
-                                      OpenFileToReadCallback callback,
-                                      int64_t size);
+  void OpenFileById(const GURL& url_decoded,
+                    OpenFileToReadCallback callback,
+                    const base::Optional<std::string>& id);
 
   // Used to implement OpenFileToRead().
-  void OnOpenFile(const GURL& url_decoded,
-                  OpenFileToReadCallback callback,
-                  const std::string& id,
-                  base::ScopedFD fd);
+  void OnOpenFileById(const GURL& url_decoded,
+                      OpenFileToReadCallback callback,
+                      const std::string& id,
+                      base::ScopedFD fd);
 
   // Called when FileStreamForwarder completes read request.
   void OnReadRequestCompleted(const std::string& id,

chrome/browser/chromeos/arc/fileapi/arc_file_system_bridge_unittest.cc

@@ -190,6 +190,34 @@ TEST_F(ArcFileSystemBridgeTest, GetFileType) {
   run_loop.Run();
 }
 
+TEST_F(ArcFileSystemBridgeTest, GetVirtualFileId) {
+  // Set up fake virtual file provider client.
+  constexpr char kId[] = "testfile";
+  auto* fake_client = static_cast<chromeos::FakeVirtualFileProviderClient*>(
+      chromeos::DBusThreadManager::Get()->GetVirtualFileProviderClient());
+  fake_client->set_expected_size(kTestFileSize);
+  fake_client->set_result_id(kId);
+
+  // GetVirtualFileId().
+  base::RunLoop run_loop;
+  arc_file_system_bridge_->GetVirtualFileId(
+      EncodeToChromeContentProviderUrl(GURL(kTestUrl)).spec(),
+      base::BindOnce(
+          [](base::RunLoop* run_loop, const char* kId,
+             const base::Optional<std::string>& id) {
+            ASSERT_NE(base::nullopt, id);
+            EXPECT_EQ(kId, id.value());
+            run_loop->Quit();
+          },
+          &run_loop, kId));
+  run_loop.Run();
+
+  content::RunAllTasksUntilIdle();
+
+  // ID is released.
+  EXPECT_TRUE(arc_file_system_bridge_->HandleIdReleased(kId));
+}
+
 TEST_F(ArcFileSystemBridgeTest, OpenFileToRead) {
   // Set up fake virtual file provider client.
   base::FilePath temp_path;

chromeos/dbus/fake_virtual_file_provider_client.cc

@@ -16,18 +16,30 @@ FakeVirtualFileProviderClient::~FakeVirtualFileProviderClient() = default;
 
 void FakeVirtualFileProviderClient::Init(dbus::Bus* bus) {}
 
-void FakeVirtualFileProviderClient::OpenFile(int64_t size,
-                                             OpenFileCallback callback) {
-  std::string id;
-  base::ScopedFD fd;
-  if (size != expected_size_) {
+void FakeVirtualFileProviderClient::GenerateVirtualFileId(
+    int64_t size,
+    GenerateVirtualFileIdCallback callback) {
+  base::Optional<std::string> id;
+  if (size != expected_size_)
     LOG(ERROR) << "Unexpected size " << size << " vs " << expected_size_;
-  } else {
+  else
     id = result_id_;
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callback), std::move(id)));
+}
+
+void FakeVirtualFileProviderClient::OpenFileById(
+    const std::string& id,
+    OpenFileByIdCallback callback) {
+  base::ScopedFD fd;
+  if (id != result_id_)
+    LOG(ERROR) << "Unexpected id " << id << " vs " << result_id_;
+  else
     fd = std::move(result_fd_);
-  }
+
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(std::move(callback), id, std::move(fd)));
+      FROM_HERE, base::BindOnce(std::move(callback), std::move(fd)));
 }
 
 }  // namespace chromeos

chromeos/dbus/fake_virtual_file_provider_client.h

@@ -22,16 +22,19 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeVirtualFileProviderClient
   void Init(dbus::Bus* bus) override;
 
   // VirtualFileProviderClient overrides:
-  void OpenFile(int64_t size, OpenFileCallback callback) override;
+  void GenerateVirtualFileId(int64_t size,
+                             GenerateVirtualFileIdCallback callback) override;
+  void OpenFileById(const std::string& id,
+                    OpenFileByIdCallback callback) override;
 
   void set_expected_size(int64_t size) { expected_size_ = size; }
   void set_result_id(const std::string& id) { result_id_ = id; }
   void set_result_fd(base::ScopedFD fd) { result_fd_ = std::move(fd); }
 
  private:
-  int64_t expected_size_ = 0;  // Expectation for OpenFile.
-  std::string result_id_;      // Returned by OpenFile.
-  base::ScopedFD result_fd_;   // Returned by OpenFile.
+  int64_t expected_size_ = 0;  // Expectation for GenerateVirtualFileId.
+  std::string result_id_;      // Returned by GenerateVirtualFileId.
+  base::ScopedFD result_fd_;   // Returned by OpenFileById.
 
   DISALLOW_COPY_AND_ASSIGN(FakeVirtualFileProviderClient);
 };

chromeos/dbus/virtual_file_provider_client.cc

@@ -25,15 +25,28 @@ class VirtualFileProviderClientImpl : public VirtualFileProviderClient {
   ~VirtualFileProviderClientImpl() override = default;
 
   // VirtualFileProviderClient override:
-  void OpenFile(int64_t size, OpenFileCallback callback) override {
+  void GenerateVirtualFileId(int64_t size,
+                             GenerateVirtualFileIdCallback callback) override {
     dbus::MethodCall method_call(
         virtual_file_provider::kVirtualFileProviderInterface,
-        virtual_file_provider::kOpenFileMethod);
+        virtual_file_provider::kGenerateVirtualFileIdMethod);
     dbus::MessageWriter writer(&method_call);
     writer.AppendInt64(size);
     proxy_->CallMethod(
         &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&VirtualFileProviderClientImpl::OnOpenFile,
+        base::BindOnce(&VirtualFileProviderClientImpl::OnGenerateVirtualFileId,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
+  }
+  void OpenFileById(const std::string& id,
+                    OpenFileByIdCallback callback) override {
+    dbus::MethodCall method_call(
+        virtual_file_provider::kVirtualFileProviderInterface,
+        virtual_file_provider::kOpenFileByIdMethod);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendString(std::move(id));
+    proxy_->CallMethod(
+        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::BindOnce(&VirtualFileProviderClientImpl::OnOpenFileById,
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
@@ -47,21 +60,37 @@ class VirtualFileProviderClientImpl : public VirtualFileProviderClient {
   }
 
  private:
-  // Runs the callback with OpenFile method call result.
-  void OnOpenFile(OpenFileCallback callback, dbus::Response* response) {
+  // Runs the callback with GenerateVirtualFileId method call result.
+  void OnGenerateVirtualFileId(GenerateVirtualFileIdCallback callback,
+                               dbus::Response* response) {
     if (!response) {
-      std::move(callback).Run(std::string(), base::ScopedFD());
+      std::move(callback).Run(base::nullopt);
       return;
     }
     dbus::MessageReader reader(response);
     std::string id;
+    if (!reader.PopString(&id)) {
+      LOG(ERROR) << "Invalid method call result.";
+      std::move(callback).Run(base::nullopt);
+      return;
+    }
+    std::move(callback).Run(std::move(id));
+  }
+
+  // Runs the callback with OpenFileById method call result.
+  void OnOpenFileById(OpenFileByIdCallback callback, dbus::Response* response) {
+    if (!response) {
+      std::move(callback).Run(base::ScopedFD());
+      return;
+    }
+    dbus::MessageReader reader(response);
     base::ScopedFD fd;
-    if (!reader.PopString(&id) || !reader.PopFileDescriptor(&fd)) {
+    if (!reader.PopFileDescriptor(&fd)) {
       LOG(ERROR) << "Invalid method call result.";
-      std::move(callback).Run(std::string(), base::ScopedFD());
+      std::move(callback).Run(base::ScopedFD());
       return;
     }
-    std::move(callback).Run(id, std::move(fd));
+    std::move(callback).Run(std::move(fd));
   }
 
   dbus::ObjectProxy* proxy_ = nullptr;

chromeos/dbus/virtual_file_provider_client.h

@@ -14,6 +14,7 @@
 #include "base/component_export.h"
 #include "base/files/scoped_file.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "chromeos/dbus/dbus_client.h"
 
 namespace chromeos {
@@ -26,8 +27,9 @@ namespace chromeos {
 class COMPONENT_EXPORT(CHROMEOS_DBUS) VirtualFileProviderClient
     : public DBusClient {
  public:
-  using OpenFileCallback =
-      base::OnceCallback<void(const std::string& id, base::ScopedFD fd)>;
+  using GenerateVirtualFileIdCallback =
+      base::OnceCallback<void(const base::Optional<std::string>& id)>;
+  using OpenFileByIdCallback = base::OnceCallback<void(base::ScopedFD fd)>;
 
   VirtualFileProviderClient();
   ~VirtualFileProviderClient() override;
@@ -36,10 +38,16 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) VirtualFileProviderClient
   // For normal usage, access the singleton via DBusThreadManager::Get().
   static std::unique_ptr<VirtualFileProviderClient> Create();
 
-  // Creates a new file descriptor and returns it with a unique ID.
-  // |size| will be used to perform boundary check when FD is seeked.
-  // When the FD is read, the read request is forwarded to the request handler.
-  virtual void OpenFile(int64_t size, OpenFileCallback callback) = 0;
+  // Generates and returns a unique ID, to be used by OpenFileById() for FD
+  // creation. |size| will be used to perform boundary check when FD is seeked.
+  virtual void GenerateVirtualFileId(
+      int64_t size,
+      GenerateVirtualFileIdCallback callback) = 0;
+
+  // Given a unique ID, creates a new file descriptor. When the FD is read,
+  // the read request is forwarded to the request handler.
+  virtual void OpenFileById(const std::string& id,
+                            OpenFileByIdCallback callback) = 0;
 };
 
 }  // namespace chromeos

components/arc/mojom/file_system.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 MinVersion: 16
+// Next MinVersion: 17
 
 module arc.mojom;
 
@@ -249,7 +249,7 @@ struct FileSelectorElements {
   string? search_query;
 };
 
-// Next method ID: 9
+// Next method ID: 11
 interface FileSystemHost {
   // Returns the name of the file specified by the URL.
   // When an error occurs, returns null value.
@@ -271,6 +271,14 @@ interface FileSystemHost {
   // Called when the list of available roots or their metadata are changed.
   [MinVersion=10] OnRootsChanged@6();
 
+  // Returns a unique ID to represent the file specified by the URL.
+  // The FD needs to be created by the caller itself.
+  [MinVersion=16] GetVirtualFileId@9(string url) => (string? id);
+
+  // Releases the ID generated by GetVirtualFileId() when it is no longer
+  // used.
+  [MinVersion=16] HandleIdReleased@10(string id) => (bool success);
+
   // Returns an FD for reading the file specified by the URL.
   [MinVersion=6] OpenFileToRead@4(string url) => (handle? fd);