Reland "Observe file modifications in FileChangeService"

This is a reland of c52857a9

The original cl missed calls to run_loop.Run() in test methods
that wrote to a file, and waited for the write to finish.

TBR=dats@chromium.org

Original change's description:
> Observe file modifications in FileChangeService
>
> Adds OnFileModified observer interface to FileChangeServiceObserver
> called when the file is written, truncated, or copied to.
>
> Fixes file change service unit tests to verify that mocked observer
> methods actually get called. The observers were previously not
> getting invoked because the test suite did not set up fake chrome
> user manager, so user account IDs were not correctly mapped to the
> associated test profile, which is something
> ObservableFileSystemOperationImpl depends on to get the
> FileChangeService to notify of file changes.
>
> BUG=1139115
>
> Change-Id: I9829db0a496eec301a7f7547ac74f61bc1a18bdf
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2603467
> Reviewed-by: Sergei Datsenko <dats@chromium.org>
> Reviewed-by: David Black <dmblack@google.com>
> Commit-Queue: Toni Baržić <tbarzic@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#839697}

Bug: 1139115
Change-Id: I70e23cd1b11c9168ee99d8ed314942d25bd4a004
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2607415
Commit-Queue: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#839780}

chrome/browser/chromeos/fileapi/file_change_service.cc

