Make FileSystemOperation's lifetime more explicit.

In the current code calling dispatcher->DidXxx in an operation's DidXxx method MAY indirectly delete the operation itself depending on the dispatcher's implementation.  I was confused by this several times and I want to make this flow more explicit.

This patch lets FileSystemOperation control its lifetime by itself so that each callback dispatcher implementation does not need to take care of it.

Also moved BrowserFileSystemCallbackDispatcher into file_system_dispatcher_host.cc as it's only used in it and its implementation is tightly coupled with the DispatcherHost.

BUG=60243
TEST=none

Review URL: http://codereview.chromium.org/4821005

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

chrome/browser/file_system/browser_file_system_callback_dispatcher.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/file_system/browser_file_system_callback_dispatcher.h"
-
-#include "chrome/browser/file_system/file_system_dispatcher_host.h"
-#include "chrome/common/render_messages.h"
-
-BrowserFileSystemCallbackDispatcher::BrowserFileSystemCallbackDispatcher(
-    FileSystemDispatcherHost* dispatcher_host, int request_id)
-    : dispatcher_host_(dispatcher_host),
-      request_id_(request_id) {
-  DCHECK(dispatcher_host_);
-}
-
-BrowserFileSystemCallbackDispatcher::~BrowserFileSystemCallbackDispatcher() {}
-
-void BrowserFileSystemCallbackDispatcher::DidSucceed() {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidSucceed(request_id_));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidReadMetadata(
-    const base::PlatformFileInfo& info) {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidReadMetadata(
-      request_id_, info));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidReadDirectory(
-    const std::vector<base::FileUtilProxy::Entry>& entries, bool has_more) {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidReadDirectory(
-      request_id_, entries, has_more));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidOpenFileSystem(
-    const std::string& name, const FilePath& path) {
-  dispatcher_host_->Send(
-      new ViewMsg_OpenFileSystemRequest_Complete(
-          request_id_, !path.empty(), name, path));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidFail(
-    base::PlatformFileError error_code) {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidFail(
-      request_id_, error_code));
-  dispatcher_host_->RemoveCompletedOperation(request_id_);
-}
-
-void BrowserFileSystemCallbackDispatcher::DidWrite(
-    int64 bytes,
-    bool complete) {
-  dispatcher_host_->Send(new ViewMsg_FileSystem_DidWrite(
-      request_id_, bytes, complete));
-  if (complete)
-    dispatcher_host_->RemoveCompletedOperation(request_id_);
-}

chrome/browser/file_system/browser_file_system_callback_dispatcher.h

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_FILE_SYSTEM_BROWSER_FILE_SYSTEM_CALLBACK_DISPATCHER_H_
-#define CHROME_BROWSER_FILE_SYSTEM_BROWSER_FILE_SYSTEM_CALLBACK_DISPATCHER_H_
-
-#include "webkit/fileapi/file_system_callback_dispatcher.h"
-
-class FileSystemDispatcherHost;
-
-class BrowserFileSystemCallbackDispatcher
-    : public fileapi::FileSystemCallbackDispatcher {
- public:
-  BrowserFileSystemCallbackDispatcher(FileSystemDispatcherHost* dispatcher_host,
-                                      int request_id);
-  virtual ~BrowserFileSystemCallbackDispatcher();
-
-  // FileSystemCallbackDispatcher implementation.
-  virtual void DidSucceed();
-  virtual void DidReadMetadata(const base::PlatformFileInfo& file_info);
-  virtual void DidReadDirectory(
-      const std::vector<base::FileUtilProxy::Entry>& entries,
-      bool has_more);
-  virtual void DidOpenFileSystem(const std::string& name,
-                                 const FilePath& root_path);
-  virtual void DidFail(base::PlatformFileError error_code);
-  virtual void DidWrite(int64 bytes, bool complete);
-
- private:
-  scoped_refptr<FileSystemDispatcherHost> dispatcher_host_;
-  int request_id_;
-};
-
-#endif  // CHROME_BROWSER_FILE_SYSTEM_BROWSER_FILE_SYSTEM_CALLBACK_DISPATCHER_H_

chrome/browser/file_system/file_system_dispatcher_host.cc

