Relanding r134248 with leak fix in tests - Make LocalFileReader deletable at any time and add unittests.

Added one-line change (to force running pending cleanup tasks) for leak fix in the test.

Original review URL: https://chromiumcodereview.appspot.com/10234006

BUG=123570
TEST=unit_tests:LocalFileReaderTest.*
TBR=michaeln

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

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

4 files changed, 369 insertions, 137 deletions

diff --git a/webkit/blob/file_reader.h b/webkit/blob/file_reader.h
index 78dfbf9..8aedf40 100644
--- a/webkit/blob/file_reader.h
+++ b/webkit/blob/file_reader.h
@@ -21,6 +21,8 @@ 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.
@@ -33,6 +35,9 @@ 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 068ad13..8b5fb05 100644
--- a/webkit/blob/local_file_reader.cc
+++ b/webkit/blob/local_file_reader.cc
@@ -30,39 +30,6 @@ 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
@@ -80,70 +47,6 @@ 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,
@@ -157,9 +60,6 @@ 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,
@@ -167,52 +67,113 @@ 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::DidOpen, weak_factory_.GetWeakPtr(),
+  return Open(base::Bind(&LocalFileReader::DidOpenForRead,
+                         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(&DidGetFileInfoForGetLength, callback,
-                 expected_modification_time_));
+      base::Bind(&LocalFileReader::DidGetFileInfoForGetLength,
+                 weak_factory_.GetWeakPtr(), callback));
   DCHECK(posted);
   return net::ERR_IO_PENDING;
 }
 
-int LocalFileReader::Open(const OpenFileStreamCallback& callback) {
+int LocalFileReader::Open(const net::CompletionCallback& callback) {
   DCHECK(!has_pending_open_);
   DCHECK(!stream_impl_.get());
   has_pending_open_ = true;
-  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;
+
+  // 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::DidOpen(net::IOBuffer* buf,
-                              int buf_len,
-                              const net::CompletionCallback& callback,
-                              int open_error,
-                              scoped_ptr<net::FileStream> stream_impl) {
+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);
+}
+
+void LocalFileReader::DidOpenForRead(net::IOBuffer* buf,
+                                     int buf_len,
+                                     const net::CompletionCallback& callback,
+                                     int open_result) {
   DCHECK(has_pending_open_);
-  DCHECK(!stream_impl_.get());
   has_pending_open_ = false;
-  if (open_error != net::OK) {
-    callback.Run(open_error);
+  if (open_result != net::OK) {
+    stream_impl_.reset();
+    callback.Run(open_result);
     return;
   }
-  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);
+  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);
 }
 
 }  // namespace webkit_blob
diff --git a/webkit/blob/local_file_reader.h b/webkit/blob/local_file_reader.h
index 23abe7b..8f0508d 100644
--- a/webkit/blob/local_file_reader.h
+++ b/webkit/blob/local_file_reader.h
@@ -25,9 +25,6 @@ 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);
 
@@ -56,17 +53,28 @@ 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:
-  class OpenFileStreamHelper;
+  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);
 
-  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);
+  void DidGetFileInfoForGetLength(const net::Int64CompletionCallback& callback,
+                                  base::PlatformFileError error,
+                                  const base::PlatformFileInfo& file_info);
 
   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
new file mode 100644
index 0000000..6ddf0eb
--- /dev/null
+++ b/webkit/blob/local_file_reader_unittest.cc
@@ -0,0 +1,258 @@
+// 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();
+}
+
+void QuitLoop() {
+  MessageLoop::current()->Quit();
+}
+
+}  // 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()->PostTask(FROM_HERE, base::Bind(&QuitLoop));
+    MessageLoop::current()->Run();
+    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