Revert 134248 - Make LocalFileReader deletable at any time and add unittests.

Changed the internal Open() logic not to deal with raw PlatformFile handle but instead to rely on net::FileStream's destructor to close the file.

Also adding more specific comment to FileReader interface.

BUG=123570
TEST=unit_tests:LocalFileReader.*

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

TBR=kinuko@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10209025

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

4 files changed, 137 insertions, 364 deletions

diff --git a/webkit/blob/file_reader.h b/webkit/blob/file_reader.h
index 8aedf40..78dfbf9 100644
--- a/webkit/blob/file_reader.h
+++ b/webkit/blob/file_reader.h
@@ -21,8 +21,6 @@ namespace webkit_blob {
 // A generic interface for reading a file-like object.
 class BLOB_EXPORT FileReader {
  public:
-  // It is valid to delete the reader at any time.  If the stream is deleted
-  // while it has a pending read, its callback will not be called.
   virtual ~FileReader() {}
 
   // Reads from the current cursor position asynchronously.
@@ -35,9 +33,6 @@ class BLOB_EXPORT FileReader {
   // was called, when the read has completed.
   //
   // It is invalid to call Read while there is an in-flight Read operation.
-  //
-  // If the stream is deleted while it has an in-flight Read operation
-  // |callback| will not be called.
   virtual int Read(net::IOBuffer* buf, int buf_len,
                    const net::CompletionCallback& callback) = 0;
 };
diff --git a/webkit/blob/local_file_reader.cc b/webkit/blob/local_file_reader.cc
index 8b5fb05..068ad13 100644
--- a/webkit/blob/local_file_reader.cc
+++ b/webkit/blob/local_file_reader.cc
@@ -30,6 +30,39 @@ bool VerifySnapshotTime(const base::Time& expected_modification_time,
              file_info.last_modified.ToTimeT();
 }
 
+void DidGetFileInfoForGetLength(const net::Int64CompletionCallback& callback,
+                                const base::Time& expected_modification_time,
+                                base::PlatformFileError error,
+                                const base::PlatformFileInfo& file_info) {
+  if (file_info.is_directory) {
+    callback.Run(net::ERR_FILE_NOT_FOUND);
+    return;
+  }
+  if (error != base::PLATFORM_FILE_OK) {
+    callback.Run(LocalFileReader::PlatformFileErrorToNetError(error));
+    return;
+  }
+  if (!VerifySnapshotTime(expected_modification_time, file_info)) {
+    callback.Run(net::ERR_UPLOAD_FILE_CHANGED);
+    return;
+  }
+  callback.Run(file_info.size);
+}
+
+void DidSeekFile(const LocalFileReader::OpenFileStreamCallback& callback,
+                 scoped_ptr<net::FileStream> stream_impl,
+                 int64 initial_offset,
+                 int64 new_offset) {
+  int result = net::OK;
+  if (new_offset < 0)
+    result = static_cast<int>(new_offset);
+  else if (new_offset != initial_offset)
+    result = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
+  callback.Run(result, stream_impl.Pass());
+}
+
+void EmptyCompletionCallback(int) {}
+
 }  // namespace
 
 // static
@@ -47,6 +80,70 @@ int LocalFileReader::PlatformFileErrorToNetError(
   }
 }
 
