IndexedDB: Fixed support for empty blobs.

storage/browser/fileapi/file_writer_delegate.cc

 // 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 "storage/browser/fileapi/file_writer_delegate.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file_util_proxy.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 13 matching lines @@
 
 FileWriterDelegate::FileWriterDelegate(
     scoped_ptr<FileStreamWriter> file_stream_writer,
     FlushPolicy flush_policy)
     : file_stream_writer_(file_stream_writer.Pass()),
       writing_started_(false),
       flush_policy_(flush_policy),
       bytes_written_backlog_(0),
       bytes_written_(0),
       bytes_read_(0),
+      total_bytes_read_(0),
       io_buffer_(new net::IOBufferWithSize(kReadBufSize)),
       weak_factory_(this) {
 }
 
 FileWriterDelegate::~FileWriterDelegate() {
 }
 
 void FileWriterDelegate::Start(scoped_ptr<net::URLRequest> request,
                                const DelegateWriteCallback& write_callback) {
   write_callback_ = write_callback;
@@ skipping 84 matching lines @@
         FROM_HERE,
         base::Bind(&FileWriterDelegate::OnDataReceived,
                    weak_factory_.GetWeakPtr(), bytes_read_));
   } else if (!request_->status().is_io_pending()) {
     OnError(base::File::FILE_ERROR_FAILED);
   }
 }
 
 void FileWriterDelegate::OnDataReceived(int bytes_read) {
   bytes_read_ = bytes_read;
-  if (!bytes_read_) {  // We're done.
-    OnProgress(0, true);
-  } else {
+  total_bytes_read_ += bytes_read;
+
+  if (bytes_read_ || !total_bytes_read_) {
+    // If we received bytes then write them. Zero bytes signals EOF, but if no

[jsbell, 2015/02/25 19:53:27]

Excellent comment, thanks!

+    // bytes were ever read then still call Write(). This is necessary because
+    // the file is created when the first chunk of data is received, but we need
+    // to support zero length files so explicitly write zero bytes to force the
+    // file creation.

[michaeln, 2015/02/25 20:35:43]

Should we fix this problem directly in the FileStreamWriter class? It fails to 
create a file when kCreateFlagsForWrite is used but then 0 bytes are actually
written, that seems like a bug in FileStreamWriter.

[cmumford, 2015/02/25 22:37:29]

Was reading the comment above net::FileStream::Writer, and just saw this: "Zero
byte writes are not allowed". I think I'm going to have to take a different
approach where I don't call FileStreamWriter::Write() with zero bytes, and
instead use base::File to create an empty file.

[cmumford, 2015/02/25 23:52:49]

On second thought, I'm back to the idea of fixing FileStreamWriter. Do you think
that we might create problems elsewhere where a FileStreamWriter is created, but
Write is never called?

[michaeln, 2015/02/26 02:05:37]

Maybe fix that class w/o altering behavior for existing callers.

  enum OpenOrCreate { OPEN_EXISTING_FILE, CREATE_NEW_FILE, CREATE_NEW_EMPTY_FILE
};

Where the new 3rd enum means

  delete CreateForLocalFile(task_runner, ..., CREATE_NEW_EMPTY_FILE);

should result in a task having been posted to the task_runner to create the
file.

+    //
     // This could easily be optimized to rotate between a pool of buffers, so
     // that we could read and write at the same time.  It's not yet clear that
     // it's necessary.
     cursor_ = new net::DrainableIOBuffer(io_buffer_.get(), bytes_read_);
     Write();
   }
+  if (!bytes_read_)  // We're done.
+    OnProgress(0, true);
 }
 
 void FileWriterDelegate::Write() {
   writing_started_ = true;
   int64 bytes_to_write = bytes_read_ - bytes_written_;

[michaeln, 2015/02/25 20:35:43]

It looks like bytes_to_write here will go negative in some cases, will
FileStreamWriter::Write be ok with that?

[michaeln, 2015/02/25 21:55:42]

oh, nm it cant be < zero

   int write_response =
       file_stream_writer_->Write(cursor_.get(),

[michaeln, 2015/02/25 21:55:42]

I'm not sure if this is depending on a return value of 0 immediately or if its
expecting IO_PENDING and a later call to OnDataWritten(0) , either type of
return would be valid and FileStreamWriter is an abstract interface so 
different subclasses may handle this case differently.

[cmumford, 2015/02/25 22:37:30]

I think that now that I won't be calling Write() I'll create a new method
(CreateEmpty???) which just creates the file, and then OnDataWritten(0), etc.

[michaeln, 2015/02/26 02:05:37]

The lack of a file being created by FileStreamWriter definitely was unexpected
behavior. I do think it's best to put the fix in that class. It would document
the existing behavior of the existing CREATE enum and give callers an easy way
to get the behavior they most likely want.

                                  static_cast<int>(bytes_to_write),
                                  base::Bind(&FileWriterDelegate::OnDataWritten,
                                             weak_factory_.GetWeakPtr()));
   if (write_response > 0) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(&FileWriterDelegate::OnDataWritten,
                    weak_factory_.GetWeakPtr(), write_response));
   } else if (net::ERR_IO_PENDING != write_response) {
     OnError(NetErrorToFileError(write_response));
@@ skipping 63 matching lines @@
     base::File::Error error,
     int bytes_written,
     WriteProgressStatus progress_status) {
   if (flush_policy_ == FlushPolicy::NO_FLUSH_ON_COMPLETION) {
     write_callback_.Run(error, bytes_written, progress_status);
     return;
   }
   // DCHECK_EQ on enum classes is not supported.
   DCHECK(flush_policy_ == FlushPolicy::FLUSH_ON_COMPLETION);
 
-  int flush_error = file_stream_writer_->Flush(
-      base::Bind(&FileWriterDelegate::OnFlushed, weak_factory_.GetWeakPtr(),
-                 error, bytes_written, progress_status));
+  int flush_error = 0;
+  if (bytes_written) {
+    flush_error = file_stream_writer_->Flush(
+        base::Bind(&FileWriterDelegate::OnFlushed, weak_factory_.GetWeakPtr(),
+                   error, bytes_written, progress_status));
+  }
   if (flush_error != net::ERR_IO_PENDING)
     OnFlushed(error, bytes_written, progress_status, flush_error);
 }
 
 void FileWriterDelegate::OnFlushed(base::File::Error error,
                                    int bytes_written,
                                    WriteProgressStatus progress_status,
                                    int flush_error) {
   if (error == base::File::FILE_OK && flush_error != net::OK) {
     // If the Flush introduced an error, overwrite the status.
     // Otherwise, keep the original error status.
     error = NetErrorToFileError(flush_error);
     progress_status = GetCompletionStatusOnError();
   }
   write_callback_.Run(error, bytes_written, progress_status);
 }
 
 }  // namespace storage