Prohibiting multiple OpenFile's on a same GData path.

The patch is copied from kinaba's original version
http://codereview.chromium.org/10553032/

BUG=132966
TEST=unit_tests --gtest_filter='*GData*'
TEST=browser_tests --gtest_filter='*RemoteFileSystem*'

Review URL: https://chromiumcodereview.appspot.com/10563025

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@142790 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 97 insertions, 8 deletions

diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.cc b/chrome/browser/chromeos/gdata/gdata_file_system.cc
index 76fb085..fd39c6a 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.cc
@@ -3555,12 +3555,28 @@ void GDataFileSystem::OpenFileOnUIThread(const FilePath& file_path,
                                          const OpenFileCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // If the file is already opened, it cannot be opened again before closed.
+  // This is for avoiding simultaneous modification to the file, and moreover
+  // to avoid an inconsistent cache state (suppose an operation sequence like
+  // Open->Open->modify->Close->modify->Close; the second modify may not be
+  // synchronized to the server since it is already Closed on the cache).
+  if (open_files_.find(file_path) != open_files_.end()) {
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, base::PLATFORM_FILE_ERROR_IN_USE, FilePath()));
+    return;
+  }
+  open_files_.insert(file_path);
+
   GetFileInfoByPath(
       file_path,
       base::Bind(&GDataFileSystem::OnGetFileInfoCompleteForOpenFile,
                  ui_weak_ptr_,
                  file_path,
-                 callback));
+                 base::Bind(&GDataFileSystem::OnOpenFileFinished,
+                            ui_weak_ptr_,
+                            file_path,
+                            callback)));
 }
 
 void GDataFileSystem::OnGetFileInfoCompleteForOpenFile(
@@ -3635,6 +3651,22 @@ void GDataFileSystem::OnMarkDirtyInCacheCompleteForOpenFile(
     callback.Run(error, cache_file_path);
 }
 
+void GDataFileSystem::OnOpenFileFinished(const FilePath& file_path,
+                                         const OpenFileCallback& callback,
+                                         base::PlatformFileError result,
+                                         const FilePath& cache_file_path) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // All the invocation of |callback| from operations initiated from OpenFile
+  // must go through here. Removes the |file_path| from the remembered set when
+  // the file was not successfully opened.
+  if (result != base::PLATFORM_FILE_OK)
+    open_files_.erase(file_path);
+
+  if (!callback.is_null())
+    callback.Run(result, cache_file_path);
+}
+
 void GDataFileSystem::CloseFile(const FilePath& file_path,
                                 const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
@@ -3650,6 +3682,14 @@ void GDataFileSystem::CloseFileOnUIThread(
     const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (open_files_.find(file_path) == open_files_.end()) {
+    // The file is not being opened.
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, base::PLATFORM_FILE_ERROR_NOT_FOUND));
+    return;
+  }
+
   // CloseFile should only be applied for a previously OpenFile'd file, so the
   // file should already be cached. The sole purpose of the following call to
   // GetFileByPathOnUIThread is to get the path to the local cache file.
@@ -3658,7 +3698,10 @@ void GDataFileSystem::CloseFileOnUIThread(
       base::Bind(&GDataFileSystem::OnGetFileCompleteForCloseFile,
                  ui_weak_ptr_,
                  file_path,
-                 callback),
+                 base::Bind(&GDataFileSystem::OnCloseFileFinished,
+                            ui_weak_ptr_,
+                            file_path,
+                            callback)),
       GetDownloadDataCallback());
 }
 
@@ -3769,4 +3812,20 @@ void GDataFileSystem::OnCommitDirtyInCacheCompleteForCloseFile(
     callback.Run(error);
 }
 
+void GDataFileSystem::OnCloseFileFinished(
+    const FilePath& file_path,
+    const FileOperationCallback& callback,
+    base::PlatformFileError result) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // All the invocation of |callback| from operations initiated from CloseFile
+  // must go through here. Removes the |file_path| from the remembered set so
+  // that subsequent operations can open the file again.
+  open_files_.erase(file_path);
+
+  // Then invokes the user-supplied callback function.
+  if (!callback.is_null())
+    callback.Run(result);
+}
+
 }  // namespace gdata
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.h b/chrome/browser/chromeos/gdata/gdata_file_system.h
index 35de97f..645fc16 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.h
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.h
@@ -620,6 +620,14 @@ class GDataFileSystem : public GDataFileSystemInterface,
                                         base::PlatformFileError error,
                                         scoped_ptr<GDataFileProto> file_info);
 