+// A helper class to open, verify and seek a file stream for a given path.
+class LocalFileReader::OpenFileStreamHelper {
+ public:
+  explicit OpenFileStreamHelper(base::MessageLoopProxy* file_thread_proxy)
+      : file_thread_proxy_(file_thread_proxy),
+        file_handle_(base::kInvalidPlatformFileValue),
+        result_(net::OK) {}
+  ~OpenFileStreamHelper() {
+    if (file_handle_ != base::kInvalidPlatformFileValue) {
+      file_thread_proxy_->PostTask(
+          FROM_HERE,
+          base::Bind(base::IgnoreResult(&base::ClosePlatformFile),
+                     file_handle_));
+    }
+  }
+
+  void OpenAndVerifyOnFileThread(const FilePath& file_path,
+                                 const base::Time& expected_modification_time) {
+    base::PlatformFileError file_error = base::PLATFORM_FILE_OK;
+    file_handle_ = base::CreatePlatformFile(
+        file_path, kOpenFlagsForRead, NULL, &file_error);
+    if (file_error != base::PLATFORM_FILE_OK) {
+      result_ = PlatformFileErrorToNetError(file_error);
+      return;
+    }
+    DCHECK_NE(base::kInvalidPlatformFileValue, file_handle_);
+    base::PlatformFileInfo file_info;
+    if (!base::GetPlatformFileInfo(file_handle_, &file_info)) {
+      result_ = net::ERR_FAILED;
+      return;
+    }
+    if (!VerifySnapshotTime(expected_modification_time, file_info)) {
+      result_ = net::ERR_UPLOAD_FILE_CHANGED;
+      return;
+    }
+    result_ = net::OK;
+  }
+
+  void OpenStreamOnCallingThread(int64 initial_offset,
+                                 const OpenFileStreamCallback& callback) {
+    DCHECK(!callback.is_null());
+    scoped_ptr<net::FileStream> stream_impl;
+    if (result_ != net::OK) {
+      callback.Run(result_, stream_impl.Pass());
+      return;
+    }
+    stream_impl.reset(
+        new net::FileStream(file_handle_, kOpenFlagsForRead, NULL));
+    file_handle_ = base::kInvalidPlatformFileValue;
+    result_ = stream_impl->Seek(net::FROM_BEGIN, initial_offset,
+                                base::Bind(&DidSeekFile, callback,
+                                           base::Passed(&stream_impl),
+                                           initial_offset));
+    if (result_ != net::ERR_IO_PENDING)
+      callback.Run(result_, stream_impl.Pass());
+  }
+
+ private:
+  scoped_refptr<base::MessageLoopProxy> file_thread_proxy_;
+  base::PlatformFile file_handle_;
+  int result_;
+  DISALLOW_COPY_AND_ASSIGN(OpenFileStreamHelper);
+};
+
 LocalFileReader::LocalFileReader(
     base::MessageLoopProxy* file_thread_proxy,
     const FilePath& file_path,
@@ -60,6 +157,9 @@ LocalFileReader::LocalFileReader(
       weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {}
 
 LocalFileReader::~LocalFileReader() {
+  if (!stream_impl_.get())
+    return;
+  stream_impl_->Close(base::Bind(&EmptyCompletionCallback));
 }
 
 int LocalFileReader::Read(net::IOBuffer* buf, int buf_len,
@@ -67,113 +167,52 @@ int LocalFileReader::Read(net::IOBuffer* buf, int buf_len,
   DCHECK(!has_pending_open_);
   if (stream_impl_.get())
     return stream_impl_->Read(buf, buf_len, callback);
-  return Open(base::Bind(&LocalFileReader::DidOpenForRead,
-                         weak_factory_.GetWeakPtr(),
+  return Open(base::Bind(&LocalFileReader::DidOpen, weak_factory_.GetWeakPtr(),
                          make_scoped_refptr(buf), buf_len, callback));
 }
 
 int LocalFileReader::GetLength(const net::Int64CompletionCallback& callback) {
   const bool posted = base::FileUtilProxy::GetFileInfo(
       file_thread_proxy_, file_path_,
-      base::Bind(&LocalFileReader::DidGetFileInfoForGetLength,
-                 weak_factory_.GetWeakPtr(), callback));
+      base::Bind(&DidGetFileInfoForGetLength, callback,
+                 expected_modification_time_));
   DCHECK(posted);
   return net::ERR_IO_PENDING;
 }
 
-int LocalFileReader::Open(const net::CompletionCallback& callback) {
+int LocalFileReader::Open(const OpenFileStreamCallback& callback) {
   DCHECK(!has_pending_open_);
   DCHECK(!stream_impl_.get());
   has_pending_open_ = true;
-
-  // Call GetLength first to make it perform last-modified-time verification,
-  // and then call DidVerifyForOpen for do the rest.
-  return GetLength(base::Bind(&LocalFileReader::DidVerifyForOpen,
-                              weak_factory_.GetWeakPtr(), callback));
-}
-
-void LocalFileReader::DidVerifyForOpen(
-    const net::CompletionCallback& callback,
-    int64 get_length_result) {
-  if (get_length_result < 0) {
-    callback.Run(static_cast<int>(get_length_result));
-    return;
-  }
-
-  stream_impl_.reset(new net::FileStream(NULL));
-  const int result = stream_impl_->Open(
-      file_path_, kOpenFlagsForRead,
-      base::Bind(&LocalFileReader::DidOpenFileStream,
-                 weak_factory_.GetWeakPtr(),
-                 callback));
-  if (result != net::ERR_IO_PENDING)
-    callback.Run(result);
-}
-
-void LocalFileReader::DidOpenFileStream(
-    const net::CompletionCallback& callback,
-    int result) {
-  if (result != net::OK) {
-    callback.Run(result);
-    return;
-  }
-  result = stream_impl_->Seek(net::FROM_BEGIN, initial_offset_,
-                              base::Bind(&LocalFileReader::DidSeekFileStream,
-                                         weak_factory_.GetWeakPtr(),
-                                         callback));
-  if (result != net::ERR_IO_PENDING) {
-    callback.Run(result);
-  }
-}
-
-void LocalFileReader::DidSeekFileStream(
-    const net::CompletionCallback& callback,
-    int64 seek_result) {
-  if (seek_result < 0) {
-    callback.Run(static_cast<int>(seek_result));
-    return;
-  }
-  if (seek_result != initial_offset_) {
-    callback.Run(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
-    return;
-  }
-  callback.Run(net::OK);
+  OpenFileStreamHelper* helper = new OpenFileStreamHelper(file_thread_proxy_);
+  const bool posted = file_thread_proxy_->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&OpenFileStreamHelper::OpenAndVerifyOnFileThread,
+                 base::Unretained(helper), file_path_,
+                 expected_modification_time_),
+      base::Bind(&OpenFileStreamHelper::OpenStreamOnCallingThread,
+                 base::Owned(helper), initial_offset_, callback));
+  DCHECK(posted);
+  return net::ERR_IO_PENDING;
 }
 
-void LocalFileReader::DidOpenForRead(net::IOBuffer* buf,
-                                     int buf_len,
-                                     const net::CompletionCallback& callback,
-                                     int open_result) {
+void LocalFileReader::DidOpen(net::IOBuffer* buf,
+                              int buf_len,
+                              const net::CompletionCallback& callback,
+                              int open_error,
+                              scoped_ptr<net::FileStream> stream_impl) {
   DCHECK(has_pending_open_);
+  DCHECK(!stream_impl_.get());
   has_pending_open_ = false;
-  if (open_result != net::OK) {
-    stream_impl_.reset();
-    callback.Run(open_result);
+  if (open_error != net::OK) {
+    callback.Run(open_error);
     return;
   }
-  DCHECK(stream_impl_.get());
-  const int read_result = stream_impl_->Read(buf, buf_len, callback);
-  if (read_result != net::ERR_IO_PENDING)
-    callback.Run(read_result);
-}
-
-void LocalFileReader::DidGetFileInfoForGetLength(
-    const net::Int64CompletionCallback& callback,
-    base::PlatformFileError error,
-    const base::PlatformFileInfo& file_info) {
-  if (file_info.is_directory) {
-    callback.Run(net::ERR_FILE_NOT_FOUND);
-    return;
-  }
-  if (error != base::PLATFORM_FILE_OK) {
-    callback.Run(LocalFileReader::PlatformFileErrorToNetError(error));
-    return;
-  }
-  if (!VerifySnapshotTime(expected_modification_time_, file_info)) {
-    callback.Run(net::ERR_UPLOAD_FILE_CHANGED);
-    return;
-  }
-  callback.Run(file_info.size);
+  DCHECK(stream_impl.get());
+  stream_impl_ = stream_impl.Pass();
+  const int read_error = stream_impl_->Read(buf, buf_len, callback);
+  if (read_error != net::ERR_IO_PENDING)
+    callback.Run(read_error);
 }
 
 }  // namespace webkit_blob
diff --git a/webkit/blob/local_file_reader.h b/webkit/blob/local_file_reader.h
index 8f0508d..23abe7b 100644
--- a/webkit/blob/local_file_reader.h
+++ b/webkit/blob/local_file_reader.h
@@ -25,6 +25,9 @@ namespace webkit_blob {
 // handling.
 class BLOB_EXPORT LocalFileReader : public FileReader {
  public:
+  typedef base::Callback<void(int error, scoped_ptr<net::FileStream> stream)>
+      OpenFileStreamCallback;
+
   // A convenient method to translate platform file error to net error code.
   static int PlatformFileErrorToNetError(base::PlatformFileError file_error);
 
@@ -53,28 +56,17 @@ class BLOB_EXPORT LocalFileReader : public FileReader {
   // file info *and* its last modification time equals to
   // expected_modification_time_ (rv >= 0 cases).
   // Otherwise, a negative error code is returned (rv < 0 cases).
-  // If the stream is deleted while it has an in-flight GetLength operation
-  // |callback| will not be called.
   int GetLength(const net::Int64CompletionCallback& callback);
 
  private:
-  int Open(const net::CompletionCallback& callback);
-
-  // Callbacks that are chained from Open for Read.
-  void DidVerifyForOpen(const net::CompletionCallback& callback,
-                        int64 get_length_result);
-  void DidOpenFileStream(const net::CompletionCallback& callback,
-                         int result);
-  void DidSeekFileStream(const net::CompletionCallback& callback,
-                         int64 seek_result);
-  void DidOpenForRead(net::IOBuffer* buf,
-                      int buf_len,
-                      const net::CompletionCallback& callback,
-                      int open_result);
+  class OpenFileStreamHelper;
 
-  void DidGetFileInfoForGetLength(const net::Int64CompletionCallback& callback,
-                                  base::PlatformFileError error,
-                                  const base::PlatformFileInfo& file_info);
+  int Open(const OpenFileStreamCallback& callback);
+  void DidOpen(net::IOBuffer* buf,
+               int buf_len,
+               const net::CompletionCallback& callback,
+               int open_error,
+               scoped_ptr<net::FileStream> stream);
 
   scoped_refptr<base::MessageLoopProxy> file_thread_proxy_;
   scoped_ptr<net::FileStream> stream_impl_;
diff --git a/webkit/blob/local_file_reader_unittest.cc b/webkit/blob/local_file_reader_unittest.cc
deleted file mode 100644
index 7055c91..0000000
--- a/webkit/blob/local_file_reader_unittest.cc
+++ /dev/null
@@ -1,253 +0,0 @@
-// Copyright (c) 2012 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 "webkit/blob/local_file_reader.h"
-
-#include <string>
-
-#include "base/file_path.h"
-#include "base/file_util.h"
-#include "base/memory/scoped_ptr.h"
-#include "base/message_loop.h"
-#include "base/platform_file.h"
-#include "base/scoped_temp_dir.h"
-#include "base/threading/thread.h"
-#include "net/base/io_buffer.h"
-#include "net/base/net_errors.h"
-#include "net/base/test_completion_callback.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace webkit_blob {
-
-namespace {
-
-const char kTestData[] = "0123456789";
-const int kTestDataSize = arraysize(kTestData) - 1;
-
-void ReadFromReader(LocalFileReader* reader,
-                    std::string* data, size_t size,
-                    int* result) {
-  ASSERT_TRUE(reader != NULL);
-  ASSERT_TRUE(result != NULL);
-  *result = net::OK;
-  net::TestCompletionCallback callback;
-  size_t total_bytes_read = 0;
-  while (total_bytes_read < size) {
-    scoped_refptr<net::IOBufferWithSize> buf(
-        new net::IOBufferWithSize(size - total_bytes_read));
-    int rv = reader->Read(buf, buf->size(), callback.callback());
-    if (rv == net::ERR_IO_PENDING)
-      rv = callback.WaitForResult();
-    if (rv < 0)
-      *result = rv;
-    if (rv <= 0)
-      break;
-    total_bytes_read += rv;
-    data->append(buf->data(), rv);
-  }
-}
-
-void NeverCalled(int) {
-  ADD_FAILURE();
-}
-
-}  // namespace
-
-class LocalFileReaderTest : public testing::Test {
- public:
-  LocalFileReaderTest()
-      : message_loop_(MessageLoop::TYPE_IO),
-        file_thread_("FileUtilProxyTestFileThread") {}
-
-  virtual void SetUp() OVERRIDE {
-    ASSERT_TRUE(file_thread_.Start());
-    ASSERT_TRUE(dir_.CreateUniqueTempDir());
-
-    file_util::WriteFile(test_path(), kTestData, kTestDataSize);
-    base::PlatformFileInfo info;
-    ASSERT_TRUE(file_util::GetFileInfo(test_path(), &info));
-    test_file_modification_time_ = info.last_modified;
-  }
-
-  virtual void TearDown() OVERRIDE {
-    // Give another chance for deleted streams to perform Close.
-    MessageLoop::current()->RunAllPending();
-    file_thread_.Stop();
-  }
-
- protected:
-  LocalFileReader* CreateFileReader(
-      const FilePath& path,
-      int64 initial_offset,
-      const base::Time& expected_modification_time) {
-    return new LocalFileReader(
-            file_task_runner(),
-            path,
-            initial_offset,
-            expected_modification_time);
-  }
-
-  void TouchTestFile() {
-    base::Time new_modified_time =
-        test_file_modification_time() - base::TimeDelta::FromSeconds(1);
-    ASSERT_TRUE(file_util::TouchFile(test_path(),
-                                    test_file_modification_time(),
-                                    new_modified_time));
-  }
-
-  base::MessageLoopProxy* file_task_runner() const {
-    return file_thread_.message_loop_proxy().get();
-  }
-
-  FilePath test_dir() const { return dir_.path(); }
-  FilePath test_path() const { return dir_.path().AppendASCII("test"); }
-
-  base::Time test_file_modification_time() const {
-    return test_file_modification_time_;
-  }
-
- private:
-  MessageLoop message_loop_;
-  base::Thread file_thread_;
-  ScopedTempDir dir_;
-  base::Time test_file_modification_time_;
-};
-
-TEST_F(LocalFileReaderTest, NonExistent) {
-  FilePath nonexistent_path = test_dir().AppendASCII("nonexistent");
-  scoped_ptr<LocalFileReader> reader(
-      CreateFileReader(nonexistent_path, 0, base::Time()));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, 10, &result);
-  ASSERT_EQ(net::ERR_FILE_NOT_FOUND, result);
-  ASSERT_EQ(0U, data.size());
-}
-
-TEST_F(LocalFileReaderTest, Empty) {
-  FilePath empty_path = test_dir().AppendASCII("empty");
-  base::PlatformFileError error = base::PLATFORM_FILE_OK;
-  base::PlatformFile file = base::CreatePlatformFile(
-      empty_path,
-      base::PLATFORM_FILE_CREATE | base::PLATFORM_FILE_READ,
-      NULL, &error);
-  ASSERT_EQ(base::PLATFORM_FILE_OK, error);
-  ASSERT_NE(base::kInvalidPlatformFileValue, file);
-  base::ClosePlatformFile(file);
-
-  scoped_ptr<LocalFileReader> reader(
-      CreateFileReader(empty_path, 0, base::Time()));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, 10, &result);
-  ASSERT_EQ(net::OK, result);
-  ASSERT_EQ(0U, data.size());
-
-  net::TestInt64CompletionCallback callback;
-  result = reader->GetLength(callback.callback());
-  if (result == net::ERR_IO_PENDING)
-    result = callback.WaitForResult();
-  ASSERT_EQ(0, result);
-}
-
-TEST_F(LocalFileReaderTest, GetLengthNormal) {
-  scoped_ptr<LocalFileReader> reader(
-      CreateFileReader(test_path(), 0, test_file_modification_time()));
-  net::TestInt64CompletionCallback callback;
-  int result = reader->GetLength(callback.callback());
-  if (result == net::ERR_IO_PENDING)
-    result = callback.WaitForResult();
-  ASSERT_EQ(kTestDataSize, result);
-}
-
-TEST_F(LocalFileReaderTest, GetLengthAfterModified) {
-  // Touch file so that the file's modification time becomes different
-  // from what we expect.
-  TouchTestFile();
-
-  scoped_ptr<LocalFileReader> reader(
-      CreateFileReader(test_path(), 0, test_file_modification_time()));
-  net::TestInt64CompletionCallback callback;
-  int result = reader->GetLength(callback.callback());
-  if (result == net::ERR_IO_PENDING)
-    result = callback.WaitForResult();
-  ASSERT_EQ(net::ERR_UPLOAD_FILE_CHANGED, result);
-
-  // With NULL expected modification time this should work.
-  reader.reset(CreateFileReader(test_path(), 0, base::Time()));
-  result = reader->GetLength(callback.callback());
-  if (result == net::ERR_IO_PENDING)
-    result = callback.WaitForResult();
-  ASSERT_EQ(kTestDataSize, result);
-}
-
-TEST_F(LocalFileReaderTest, GetLengthWithOffset) {
-  scoped_ptr<LocalFileReader> reader(
-      CreateFileReader(test_path(), 3, base::Time()));
-  net::TestInt64CompletionCallback callback;
-  int result = reader->GetLength(callback.callback());
-  if (result == net::ERR_IO_PENDING)
-    result = callback.WaitForResult();
-  // Initial offset does not affect the result of GetLength.
-  ASSERT_EQ(kTestDataSize, result);
-}
-
-TEST_F(LocalFileReaderTest, ReadNormal) {
-  scoped_ptr<LocalFileReader> reader(
-      CreateFileReader(test_path(), 0, test_file_modification_time()));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, kTestDataSize, &result);
-  ASSERT_EQ(net::OK, result);
-  ASSERT_EQ(kTestData, data);
-}
-
-TEST_F(LocalFileReaderTest, ReadAfterModified) {
-  // Touch file so that the file's modification time becomes different
-  // from what we expect.
-  TouchTestFile();
-
-  scoped_ptr<LocalFileReader> reader(
-      CreateFileReader(test_path(), 0, test_file_modification_time()));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, kTestDataSize, &result);
-  ASSERT_EQ(net::ERR_UPLOAD_FILE_CHANGED, result);
-  ASSERT_EQ(0U, data.size());
-
-  // With NULL expected modification time this should work.
-  data.clear();
-  reader.reset(CreateFileReader(test_path(), 0, base::Time()));
-  ReadFromReader(reader.get(), &data, kTestDataSize, &result);
-  ASSERT_EQ(net::OK, result);
-  ASSERT_EQ(kTestData, data);
-}
-
-TEST_F(LocalFileReaderTest, ReadWithOffset) {
-  scoped_ptr<LocalFileReader> reader(
-      CreateFileReader(test_path(), 3, base::Time()));
-  int result = 0;
-  std::string data;
-  ReadFromReader(reader.get(), &data, kTestDataSize, &result);
-  ASSERT_EQ(net::OK, result);
-  ASSERT_EQ(&kTestData[3], data);
-}
-
-TEST_F(LocalFileReaderTest, DeleteWithUnfinishedRead) {
-  scoped_ptr<LocalFileReader> reader(
-      CreateFileReader(test_path(), 0, base::Time()));
-
-  net::TestCompletionCallback callback;
-  scoped_refptr<net::IOBufferWithSize> buf(
-      new net::IOBufferWithSize(kTestDataSize));
-  int rv = reader->Read(buf, buf->size(), base::Bind(&NeverCalled));
-  ASSERT_TRUE(rv == net::ERR_IO_PENDING || rv >= 0);
-
-  // Delete immediately.
-  // Should not crash; nor should NeverCalled be callback.
-  reader.reset();
-  MessageLoop::current()->RunAllPending();
-}
-
-}  // namespace webkit_blob