Reland 132134 with memory fix - Use LocalFileReader in FileSystemURLRequestJob

LocalFileReader change https://chromiumcodereview.appspot.com/10065011
with memory fix http://codereview.chromium.org/10067031/

BUG=113300, 114999, 123302
TEST=FileSystemURLRequestJobTest.*

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

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

webkit/blob/blob_url_request_job.cc

@@ -43,10 +43,6 @@ const char kHTTPRequestedRangeNotSatisfiableText[] =
     "Requested Range Not Satisfiable";
 const char kHTTPInternalErrorText[] = "Internal Server Error";
 
-const int kFileOpenFlags = base::PLATFORM_FILE_OPEN |
-                           base::PLATFORM_FILE_READ |
-                           base::PLATFORM_FILE_ASYNC;
-
 }  // namespace
 
 BlobURLRequestJob::BlobURLRequestJob(
@@ -361,12 +357,18 @@ bool BlobURLRequestJob::ReadFileItem(LocalFileReader* reader,
       read_buf_, bytes_to_read,
       base::Bind(&BlobURLRequestJob::DidReadFile,
                  base::Unretained(this)));
-  if (result != net::ERR_IO_PENDING) {
-    DCHECK(result != net::OK);
-    NotifyFailure(result);
-    return false;
+  if (result >= 0) {
+    // Data is immediately available.
+    if (GetStatus().is_io_pending())
+      DidReadFile(result);
+    else
+      AdvanceBytesRead(result);
+    return true;
   }
+  if (result == net::ERR_IO_PENDING)
     SetStatus(net::URLRequestStatus(net::URLRequestStatus::IO_PENDING, 0));
+  else
+    NotifyFailure(result);
   return false;
 }
 

webkit/blob/local_file_reader.cc

@@ -70,7 +70,7 @@ void DidSeekFile(const LocalFileReader::OpenFileStreamCallback& callback,
   if (new_offset < 0)
     result = static_cast<int>(new_offset);
   else if (new_offset != initial_offset)
-    result = net::ERR_FAILED;
+    result = net::ERR_REQUEST_RANGE_NOT_SATISFIABLE;
   callback.Run(result, stream_impl.Pass());
 }
 

webkit/blob/local_file_reader.h

@@ -34,7 +34,9 @@ class BLOB_EXPORT LocalFileReader {
 
   // Creates a new FileReader for a local file |file_path|.
   // |initial_offset| specifies the offset in the file where the first read
-  // should start.
+  // should start.  If the given offset is out of the file range any
+  // read operation may error out with net::ERR_REQUEST_RANGE_NOT_SATISFIABLE.
+  //
   // |expected_modification_time| specifies the expected last modification
   // If the value is non-null, the reader will check the underlying file's
   // actual modification time to see if the file has been modified, and if

webkit/fileapi/file_system_url_request_job.cc

@@ -4,8 +4,6 @@
 
 #include "webkit/fileapi/file_system_url_request_job.h"
 
-#include <vector>
-
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
@@ -13,6 +11,7 @@
 #include "base/message_loop.h"
 #include "base/platform_file.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/time.h"
 #include "build/build_config.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/file_stream.h"
@@ -24,6 +23,7 @@
 #include "net/http/http_response_info.h"
 #include "net/http/http_util.h"
 #include "net/url_request/url_request.h"
+#include "webkit/blob/local_file_reader.h"
 #include "webkit/blob/shareable_file_reference.h"
 #include "webkit/fileapi/file_system_context.h"
 #include "webkit/fileapi/file_system_operation.h"
@@ -32,13 +32,10 @@
 using net::URLRequest;
 using net::URLRequestJob;
 using net::URLRequestStatus;
+using webkit_blob::LocalFileReader;
 
 namespace fileapi {
 
-static const int kFileFlags = base::PLATFORM_FILE_OPEN |
-                              base::PLATFORM_FILE_READ |
-                              base::PLATFORM_FILE_ASYNC;
-
 static net::HttpResponseHeaders* CreateHttpResponseHeaders() {
   // HttpResponseHeaders expects its input string to be terminated by two NULs.
   static const char kStatus[] = "HTTP/1.1 200 OK\0";
@@ -62,20 +59,11 @@ FileSystemURLRequestJob::FileSystemURLRequestJob(
       file_system_context_(file_system_context),
       file_thread_proxy_(file_thread_proxy),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
-      stream_(NULL),
       is_directory_(false),
       remaining_bytes_(0) {
 }
 
-FileSystemURLRequestJob::~FileSystemURLRequestJob() {
-  // Since we use the two-arg constructor of FileStream, we need to call Close()
-  // manually: ~FileStream won't call it for us.
-  if (stream_ != NULL) {
-    // Close() performs file IO: crbug.com/113300.
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-    stream_->CloseSync();
-  }
-}
+FileSystemURLRequestJob::~FileSystemURLRequestJob() {}
 
 void FileSystemURLRequestJob::Start() {
   MessageLoop::current()->PostTask(
@@ -85,12 +73,8 @@ void FileSystemURLRequestJob::Start() {
 }
 
 void FileSystemURLRequestJob::Kill() {
-  if (stream_ != NULL) {
-    // Close() performs file IO: crbug.com/113300.
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-    stream_->CloseSync();
-    stream_.reset(NULL);
-  }
+  if (reader_.get() != NULL)
+    reader_.reset();
   URLRequestJob::Kill();
   weak_factory_.InvalidateWeakPtrs();
 }
@@ -101,7 +85,7 @@ bool FileSystemURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size,
   DCHECK(bytes_read);
   DCHECK_GE(remaining_bytes_, 0);
 
-  if (stream_ == NULL)
+  if (reader_.get() == NULL)
     return false;
 
   if (remaining_bytes_ < dest_size)
@@ -112,7 +96,7 @@ bool FileSystemURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size,
     return true;
   }
 
-  int rv = stream_->Read(dest, dest_size,
+  const int rv = reader_->Read(dest, dest_size,
                                base::Bind(&FileSystemURLRequestJob::DidRead,
                                           base::Unretained(this)));
   if (rv >= 0) {
@@ -122,8 +106,6 @@ bool FileSystemURLRequestJob::ReadRawData(net::IOBuffer* dest, int dest_size,
     DCHECK_GE(remaining_bytes_, 0);
     return true;
   }
-
-  // Otherwise, a read error occured.  We may just need to wait...
   if (rv == net::ERR_IO_PENDING)
     SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
   else
@@ -214,47 +196,24 @@ void FileSystemURLRequestJob::DidCreateSnapshot(
     return;
   }
 
-  if (!is_directory_) {
-    base::FileUtilProxy::CreateOrOpen(
-        file_thread_proxy_, platform_path, kFileFlags,
-        base::Bind(&FileSystemURLRequestJob::DidOpen,
-                   weak_factory_.GetWeakPtr()));
-  } else {
+  if (is_directory_) {
     NotifyHeadersComplete();
-  }
-}
-
-void FileSystemURLRequestJob::DidOpen(base::PlatformFileError error_code,
-                                      base::PassPlatformFile file,
-                                      bool created) {
-  if (error_code != base::PLATFORM_FILE_OK) {
-    NotifyFailed(error_code);
     return;
   }
 
-  stream_.reset(new net::FileStream(file.ReleaseValue(), kFileFlags, NULL));
-
   remaining_bytes_ = byte_range_.last_byte_position() -
                      byte_range_.first_byte_position() + 1;
   DCHECK_GE(remaining_bytes_, 0);
 
-  // TODO(adamk): Please remove this ScopedAllowIO once we support async seek
-  // on FileStream. crbug.com/113300
-  base::ThreadRestrictions::ScopedAllowIO allow_io;
-  // Do the seek at the beginning of the request.
-  if (remaining_bytes_ > 0 &&
-      byte_range_.first_byte_position() != 0 &&
-      byte_range_.first_byte_position() !=
-          stream_->SeekSync(net::FROM_BEGIN,
-                            byte_range_.first_byte_position())) {
-    NotifyFailed(net::ERR_REQUEST_RANGE_NOT_SATISFIABLE);
-    return;
-  }
+  DCHECK(!reader_.get());
+  reader_.reset(new LocalFileReader(
+      file_thread_proxy_, platform_path,
+      byte_range_.first_byte_position(),
+      base::Time()));
 
   set_expected_content_size(remaining_bytes_);
   response_info_.reset(new net::HttpResponseInfo());
   response_info_->headers = CreateHttpResponseHeaders();
-
   NotifyHeadersComplete();
 }
 

webkit/fileapi/file_system_url_request_job.h

@@ -19,11 +19,8 @@
 class GURL;
 class FilePath;
 
-namespace net {
-class FileStream;
-}
-
 namespace webkit_blob {
+class LocalFileReader;
 class ShareableFileReference;
 }
 
@@ -73,7 +70,7 @@ class FileSystemURLRequestJob : public net::URLRequestJob {
   FileSystemContext* file_system_context_;
   scoped_refptr<base::MessageLoopProxy> file_thread_proxy_;
   base::WeakPtrFactory<FileSystemURLRequestJob> weak_factory_;
-  scoped_ptr<net::FileStream> stream_;
+  scoped_ptr<webkit_blob::LocalFileReader> reader_;
   scoped_refptr<webkit_blob::ShareableFileReference> snapshot_ref_;
   bool is_directory_;
   scoped_ptr<net::HttpResponseInfo> response_info_;

webkit/fileapi/file_system_url_request_job_unittest.cc

@@ -70,7 +70,7 @@ class FileSystemURLRequestJobTest : public testing::Test {
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
   }
 
-  virtual void SetUp() {
+  virtual void SetUp() OVERRIDE {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
 
     special_storage_policy_ = new quota::MockSpecialStoragePolicy;
@@ -94,9 +94,15 @@ class FileSystemURLRequestJobTest : public testing::Test {
         "filesystem", &FileSystemURLRequestJobFactory);
   }
 
-  virtual void TearDown() {
+  virtual void TearDown() OVERRIDE {
     net::URLRequest::Deprecated::RegisterProtocolFactory("filesystem", NULL);
     ClearUnusedJob();
+    if (pending_job_) {
+      pending_job_->Kill();
+      pending_job_ = NULL;
+    }
+    // FileReader posts a task to close the file in destructor.
+    MessageLoop::current()->RunAllPending();
   }
 
   void OnValidateFileSystem(base::PlatformFileError result) {
@@ -113,10 +119,12 @@ class FileSystemURLRequestJobTest : public testing::Test {
     request_.reset(new net::URLRequest(url, delegate_.get()));
     if (headers)
       request_->SetExtraRequestHeaders(*headers);
+    ASSERT_TRUE(!job_);
     job_ = new FileSystemURLRequestJob(
         request_.get(),
         file_system_context_.get(),
         base::MessageLoopProxy::current());
+    pending_job_ = job_;
 
     request_->Start();
     ASSERT_TRUE(request_->is_pending());  // verify that we're starting async
@@ -212,6 +220,7 @@ class FileSystemURLRequestJobTest : public testing::Test {
   scoped_ptr<TestDelegate> delegate_;
   scoped_ptr<net::URLRequest> request_;
 
+  scoped_refptr<net::URLRequestJob> pending_job_;
   static net::URLRequestJob* job_;
 };