+  // Invoked at the last step of OpenFile. It removes |file_path| from the
+  // current set of opened files if |result| is an error, and then invokes the
+  // |callback| function.
+  void OnOpenFileFinished(const FilePath& file_path,
+                          const OpenFileCallback& callback,
+                          base::PlatformFileError result,
+                          const FilePath& cache_file_path);
+
   // Invoked during the process of CloseFile. It first gets the path of local
   // cache and receives it with OnGetFileCompleteForCloseFile. Then it reads the
   // metadata of the modified cache and send the information to
@@ -627,7 +635,9 @@ class GDataFileSystem : public GDataFileSystemInterface,
   // update the GData entry by FindEntryByPathAsyncOnUIThread
   // and invokes OnGetFileInfoCompleteForCloseFile. It then continues to
   // invoke CommitDirtyInCache to commit the change, and finally proceeds to
-  // OnCommitDirtyInCacheCompleteForCloseFile and calls user-supplied callback.
+  // OnCommitDirtyInCacheCompleteForCloseFile and calls OnCloseFileFinished,
+  // which removes the file from the "opened" list and invokes user-supplied
+  // callback.
   void OnGetFileCompleteForCloseFile(
       const FilePath& file_path,
       const FileOperationCallback& callback,
@@ -651,6 +661,9 @@ class GDataFileSystem : public GDataFileSystemInterface,
       base::PlatformFileError error,
       const std::string& resource_id,
       const std::string& md5);
+  void OnCloseFileFinished(const FilePath& file_path,
+                           const FileOperationCallback& callback,
+                           base::PlatformFileError result);
 
   // Invoked upon completion of GetFileByPath initiated by Copy. If
   // GetFileByPath reports no error, calls TransferRegularFile to transfer
@@ -1135,6 +1148,9 @@ class GDataFileSystem : public GDataFileSystemInterface,
   // True if hosted documents should be hidden.
   bool hide_hosted_docs_;
 
+  // The set of paths opened by OpenFile but not yet closed by CloseFile.
+  std::set<FilePath> open_files_;
+
   scoped_ptr<PrefChangeRegistrar> pref_registrar_;
 
   // WeakPtrFactory and WeakPtr bound to the UI thread.
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc b/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc
index db214ba..a0f8338 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc
@@ -3882,21 +3882,28 @@ TEST_F(GDataFileSystemTest, OpenAndCloseFile) {
   // Open kFileInRoot ("drive/File 1.txt").
   file_system_->OpenFile(kFileInRoot, callback);
   message_loop_.Run();
+  const FilePath opened_file_path = callback_helper_->opened_file_path_;
 
   // Verify that the file was properly opened.
   EXPECT_EQ(base::PLATFORM_FILE_OK, callback_helper_->last_error_);
 
+  // Try to open the already opened file.
+  file_system_->OpenFile(kFileInRoot, callback);
+  message_loop_.Run();
+
+  // It must fail.
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_IN_USE, callback_helper_->last_error_);
+
   // Verify that the file contents match the expected contents.
   std::string cache_file_data;
-  EXPECT_TRUE(file_util::ReadFileToString(callback_helper_->opened_file_path_,
-                                          &cache_file_data));
+  EXPECT_TRUE(file_util::ReadFileToString(opened_file_path, &cache_file_data));
   EXPECT_EQ(kExpectedFileData, cache_file_data);
 
   // Verify that the cache state was changed as expected.
-  VerifyCacheStateAfterOpenFile(callback_helper_->last_error_,
+  VerifyCacheStateAfterOpenFile(base::PLATFORM_FILE_OK,
                                 file_resource_id,
                                 file_md5,
-                                callback_helper_->opened_file_path_);
+                                opened_file_path);
 
   // Close kFileInRoot ("drive/File 1.txt").
   file_system_->CloseFile(kFileInRoot, close_file_callback);
@@ -3906,9 +3913,16 @@ TEST_F(GDataFileSystemTest, OpenAndCloseFile) {
   EXPECT_EQ(base::PLATFORM_FILE_OK, callback_helper_->last_error_);
 
   // Verify that the cache state was changed as expected.
-  VerifyCacheStateAfterCloseFile(callback_helper_->last_error_,
+  VerifyCacheStateAfterCloseFile(base::PLATFORM_FILE_OK,
                                  file_resource_id,
                                  file_md5);
+
+  // Try to close the same file twice.
+  file_system_->CloseFile(kFileInRoot, close_file_callback);
+  message_loop_.Run();
+
+  // It must fail.
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, callback_helper_->last_error_);
 }
 
 }   // namespace gdata