@@ -18,6 +18,11 @@ void FileChangeService::RemoveObserver(FileChangeServiceObserver* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
+void FileChangeService::NotifyFileModified(const storage::FileSystemURL& url) {
+  for (FileChangeServiceObserver& observer : observer_list_)
+    observer.OnFileModified(url);
+}
+
 void FileChangeService::NotifyFileCopied(const storage::FileSystemURL& src,
                                          const storage::FileSystemURL& dst) {
   for (FileChangeServiceObserver& observer : observer_list_)

chrome/browser/chromeos/fileapi/file_change_service.h

@@ -25,6 +25,9 @@ class FileChangeService : public KeyedService {
   void AddObserver(FileChangeServiceObserver* observer);
   void RemoveObserver(FileChangeServiceObserver* observer);
 
+  // Notifies the service that a file identified by `url` has been modified.
+  void NotifyFileModified(const storage::FileSystemURL& url);
+
   // Notifies the service that a file has been copied from `src` to `dst`.
   void NotifyFileCopied(const storage::FileSystemURL& src,
                         const storage::FileSystemURL& dst);

chrome/browser/chromeos/fileapi/file_change_service_observer.h

@@ -16,6 +16,10 @@ namespace chromeos {
 // An interface for an observer which receives `FileChangeService` events.
 class FileChangeServiceObserver : public base::CheckedObserver {
  public:
+  // Invoked when a file identified by `url` has been modified. Note that this
+  // will not get called on file creation or deletion.
+  virtual void OnFileModified(const storage::FileSystemURL& url) {}
+
   // Invoked when a file has been copied from `src` to `dst`.
   virtual void OnFileCopied(const storage::FileSystemURL& src,
                             const storage::FileSystemURL& dst) {}

chrome/browser/chromeos/fileapi/file_change_service_unittest.cc

@@ -6,16 +6,24 @@
 
 #include "base/files/scoped_temp_dir.h"
 #include "base/scoped_observation.h"
+#include "base/test/bind.h"
 #include "base/unguessable_token.h"
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/chromeos/file_manager/fileapi_util.h"
 #include "chrome/browser/chromeos/fileapi/file_change_service_factory.h"
 #include "chrome/browser/chromeos/fileapi/file_change_service_observer.h"
+#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/testing_profile_manager.h"
+#include "components/user_manager/scoped_user_manager.h"
+#include "mojo/public/cpp/system/data_pipe.h"
+#include "mojo/public/cpp/system/data_pipe_producer.h"
+#include "mojo/public/cpp/system/string_data_source.h"
+#include "storage/browser/blob/blob_storage_context.h"
 #include "storage/browser/file_system/external_mount_points.h"
 #include "storage/browser/file_system/file_system_operation_runner.h"
 #include "storage/browser/test/async_file_test_helper.h"
+#include "storage/browser/test/mock_blob_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 namespace chromeos {
@@ -37,11 +45,40 @@ storage::FileSystemOperationRunner* GetFileSystemOperationRunner(
   return GetFileSystemContext(profile)->operation_runner();
 }
 
+// Creates a mojo data pipe with the provided `content`.
+mojo::ScopedDataPipeConsumerHandle CreateStream(const std::string& contents) {
+  mojo::ScopedDataPipeProducerHandle producer_handle;
+  mojo::ScopedDataPipeConsumerHandle consumer_handle;
+  MojoCreateDataPipeOptions options;
+  options.struct_size = sizeof(MojoCreateDataPipeOptions);
+  options.flags = MOJO_CREATE_DATA_PIPE_FLAG_NONE;
+  options.element_num_bytes = 1;
+  options.capacity_num_bytes = 16;
+  mojo::CreateDataPipe(&options, &producer_handle, &consumer_handle);
+  CHECK(producer_handle.is_valid());
+  auto producer =
+      std::make_unique<mojo::DataPipeProducer>(std::move(producer_handle));
+  auto* producer_raw = producer.get();
+  producer_raw->Write(
+      std::make_unique<mojo::StringDataSource>(
+          contents, mojo::StringDataSource::AsyncWritingMode::
+                        STRING_MAY_BE_INVALIDATED_BEFORE_COMPLETION),
+      base::BindOnce(
+          base::DoNothing::Once<std::unique_ptr<mojo::DataPipeProducer>,
+                                MojoResult>(),
+          std::move(producer)));
+  return consumer_handle;
+}
+
 // MockFileChangeServiceObserver -----------------------------------------------
 
 class MockFileChangeServiceObserver : public FileChangeServiceObserver {
  public:
   // FileChangeServiceObserver:
+  MOCK_METHOD(void,
+              OnFileModified,
+              (const storage::FileSystemURL& url),
+              (override));
   MOCK_METHOD(void,
               OnFileCopied,
               (const storage::FileSystemURL& src,
@@ -98,6 +135,52 @@ class TempFileSystem {
         temp_dir_.GetPath().Append(base::FilePath::FromUTF8Unsafe(path)));
   }
 
+  // Synchronously writes `content` to the file specified by `url`.
+  base::File::Error WriteFile(const storage::FileSystemURL& url,
+                              const std::string& data) {
+    storage::BlobStorageContext blob_storage_context;
+    storage::ScopedTextBlob blob(&blob_storage_context, "blob-id:test", data);
+    base::File::Error result = base::File::FILE_ERROR_FAILED;
+    base::RunLoop run_loop;
+    GetFileSystemContext(profile_)->operation_runner()->Write(
+        url, blob.GetBlobDataHandle(), 0,
+        base::BindLambdaForTesting([&](base::File::Error operation_result,
+                                       int64_t bytes, bool complete) {
+          if (!complete)
+            return;
+          result = operation_result;
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+    return result;
+  }
+
+  // Synchronously writes contents from `stream` to the file specified by `url`.
+  base::File::Error WriteStreamToFile(
+      const storage::FileSystemURL& url,
+      mojo::ScopedDataPipeConsumerHandle stream) {
+    base::File::Error result = base::File::FILE_ERROR_FAILED;
+    base::RunLoop run_loop;
+    GetFileSystemContext(profile_)->operation_runner()->WriteStream(
+        url, std::move(stream), 0,
+        base::BindLambdaForTesting([&](base::File::Error operation_result,
+                                       int64_t bytes, bool complete) {
+          if (!complete)
+            return;
+          result = operation_result;
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+    return result;
+  }
+
+  // Synchronously truncates the file specified by `url` to `size`.
+  base::File::Error TruncateFile(const storage::FileSystemURL& url,
+                                 size_t size) {
+    storage::FileSystemContext* context = GetFileSystemContext(profile_);
+    return storage::AsyncFileTestHelper::TruncateFile(context, url, size);
+  }
+
   // Synchronously copies the file specified by `src` to `dst`.
   base::File::Error CopyFile(const storage::FileSystemURL& src,
                              const storage::FileSystemURL& dst) {
@@ -144,13 +227,19 @@ class TempFileSystem {
 
 class FileChangeServiceTest : public BrowserWithTestWindowTest {
  public:
-  FileChangeServiceTest() = default;
+  FileChangeServiceTest()
+      : fake_user_manager_(new chromeos::FakeChromeUserManager),
+        user_manager_enabler_(base::WrapUnique(fake_user_manager_)) {}
+
   FileChangeServiceTest(const FileChangeServiceTest& other) = delete;
   FileChangeServiceTest& operator=(const FileChangeServiceTest& other) = delete;
   ~FileChangeServiceTest() override = default;
 
   // Creates and returns a new profile for the specified `name`.
   TestingProfile* CreateProfileWithName(const std::string& name) {
+    const AccountId account_id(AccountId::FromUserEmail(name));
+    fake_user_manager_->AddUser(account_id);
+    fake_user_manager_->LoginUser(account_id);
     return profile_manager()->CreateTestingProfile(name);
   }
 
@@ -160,6 +249,9 @@ class FileChangeServiceTest : public BrowserWithTestWindowTest {
     constexpr char kPrimaryProfileName[] = "primary_profile";
     return CreateProfileWithName(kPrimaryProfileName);
   }
+
+  chromeos::FakeChromeUserManager* fake_user_manager_;
+  user_manager::ScopedUserManager user_manager_enabler_;
 };
 
 }  // namespace
@@ -205,16 +297,61 @@ TEST_F(FileChangeServiceTest, PropagatesOnFileCopiedEvents) {
 
   ASSERT_EQ(temp_file_system.CreateFile(src), base::File::FILE_OK);
 
-  EXPECT_CALL(mock_observer, OnFileCopied)
-      .WillRepeatedly([&](const storage::FileSystemURL& propagated_src,
-                          const storage::FileSystemURL& propagated_dst) {
-        EXPECT_EQ(src, propagated_src);
-        EXPECT_EQ(dst, propagated_dst);
+  EXPECT_CALL(mock_observer, OnFileModified)
+      .WillRepeatedly([&](const storage::FileSystemURL& propagated_url) {
+        EXPECT_EQ(dst, propagated_url);
       });
 
-  ASSERT_EQ(temp_file_system.CopyFile(src, dst), base::File::FILE_OK);
+  {
+    base::RunLoop copy_run_loop;
+    EXPECT_CALL(mock_observer, OnFileCopied)
+        .WillOnce([&](const storage::FileSystemURL& propagated_src,
+                      const storage::FileSystemURL& propagated_dst) {
+          EXPECT_EQ(src, propagated_src);
+          EXPECT_EQ(dst, propagated_dst);
+          copy_run_loop.Quit();
+        })
+        .RetiresOnSaturation();
+
+    base::RunLoop modify_run_loop;
+    EXPECT_CALL(mock_observer, OnFileModified)
+        .WillOnce([&](const storage::FileSystemURL& propagated_url) {
+          EXPECT_EQ(dst, propagated_url);
+          modify_run_loop.Quit();
+        })
+        .RetiresOnSaturation();
+
+    ASSERT_EQ(temp_file_system.CopyFile(src, dst), base::File::FILE_OK);
+    copy_run_loop.Run();
+    modify_run_loop.Run();
+  }
+
+  ::testing::Mock::VerifyAndClearExpectations(&mock_observer);
   ASSERT_EQ(temp_file_system.RemoveFile(dst), base::File::FILE_OK);
-  ASSERT_EQ(temp_file_system.CopyFileLocal(src, dst), base::File::FILE_OK);
+
+  {
+    base::RunLoop copy_run_loop;
+    EXPECT_CALL(mock_observer, OnFileCopied)
+        .WillOnce([&](const storage::FileSystemURL& propagated_src,
+                      const storage::FileSystemURL& propagated_dst) {
+          EXPECT_EQ(src, propagated_src);
+          EXPECT_EQ(dst, propagated_dst);
+          copy_run_loop.Quit();
+        })
+        .RetiresOnSaturation();
+
+    base::RunLoop modify_run_loop;
+    EXPECT_CALL(mock_observer, OnFileModified)
+        .WillOnce([&](const storage::FileSystemURL& propagated_url) {
+          EXPECT_EQ(dst, propagated_url);
+          modify_run_loop.Quit();
+        })
+        .RetiresOnSaturation();
+
+    ASSERT_EQ(temp_file_system.CopyFileLocal(src, dst), base::File::FILE_OK);
+    copy_run_loop.Run();
+    modify_run_loop.Run();
+  }
 }
 
 // Verifies `OnFileMoved` events are propagated to observers.
@@ -236,16 +373,115 @@ TEST_F(FileChangeServiceTest, PropagatesOnFileMovedEvents) {
 
   ASSERT_EQ(temp_file_system.CreateFile(src), base::File::FILE_OK);
 
-  EXPECT_CALL(mock_observer, OnFileMoved)
-      .WillRepeatedly([&](const storage::FileSystemURL& propagated_src,
-                          const storage::FileSystemURL& propagated_dst) {
-        EXPECT_EQ(src, propagated_src);
-        EXPECT_EQ(dst, propagated_dst);
-      });
+  {
+    base::RunLoop move_run_loop;
+    EXPECT_CALL(mock_observer, OnFileMoved)
+        // NOTE: `Move()` internally calls `MoveFileLocal()`, so move operation
+        // gets reported twice.
+        .WillOnce([&](const storage::FileSystemURL& propagated_src,
+                      const storage::FileSystemURL& propagated_dst) {
+          EXPECT_EQ(src, propagated_src);
+          EXPECT_EQ(dst, propagated_dst);
+        })
+        .WillOnce([&](const storage::FileSystemURL& propagated_src,
+                      const storage::FileSystemURL& propagated_dst) {
+          EXPECT_EQ(src, propagated_src);
+          EXPECT_EQ(dst, propagated_dst);
+          move_run_loop.Quit();
+        })
+        .RetiresOnSaturation();
+
+    EXPECT_CALL(mock_observer, OnFileModified).Times(0);
+
+    ASSERT_EQ(temp_file_system.MoveFile(src, dst), base::File::FILE_OK);
+    move_run_loop.Run();
+  }
+
+  ::testing::Mock::VerifyAndClearExpectations(&mock_observer);
+
+  {
+    base::RunLoop move_run_loop;
+    EXPECT_CALL(mock_observer, OnFileMoved)
+        .WillOnce([&](const storage::FileSystemURL& propagated_src,
+                      const storage::FileSystemURL& propagated_dst) {
+          EXPECT_EQ(dst, propagated_src);
+          EXPECT_EQ(src, propagated_dst);
+          move_run_loop.Quit();
+        })
+        .RetiresOnSaturation();
 
-  ASSERT_EQ(temp_file_system.MoveFile(src, dst), base::File::FILE_OK);
-  std::swap(dst, src);
-  ASSERT_EQ(temp_file_system.MoveFileLocal(src, dst), base::File::FILE_OK);
+    EXPECT_CALL(mock_observer, OnFileModified).Times(0);
+    ASSERT_EQ(temp_file_system.MoveFileLocal(dst, src), base::File::FILE_OK);
+
+    move_run_loop.Run();
+  }
+}
+
+// Verifies `OnFileModified` events are propagated to observers.
+TEST_F(FileChangeServiceTest, PropagatesOnFileModifiedEvents) {
+  auto* profile = GetProfile();
+  auto* service = FileChangeServiceFactory::GetInstance()->GetService(profile);
+  ASSERT_TRUE(service);
+
+  testing::NiceMock<MockFileChangeServiceObserver> mock_observer;
+  base::ScopedObservation<FileChangeService, FileChangeServiceObserver>
+      scoped_observation{&mock_observer};
+  scoped_observation.Observe(service);
+
+  TempFileSystem temp_file_system(profile);
+  temp_file_system.SetUp();
+
+  storage::FileSystemURL url =
+      temp_file_system.CreateFileSystemURL("test_file");
+
+  ASSERT_EQ(temp_file_system.CreateFile(url), base::File::FILE_OK);
+
+  // Test writing to file.
+  {
+    base::RunLoop modify_run_loop;
+    EXPECT_CALL(mock_observer, OnFileModified)
+        .WillOnce([&](const storage::FileSystemURL& propagated_url) {
+          EXPECT_EQ(url, propagated_url);
+          modify_run_loop.Quit();
+        })
+        .RetiresOnSaturation();
+
+    ASSERT_EQ(temp_file_system.WriteFile(url, "Test file contents\n"),
+              base::File::FILE_OK);
+    modify_run_loop.Run();
+  }
+
+  ::testing::Mock::VerifyAndClearExpectations(&mock_observer);
+
+  // Test truncating file.
+  {
+    base::RunLoop modify_run_loop;
+    EXPECT_CALL(mock_observer, OnFileModified)
+        .WillOnce([&](const storage::FileSystemURL& propagated_url) {
+          EXPECT_EQ(url, propagated_url);
+          modify_run_loop.Quit();
+        })
+        .RetiresOnSaturation();
+
+    ASSERT_EQ(temp_file_system.TruncateFile(url, 10), base::File::FILE_OK);
+    modify_run_loop.Run();
+  }
+
+  // Test writing a stream to file.
+  {
+    base::RunLoop modify_run_loop;
+    EXPECT_CALL(mock_observer, OnFileModified)
+        .WillOnce([&](const storage::FileSystemURL& propagated_url) {
+          EXPECT_EQ(url, propagated_url);
+          modify_run_loop.Quit();
+        })
+        .RetiresOnSaturation();
+
+    ASSERT_EQ(temp_file_system.WriteStreamToFile(
+                  url, CreateStream("Test file contents from stream")),
+              base::File::FILE_OK);
+    modify_run_loop.Run();
+  }
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/fileapi/observable_file_system_operation_impl.cc

@@ -16,6 +16,7 @@ namespace chromeos {
 namespace {
 
 using StatusCallback = storage::FileSystemOperation::StatusCallback;
+using WriteCallback = storage::FileSystemOperation::WriteCallback;
 
 // Helpers ---------------------------------------------------------------------
 
@@ -34,8 +35,10 @@ void NotifyFileCopiedOnUiThread(const AccountId& account_id,
                                 const storage::FileSystemURL& dst) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   FileChangeService* service = GetFileChangeService(account_id);
-  if (service)
+  if (service) {
+    service->NotifyFileModified(dst);
     service->NotifyFileCopied(src, dst);
+  }
 }
 
 // Notifies the `FileChangeService` associated with the given `account_id` of a
@@ -50,6 +53,28 @@ void NotifyFileMovedOnUiThread(const AccountId& account_id,
     service->NotifyFileMoved(src, dst);
 }
 
+// Notifies the `FileChangeService` associated with the given `account_id` of a
+// file under `url` getting modified. This method may only be called from the
+// browser UI thread.
+void NotifyFileModifiedOnUiThread(const AccountId& account_id,
+                                  const storage::FileSystemURL& url) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  FileChangeService* service = GetFileChangeService(account_id);
+  if (service)
+    service->NotifyFileModified(url);
+}
+
+// Returns a `WriteCallback` which runs the specified callbacks in order.
+WriteCallback RunInOrderCallback(WriteCallback a, WriteCallback b) {
+  return base::BindRepeating(
+      [](WriteCallback a, WriteCallback b, base::File::Error result,
+         int64_t bytes, bool complete) {
+        std::move(a).Run(result, bytes, complete);
+        std::move(b).Run(result, bytes, complete);
+      },
+      std::move(a), std::move(b));
+}
+
 // Returns a `StatusCallback` which runs the specified callbacks in order.
 StatusCallback RunInOrderCallback(StatusCallback a, StatusCallback b) {
   return base::BindOnce(
@@ -73,6 +98,21 @@ StatusCallback RunOnUiThreadOnSuccessCallback(base::OnceClosure closure) {
       std::move(closure));
 }
 
+// Returns a `WriteCallback` which runs the specified `closure` on the browser
+// UI thread if `complete` is set.
+WriteCallback RunOnUiThreadOnCompleteCallback(
+    const base::RepeatingClosure& closure) {
+  return base::BindRepeating(
+      [](const base::RepeatingClosure& closure, base::File::Error result,
+         int64_t bytes, bool complete) {
+        if (complete && result == base::File::FILE_OK) {
+          auto task_runner = content::GetUIThreadTaskRunner({});
+          task_runner->PostTask(FROM_HERE, std::move(closure));
+        }
+      },
+      std::move(closure));
+}
+
 }  // namespace
 
 // ObservableFileSystemOperationImpl -------------------------------------------
@@ -144,4 +184,39 @@ void ObservableFileSystemOperationImpl::MoveFileLocal(
           std::move(callback)));
 }
 
+void ObservableFileSystemOperationImpl::WriteBlob(
+    const storage::FileSystemURL& url,
+    std::unique_ptr<storage::FileWriterDelegate> writer_delegate,
+    std::unique_ptr<storage::BlobReader> blob_reader,
+    const WriteCallback& callback) {
+  storage::FileSystemOperationImpl::WriteBlob(
+      url, std::move(writer_delegate), std::move(blob_reader),
+      RunInOrderCallback(RunOnUiThreadOnCompleteCallback(base::BindRepeating(
+                             &NotifyFileModifiedOnUiThread, account_id_, url)),
+                         std::move(callback)));
+}
+
+void ObservableFileSystemOperationImpl::Write(
+    const storage::FileSystemURL& url,
+    std::unique_ptr<storage::FileWriterDelegate> writer_delegate,
+    mojo::ScopedDataPipeConsumerHandle data_pipe,
+    const WriteCallback& callback) {
+  storage::FileSystemOperationImpl::Write(
+      url, std::move(writer_delegate), std::move(data_pipe),
+      RunInOrderCallback(RunOnUiThreadOnCompleteCallback(base::BindRepeating(
+                             &NotifyFileModifiedOnUiThread, account_id_, url)),
+                         std::move(callback)));
+}
+
+void ObservableFileSystemOperationImpl::Truncate(
+    const storage::FileSystemURL& url,
+    int64_t length,
+    StatusCallback callback) {
+  storage::FileSystemOperationImpl::Truncate(
+      url, length,
+      RunInOrderCallback(RunOnUiThreadOnSuccessCallback(base::BindOnce(
+                             &NotifyFileModifiedOnUiThread, account_id_, url)),
+                         std::move(callback)));
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/fileapi/observable_file_system_operation_impl.h

@@ -49,6 +49,17 @@ class ObservableFileSystemOperationImpl
                      const storage::FileSystemURL& dst,
                      CopyOrMoveOption option,
                      StatusCallback callback) override;
+  void WriteBlob(const storage::FileSystemURL& url,
+                 std::unique_ptr<storage::FileWriterDelegate> writer_delegate,
+                 std::unique_ptr<storage::BlobReader> blob_reader,
+                 const WriteCallback& callback) override;
+  void Write(const storage::FileSystemURL& url,
+             std::unique_ptr<storage::FileWriterDelegate> writer_delegate,
+             mojo::ScopedDataPipeConsumerHandle data_pipe,
+             const WriteCallback& callback) override;
+  void Truncate(const storage::FileSystemURL& url,
+                int64_t length,
+                StatusCallback callback) override;
 
   const AccountId account_id_;
   base::WeakPtrFactory<ObservableFileSystemOperationImpl> weak_factory_{this};