@@ -10,7 +10,6 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
-#include "chrome/browser/file_system/browser_file_system_callback_dispatcher.h"
 #include "chrome/browser/file_system/browser_file_system_context.h"
 #include "chrome/browser/net/chrome_url_request_context.h"
 #include "chrome/browser/profile.h"
@@ -20,14 +19,67 @@
 #include "chrome/common/render_messages_params.h"
 #include "googleurl/src/gurl.h"
 #include "net/url_request/url_request_context.h"
+#include "webkit/fileapi/file_system_callback_dispatcher.h"
 #include "webkit/fileapi/file_system_operation.h"
 #include "webkit/fileapi/file_system_path_manager.h"
 #include "webkit/fileapi/file_system_quota_manager.h"
 #include "webkit/fileapi/sandboxed_file_system_operation.h"
 
+using fileapi::FileSystemCallbackDispatcher;
 using fileapi::FileSystemQuotaManager;
 using fileapi::SandboxedFileSystemOperation;
 
+class BrowserFileSystemCallbackDispatcher
+    : public FileSystemCallbackDispatcher {
+ public:
+  BrowserFileSystemCallbackDispatcher(
+      FileSystemDispatcherHost* dispatcher_host, int request_id)
+      : dispatcher_host_(dispatcher_host),
+        request_id_(request_id) {
+    DCHECK(dispatcher_host_);
+  }
+
+  virtual ~BrowserFileSystemCallbackDispatcher() {
+    dispatcher_host_->UnregisterOperation(request_id_);
+  }
+
+  virtual void DidSucceed() {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidSucceed(request_id_));
+  }
+
+  virtual void DidReadMetadata(const base::PlatformFileInfo& info) {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidReadMetadata(
+        request_id_, info));
+  }
+
+  virtual void DidReadDirectory(
+      const std::vector<base::FileUtilProxy::Entry>& entries, bool has_more) {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidReadDirectory(
+        request_id_, entries, has_more));
+  }
+
+  virtual void DidOpenFileSystem(const std::string& name,
+                                 const FilePath& path) {
+    dispatcher_host_->Send(
+        new ViewMsg_OpenFileSystemRequest_Complete(
+            request_id_, !path.empty(), name, path));
+  }
+
+  virtual void DidFail(base::PlatformFileError error_code) {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidFail(
+        request_id_, error_code));
+  }
+
+  virtual void DidWrite(int64 bytes, bool complete) {
+    dispatcher_host_->Send(new ViewMsg_FileSystem_DidWrite(
+        request_id_, bytes, complete));
+  }
+
+ private:
+  scoped_refptr<FileSystemDispatcherHost> dispatcher_host_;
+  int request_id_;
+};
+
 FileSystemDispatcherHost::FileSystemDispatcherHost(
     IPC::Message::Sender* sender, Profile* profile)
     : message_sender_(sender),
@@ -217,7 +269,7 @@ SandboxedFileSystemOperation* FileSystemDispatcherHost::GetNewOperation(
   return operation;
 }
 
-void FileSystemDispatcherHost::RemoveCompletedOperation(int request_id) {
+void FileSystemDispatcherHost::UnregisterOperation(int request_id) {
   DCHECK(operations_.Lookup(request_id));
   operations_.Remove(request_id);
 }

chrome/browser/file_system/file_system_dispatcher_host.h

@@ -79,7 +79,7 @@ class FileSystemDispatcherHost
                    const base::Time& last_modified_time);
   void OnCancel(int request_id, int request_to_cancel);
   void Send(IPC::Message* message);
-  void RemoveCompletedOperation(int request_id);
+  void UnregisterOperation(int request_id);
 
  private:
   // Creates a new SandboxedFileSystemOperation.
@@ -99,8 +99,7 @@ class FileSystemDispatcherHost
   scoped_refptr<HostContentSettingsMap> host_content_settings_map_;
 
   // Keeps ongoing file system operations.
-  typedef IDMap<fileapi::SandboxedFileSystemOperation, IDMapOwnPointer>
-      OperationsMap;
+  typedef IDMap<fileapi::SandboxedFileSystemOperation> OperationsMap;
   OperationsMap operations_;
 
   // This holds the URLRequestContextGetter until Init() can be called from the

