Do not modify SmbService or SmbFileSystem on non-UI thread

- Previously SmbService and SmbFileSystem initialized their
temp_file_manager_ members via a Task which was run on a non-UI thread.
Changes to the class state should only happen on the UI thread to avoid
race conditions.

Bug: 879911
Change-Id: If79b0cd1162d2080c9ec1d44b0beb62cc53e8278
Reviewed-on: https://chromium-review.googlesource.com/1208193Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Commit-Queue: Bailey Berro <baileyberro@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589077}

chrome/browser/chromeos/smb_client/smb_file_system.cc

@@ -102,6 +102,10 @@ filesystem::mojom::FsFileType MapEntryType(bool is_directory) {
 
 constexpr size_t kTaskQueueCapacity = 2;
 
+std::unique_ptr<TempFileManager> CreateTempFileManager() {
+  return std::make_unique<TempFileManager>();
+}
+
 }  // namespace
 
 using file_system_provider::AbortCallback;
@@ -375,7 +379,7 @@ AbortCallback SmbFileSystem::WriteFile(
     SmbTask task = base::BindOnce(
         base::IgnoreResult(&SmbFileSystem::CallWriteFile), AsWeakPtr(),
         file_handle, std::move(data), offset, length, std::move(callback));
-    InitTempFileManagerAndExecuteTask(std::move(task));
+    CreateTempFileManagerAndExecuteTask(std::move(task));
     // The call to init temp_file_manager_ will not be abortable since it is
     // asynchronous.
     return CreateAbortCallback();
@@ -384,17 +388,27 @@ AbortCallback SmbFileSystem::WriteFile(
   return CallWriteFile(file_handle, data, offset, length, std::move(callback));
 }
 
-void SmbFileSystem::InitTempFileManagerAndExecuteTask(SmbTask task) {
-  // InitTempFileManager() has to be called on a separate thread since it
+void SmbFileSystem::CreateTempFileManagerAndExecuteTask(SmbTask task) {
+  // CreateTempFileManager() has to be called on a separate thread since it
   // contains a call that requires a blockable thread.
   base::TaskTraits task_traits = {base::MayBlock(),
                                   base::TaskPriority::USER_BLOCKING,
                                   base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN};
-  base::OnceClosure init_task = base::BindOnce(
-      &SmbFileSystem::InitTempFileManager, base::Unretained(this));
+  auto init_task = base::BindOnce(&CreateTempFileManager);
+  auto reply = base::BindOnce(&SmbFileSystem::InitTempFileManagerAndExecuteTask,
+                              AsWeakPtr(), std::move(task));
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE, task_traits, std::move(init_task), std::move(reply));
+}
+
+void SmbFileSystem::InitTempFileManagerAndExecuteTask(
+    SmbTask task,
+    std::unique_ptr<TempFileManager> temp_file_manager) {
+  DCHECK(!temp_file_manager_);
+  DCHECK(temp_file_manager);
 
-  base::PostTaskWithTraitsAndReply(FROM_HERE, task_traits, std::move(init_task),
-                                   std::move(task));
+  temp_file_manager_ = std::move(temp_file_manager);
+  std::move(task).Run();
 }
 
 AbortCallback SmbFileSystem::CallWriteFile(
@@ -715,11 +729,6 @@ void SmbFileSystem::HandleStatusCallback(
   std::move(callback).Run(TranslateToFileError(error));
 }
 
-void SmbFileSystem::InitTempFileManager() {
-  DCHECK(!temp_file_manager_);
-  temp_file_manager_ = std::make_unique<TempFileManager>();
-}
-
 base::WeakPtr<file_system_provider::ProvidedFileSystemInterface>
 SmbFileSystem::GetWeakPtr() {
   return AsWeakPtr();

chrome/browser/chromeos/smb_client/smb_file_system.h

@@ -174,11 +174,14 @@ class SmbFileSystem : public file_system_provider::ProvidedFileSystemInterface,
  private:
   void Abort(OperationId operation_id);
 
-  // Initializes temp_file_manager_.
-  void InitTempFileManager();
-
-  // Calls InitTempFileManager() and executes |task|.
-  void InitTempFileManagerAndExecuteTask(SmbTask task);
+  // Calls CreateTempFileManager() and executes |task|.
+  void CreateTempFileManagerAndExecuteTask(SmbTask task);
+
+  // Initializes |temp_file_manager_| with |temp_file_manager| and executes
+  // |task|.
+  void InitTempFileManagerAndExecuteTask(
+      SmbTask task,
+      std::unique_ptr<TempFileManager> temp_file_manager);
 
   // Calls WriteFile in SmbProviderClient.
   file_system_provider::AbortCallback CallWriteFile(

chrome/browser/chromeos/smb_client/smb_service.cc

@@ -70,6 +70,10 @@ void RecordRemountResult(SmbMountResult result) {
   UMA_HISTOGRAM_ENUMERATION("NativeSmbFileShare.RemountResult", result);
 }
 
+std::unique_ptr<TempFileManager> CreateTempFileManager() {
+  return std::make_unique<TempFileManager>();
+}
+
 }  // namespace
 
 bool SmbService::service_should_run_ = false;
@@ -258,10 +262,6 @@ void SmbService::OnRemountResponse(const std::string& file_system_id,
   }
 }
 
-void SmbService::InitTempFileManager() {
-  temp_file_manager_ = std::make_unique<TempFileManager>();
-}
-
 void SmbService::StartSetup() {
   user_manager::User* user =
       chromeos::ProfileHelper::Get()->GetUserByProfile(profile_);
@@ -291,18 +291,16 @@ void SmbService::StartSetup() {
 }
 
 void SmbService::SetupTempFileManagerAndCompleteSetup() {
-  // InitTempFileManager() has to be called on a separate thread since it
+  // CreateTempFileManager() has to be called on a separate thread since it
   // contains a call that requires a blockable thread.
   base::TaskTraits task_traits = {base::MayBlock(),
                                   base::TaskPriority::USER_BLOCKING,
                                   base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN};
-  base::OnceClosure task =
-      base::BindOnce(&SmbService::InitTempFileManager, base::Unretained(this));
-  base::OnceClosure reply =
-      base::BindOnce(&SmbService::CompleteSetup, AsWeakPtr());
+  auto task = base::BindOnce(&CreateTempFileManager);
+  auto reply = base::BindOnce(&SmbService::CompleteSetup, AsWeakPtr());
 
-  base::PostTaskWithTraitsAndReply(FROM_HERE, task_traits, std::move(task),
-                                   std::move(reply));
+  base::PostTaskWithTraitsAndReplyWithResult(FROM_HERE, task_traits,
+                                             std::move(task), std::move(reply));
 }
 
 void SmbService::OnSetupKerberosResponse(bool success) {
@@ -313,7 +311,12 @@ void SmbService::OnSetupKerberosResponse(bool success) {
   SetupTempFileManagerAndCompleteSetup();
 }
 
-void SmbService::CompleteSetup() {
+void SmbService::CompleteSetup(
+    std::unique_ptr<TempFileManager> temp_file_manager) {
+  DCHECK(temp_file_manager);
+  DCHECK(!temp_file_manager_);
+
+  temp_file_manager_ = std::move(temp_file_manager);
   share_finder_ = std::make_unique<SmbShareFinder>(GetSmbProviderClient());
   RegisterHostLocators();
 

chrome/browser/chromeos/smb_client/smb_service.h

@@ -77,10 +77,7 @@ class SmbService : public KeyedService,
   void GatherSharesInNetwork(GatherSharesResponse callback);
 
  private:
-  // Initializes |temp_file_manager_|. Must be called on a non-ui thread.
-  void InitTempFileManager();
-
-  // Calls SmbProviderClient::Mount(). temp_file_manager_ must be initialized
+  // Calls SmbProviderClient::Mount(). |temp_file_manager_| must be initialized
   // before this is called.
   void CallMount(const file_system_provider::MountOptions& options,
                  const base::FilePath& share_path,
@@ -118,7 +115,7 @@ class SmbService : public KeyedService,
 
   // Completes SmbService setup including ShareFinder initialization and
   // remounting shares. Called by SetupTempFileManagerAndCompleteSetup().
-  void CompleteSetup();
+  void CompleteSetup(std::unique_ptr<TempFileManager> temp_file_manager);
 
   // Handles the response from attempting to setup Kerberos.
   void OnSetupKerberosResponse(bool success);