chrome/chrome_browser.gypi

@@ -1656,8 +1656,6 @@
         'browser/file_path_watcher_win.cc',
         'browser/file_select_helper.cc',
         'browser/file_select_helper.h',
-        'browser/file_system/browser_file_system_callback_dispatcher.cc',
-        'browser/file_system/browser_file_system_callback_dispatcher.h',
         'browser/file_system/browser_file_system_context.cc',
         'browser/file_system/browser_file_system_context.h',
         'browser/file_system/file_system_dispatcher_host.cc',

webkit/fileapi/file_system_operation.cc

@@ -16,8 +16,7 @@ FileSystemOperation::FileSystemOperation(
     scoped_refptr<base::MessageLoopProxy> proxy)
     : proxy_(proxy),
       dispatcher_(dispatcher),
-      callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
-      cancel_operation_(NULL) {
+      callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   DCHECK(dispatcher);
 #ifndef NDEBUG
   pending_operation_ = kOperationNone;
@@ -159,6 +158,7 @@ void FileSystemOperation::OnFileOpenedForWrite(
     bool created) {
   if (base::PLATFORM_FILE_OK != rv) {
     dispatcher_->DidFail(rv);
+    delete this;
     return;
   }
   file_writer_delegate_->Start(file.ReleaseValue(), blob_request_.get());
@@ -189,7 +189,8 @@ void FileSystemOperation::TouchFile(const FilePath& path,
 
 // We can only get here on a write or truncate that's not yet completed.
 // We don't support cancelling any other operation at this time.
-void FileSystemOperation::Cancel(FileSystemOperation* cancel_operation) {
+void FileSystemOperation::Cancel(FileSystemOperation* cancel_operation_ptr) {
+  scoped_ptr<FileSystemOperation> cancel_operation(cancel_operation_ptr);
   if (file_writer_delegate_.get()) {
 #ifndef NDEBUG
     DCHECK(kOperationWrite == pending_operation_);
@@ -203,9 +204,9 @@ void FileSystemOperation::Cancel(FileSystemOperation* cancel_operation) {
       // This halts any calls to file_writer_delegate_ from blob_request_.
       blob_request_->Cancel();
 
-    // This deletes us, and by proxy deletes file_writer_delegate_ if any.
     dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
-    cancel_operation->dispatcher_->DidSucceed();
+    cancel_operation->dispatcher()->DidSucceed();
+    delete this;
   } else {
 #ifndef NDEBUG
     DCHECK(kOperationTruncate == pending_operation_);
@@ -214,15 +215,17 @@ void FileSystemOperation::Cancel(FileSystemOperation* cancel_operation) {
     // since it's been proxied to another thread.  We need to save the
     // cancel_operation so that when the truncate returns, it can see that it's
     // been cancelled, report it, and report that the cancel has succeeded.
-    cancel_operation_ = cancel_operation;
+    DCHECK(!cancel_operation_.get());
+    cancel_operation_.swap(cancel_operation);
   }
 }
 
 void FileSystemOperation::DidEnsureFileExistsExclusive(
     base::PlatformFileError rv, bool created) {
-  if (rv == base::PLATFORM_FILE_OK && !created)
+  if (rv == base::PLATFORM_FILE_OK && !created) {
     dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_EXISTS);
-  else
+    delete this;
+  } else
     DidFinishFileOperation(rv);
 }
 
@@ -233,19 +236,19 @@ void FileSystemOperation::DidEnsureFileExistsNonExclusive(
 
 void FileSystemOperation::DidFinishFileOperation(
     base::PlatformFileError rv) {
-  if (cancel_operation_) {
+  if (cancel_operation_.get()) {
 #ifndef NDEBUG
     DCHECK(kOperationTruncate == pending_operation_);
 #endif
-    FileSystemOperation *cancel_op = cancel_operation_;
-    // This call deletes us, so we have to extract cancel_op first.
+
     dispatcher_->DidFail(base::PLATFORM_FILE_ERROR_ABORT);
-    cancel_op->dispatcher_->DidSucceed();
+    cancel_operation_->dispatcher()->DidSucceed();
   } else if (rv == base::PLATFORM_FILE_OK) {
     dispatcher_->DidSucceed();
   } else {
     dispatcher_->DidFail(rv);
   }
+  delete this;
 }
 
 void FileSystemOperation::DidDirectoryExists(
@@ -259,6 +262,7 @@ void FileSystemOperation::DidDirectoryExists(
   } else {
     dispatcher_->DidFail(rv);
   }
+  delete this;
 }
 
 void FileSystemOperation::DidFileExists(
@@ -272,6 +276,7 @@ void FileSystemOperation::DidFileExists(
   } else {
     dispatcher_->DidFail(rv);
   }
+  delete this;
 }
 
 void FileSystemOperation::DidGetMetadata(
@@ -281,6 +286,7 @@ void FileSystemOperation::DidGetMetadata(
     dispatcher_->DidReadMetadata(file_info);
   else
     dispatcher_->DidFail(rv);
+  delete this;
 }
 
 void FileSystemOperation::DidReadDirectory(
@@ -290,6 +296,7 @@ void FileSystemOperation::DidReadDirectory(
     dispatcher_->DidReadDirectory(entries, false /* has_more */);
   else
     dispatcher_->DidFail(rv);
+  delete this;
 }
 
 void FileSystemOperation::DidWrite(
@@ -300,6 +307,8 @@ void FileSystemOperation::DidWrite(
     dispatcher_->DidWrite(bytes, complete);
   else
     dispatcher_->DidFail(rv);
+  if (complete || rv != base::PLATFORM_FILE_OK)
+    delete this;
 }
 
 void FileSystemOperation::DidTouchFile(base::PlatformFileError rv) {
@@ -307,6 +316,7 @@ void FileSystemOperation::DidTouchFile(base::PlatformFileError rv) {
     dispatcher_->DidSucceed();
   else
     dispatcher_->DidFail(rv);
+  delete this;
 }
 
 }  // namespace fileapi

webkit/fileapi/file_system_operation.h

@@ -36,8 +36,11 @@ class FileWriterDelegate;
 // Only one method(CreateFile, CreateDirectory, Copy, Move, DirectoryExists,
 // GetMetadata, ReadDirectory and Remove) may be called during the lifetime of
 // this object and it should be called no more than once.
+// This class is self-destructed and an instance automatically gets deleted
+// when its operation is finished.
 class FileSystemOperation {
  public:
+  // |dispatcher| will be owned by this class.
   FileSystemOperation(FileSystemCallbackDispatcher* dispatcher,
                       scoped_refptr<base::MessageLoopProxy> proxy);
   virtual ~FileSystemOperation();
@@ -65,15 +68,16 @@ class FileSystemOperation {
 
   virtual void Remove(const FilePath& path, bool recursive);
 
-  virtual void Write(
-      scoped_refptr<URLRequestContext> url_request_context,
-      const FilePath& path, const GURL& blob_url, int64 offset);
+  virtual void Write(scoped_refptr<URLRequestContext> url_request_context,
+                     const FilePath& path,
+                     const GURL& blob_url,
+                     int64 offset);
 
   virtual void Truncate(const FilePath& path, int64 length);
 
   virtual void TouchFile(const FilePath& path,
-                 const base::Time& last_access_time,
-                 const base::Time& last_modified_time);
+                         const base::Time& last_access_time,
+                         const base::Time& last_modified_time);
 
   // Try to cancel the current operation [we support cancelling write or
   // truncate only].  Report failure for the current operation, then tell the
@@ -154,7 +158,7 @@ class FileSystemOperation {
   friend class FileWriterDelegate;
   scoped_ptr<FileWriterDelegate> file_writer_delegate_;
   scoped_ptr<net::URLRequest> blob_request_;
-  FileSystemOperation* cancel_operation_;
+  scoped_ptr<FileSystemOperation> cancel_operation_;
 
   DISALLOW_COPY_AND_ASSIGN(FileSystemOperation);
 };

webkit/fileapi/sandboxed_file_system_operation.cc

@@ -38,15 +38,19 @@ void SandboxedFileSystemOperation::OpenFileSystem(
 
 void SandboxedFileSystemOperation::CreateFile(
     const FilePath& path, bool exclusive) {
-  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::CreateFile(path, exclusive);
 }
 
 void SandboxedFileSystemOperation::CreateDirectory(
     const FilePath& path, bool exclusive, bool recursive) {
-  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::CreateDirectory(path, exclusive, recursive);
 }
 
@@ -54,8 +58,10 @@ void SandboxedFileSystemOperation::Copy(
     const FilePath& src_path, const FilePath& dest_path) {
   if (!VerifyFileSystemPathForRead(src_path) ||
       !VerifyFileSystemPathForWrite(dest_path, true /* create */,
-                                    FileSystemQuotaManager::kUnknownSize))
+                                    FileSystemQuotaManager::kUnknownSize)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Copy(src_path, dest_path);
 }
 
@@ -63,39 +69,51 @@ void SandboxedFileSystemOperation::Move(
     const FilePath& src_path, const FilePath& dest_path) {
   if (!VerifyFileSystemPathForRead(src_path) ||
       !VerifyFileSystemPathForWrite(dest_path, true /* create */,
-                                    FileSystemQuotaManager::kUnknownSize))
+                                    FileSystemQuotaManager::kUnknownSize)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Move(src_path, dest_path);
 }
 
 void SandboxedFileSystemOperation::DirectoryExists(const FilePath& path) {
-  if (!VerifyFileSystemPathForRead(path))
+  if (!VerifyFileSystemPathForRead(path)) {
+    delete this;
     return;
+  }
   FileSystemOperation::DirectoryExists(path);
 }
 
 void SandboxedFileSystemOperation::FileExists(const FilePath& path) {
-  if (!VerifyFileSystemPathForRead(path))
+  if (!VerifyFileSystemPathForRead(path)) {
+    delete this;
     return;
+  }
   FileSystemOperation::FileExists(path);
 }
 
 void SandboxedFileSystemOperation::GetMetadata(const FilePath& path) {
-  if (!VerifyFileSystemPathForRead(path))
+  if (!VerifyFileSystemPathForRead(path)) {
+    delete this;
     return;
+  }
   FileSystemOperation::GetMetadata(path);
 }
 
 void SandboxedFileSystemOperation::ReadDirectory(const FilePath& path) {
-  if (!VerifyFileSystemPathForRead(path))
+  if (!VerifyFileSystemPathForRead(path)) {
+    delete this;
     return;
+  }
   FileSystemOperation::ReadDirectory(path);
 }
 
 void SandboxedFileSystemOperation::Remove(
     const FilePath& path, bool recursive) {
-  if (!VerifyFileSystemPathForWrite(path, false /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, false /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Remove(path, recursive);
 }
 
@@ -103,15 +121,19 @@ void SandboxedFileSystemOperation::Write(
     scoped_refptr<URLRequestContext> url_request_context,
     const FilePath& path, const GURL& blob_url, int64 offset) {
   if (!VerifyFileSystemPathForWrite(path, true /* create */,
-                                    FileSystemQuotaManager::kUnknownSize))
+                                    FileSystemQuotaManager::kUnknownSize)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Write(url_request_context, path, blob_url, offset);
 }
 
 void SandboxedFileSystemOperation::Truncate(
     const FilePath& path, int64 length) {
-  if (!VerifyFileSystemPathForWrite(path, false /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, false /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::Truncate(path, length);
 }
 
@@ -119,8 +141,10 @@ void SandboxedFileSystemOperation::TouchFile(
     const FilePath& path,
     const base::Time& last_access_time,
     const base::Time& last_modified_time) {
-  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0))
+  if (!VerifyFileSystemPathForWrite(path, true /* create */, 0)) {
+    delete this;
     return;
+  }
   FileSystemOperation::TouchFile(path, last_access_time, last_modified_time);
 }
 
@@ -128,6 +152,7 @@ void SandboxedFileSystemOperation::DidGetRootPath(
     bool success, const FilePath& path, const std::string& name) {
   DCHECK(success || path.empty());
   dispatcher()->DidOpenFileSystem(name, path);
+  delete this;
 }
 
 bool SandboxedFileSystemOperation::VerifyFileSystemPathForRead(

webkit/fileapi/sandboxed_file_system_operation.h

@@ -71,6 +71,8 @@ class SandboxedFileSystemOperation : public FileSystemOperation {
   // Returns true if the given |path| is a valid FileSystem path.
   // Otherwise it calls dispatcher's DidFail method with
   // PLATFORM_FILE_ERROR_SECURITY and returns false.
+  // (Note: this doesn't delete this when it calls DidFail and returns false;
+  // it's the caller's responsibility.)
   bool VerifyFileSystemPathForRead(const FilePath& path);
 
   // Checks the validity of a given |path| for writing.
@@ -85,6 +87,8 @@ class SandboxedFileSystemOperation : public FileSystemOperation {
   // If |create| flag is true this also checks if the |path| contains
   // any restricted names and chars. If it does, the call fires dispatcher's
   // DidFail with PLATFORM_FILE_ERROR_SECURITY and returns false.
+  // (Note: this doesn't delete this when it calls DidFail and returns false;
+  // it's the caller's responsibility.)
   bool VerifyFileSystemPathForWrite(const FilePath& path,
                                     bool create,
                                     int64 growth);

webkit/tools/test_shell/simple_file_system.cc

@@ -54,17 +54,11 @@ class SimpleFileSystemCallbackDispatcher
   }
 
   ~SimpleFileSystemCallbackDispatcher() {
-    DCHECK(!operation_.get());
-  }
-
-  void set_operation(SandboxedFileSystemOperation* operation) {
-    operation_.reset(operation);
   }
 
   virtual void DidSucceed() {
-    if (file_system_)
-      callbacks_->didSucceed();
-    RemoveOperation();
+    DCHECK(file_system_);
+    callbacks_->didSucceed();
   }
 
   virtual void DidReadMetadata(const base::PlatformFileInfo& info) {
@@ -75,7 +69,6 @@ class SimpleFileSystemCallbackDispatcher
     web_file_info.type = info.is_directory ?
         WebFileInfo::TypeDirectory : WebFileInfo::TypeFile;
     callbacks_->didReadMetadata(web_file_info);
-    RemoveOperation();
   }
 
   virtual void DidReadDirectory(
@@ -93,7 +86,6 @@ class SimpleFileSystemCallbackDispatcher
     WebVector<WebKit::WebFileSystemEntry> web_entries =
         web_entries_vector;
     callbacks_->didReadDirectory(web_entries, has_more);
-    RemoveOperation();
   }
 
   virtual void DidOpenFileSystem(
@@ -104,14 +96,12 @@ class SimpleFileSystemCallbackDispatcher
     else
       callbacks_->didOpenFileSystem(
           UTF8ToUTF16(name), webkit_glue::FilePathToWebString(path));
-    RemoveOperation();
   }
 
   virtual void DidFail(base::PlatformFileError error_code) {
     DCHECK(file_system_);
     callbacks_->didFail(
         webkit_glue::PlatformFileErrorToWebFileError(error_code));
-    RemoveOperation();
   }
 
   virtual void DidWrite(int64, bool) {
@@ -119,17 +109,8 @@ class SimpleFileSystemCallbackDispatcher
   }
 
  private:
-  void RemoveOperation() {
-    // We need to make sure operation_ is null when we delete the operation
-    // (which in turn deletes this dispatcher instance).
-    scoped_ptr<SandboxedFileSystemOperation> operation;
-    operation.swap(operation_);
-    operation.reset();
-  }
-
   WeakPtr<SimpleFileSystem> file_system_;
   WebFileSystemCallbacks* callbacks_;
-  scoped_ptr<SandboxedFileSystemOperation> operation_;
 };
 
 }  // namespace
@@ -262,6 +243,5 @@ SandboxedFileSystemOperation* SimpleFileSystem::GetNewOperation(
   SandboxedFileSystemOperation* operation = new SandboxedFileSystemOperation(
       dispatcher, base::MessageLoopProxy::CreateForCurrentThread(),
       sandboxed_context_.get());
-  dispatcher->set_operation(operation);
   return operation;
 }

webkit/tools/test_shell/simple_file_writer.cc

@@ -28,7 +28,8 @@ class SimpleFileWriter::IOThreadProxy
     : public base::RefCountedThreadSafe<SimpleFileWriter::IOThreadProxy> {
  public:
   explicit IOThreadProxy(const base::WeakPtr<SimpleFileWriter>& simple_writer)
-      : simple_writer_(simple_writer) {
+      : simple_writer_(simple_writer),
+        operation_(NULL) {
     // The IO thread needs to be running for this class to work.
     SimpleResourceLoaderBridge::EnsureIOThread();
     io_thread_ = SimpleResourceLoaderBridge::GetIoThread();
@@ -44,8 +45,8 @@ class SimpleFileWriter::IOThreadProxy
           this, &IOThreadProxy::Truncate, path, offset));
       return;
     }
-    DCHECK(!operation_.get());
-    operation_.reset(GetNewOperation());
+    DCHECK(!operation_);
+    operation_ = GetNewOperation();
     operation_->Truncate(path, offset);
   }
 
@@ -56,8 +57,8 @@ class SimpleFileWriter::IOThreadProxy
       return;
     }
     DCHECK(request_context_);
-    DCHECK(!operation_.get());
-    operation_.reset(GetNewOperation());
+    DCHECK(!operation_);
+    operation_ = GetNewOperation();
     operation_->Write(request_context_, path, blob_url, offset);
   }
 
@@ -67,12 +68,11 @@ class SimpleFileWriter::IOThreadProxy
           this, &IOThreadProxy::Cancel));
       return;
     }
-    if (!operation_.get()) {
+    if (!operation_) {
       DidFail(base::PLATFORM_FILE_ERROR_INVALID_OPERATION);
       return;
     }
-    cancel_operation_.reset(GetNewOperation());
-    operation_->Cancel(cancel_operation_.get());
+    operation_->Cancel(GetNewOperation());
   }
 
  private:
@@ -82,6 +82,10 @@ class SimpleFileWriter::IOThreadProxy
     explicit CallbackDispatcher(IOThreadProxy* proxy) : proxy_(proxy) {
     }
 
+    ~CallbackDispatcher() {
+      proxy_->ClearOperation();
+    }
+
     virtual void DidSucceed() {
       proxy_->DidSucceed();
     }
@@ -119,7 +123,6 @@ class SimpleFileWriter::IOThreadProxy
 
   void DidSucceed() {
     if (!main_thread_->BelongsToCurrentThread()) {
-      operation_.reset();
       main_thread_->PostTask(FROM_HERE, NewRunnableMethod(
           this, &IOThreadProxy::DidSucceed));
       return;
@@ -130,7 +133,6 @@ class SimpleFileWriter::IOThreadProxy
 
   void DidFail(base::PlatformFileError error_code) {
     if (!main_thread_->BelongsToCurrentThread()) {
-      operation_.reset();
       main_thread_->PostTask(FROM_HERE, NewRunnableMethod(
           this, &IOThreadProxy::DidFail, error_code));
       return;
@@ -141,8 +143,6 @@ class SimpleFileWriter::IOThreadProxy
 
   void DidWrite(int64 bytes, bool complete) {
     if (!main_thread_->BelongsToCurrentThread()) {
-      if (complete)
-        operation_.reset();
       main_thread_->PostTask(FROM_HERE, NewRunnableMethod(
           this, &IOThreadProxy::DidWrite, bytes, complete));
       return;
@@ -151,6 +151,11 @@ class SimpleFileWriter::IOThreadProxy
       simple_writer_->DidWrite(bytes, complete);
   }
 
+  void ClearOperation() {
+    DCHECK(io_thread_->BelongsToCurrentThread());
+    operation_ = NULL;
+  }
+
   scoped_refptr<base::MessageLoopProxy> io_thread_;
   scoped_refptr<base::MessageLoopProxy> main_thread_;
 
@@ -158,8 +163,7 @@ class SimpleFileWriter::IOThreadProxy
   base::WeakPtr<SimpleFileWriter> simple_writer_;
 
   // Only used on the io thread.
-  scoped_ptr<FileSystemOperation> operation_;
-  scoped_ptr<FileSystemOperation> cancel_operation_;
+  FileSystemOperation* operation_;
 };