[Blob] Fix for resetting reader.

storage/browser/blob/blob_reader.cc

Index: storage/browser/blob/blob_reader.cc
diff --git a/storage/browser/blob/blob_reader.cc b/storage/browser/blob/blob_reader.cc
index 9c6375d5c7d3b2a782f4554068333d8f81bca628..12f2c4577e89492e8a3b12e5df384c9c8a07678b 100644
--- a/storage/browser/blob/blob_reader.cc
+++ b/storage/browser/blob/blob_reader.cc
@@ -13,7 +13,6 @@
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "net/base/io_buffer.h"
-#include "net/base/net_errors.h"
 #include "net/disk_cache/disk_cache.h"
 #include "storage/browser/blob/blob_data_handle.h"
 #include "storage/browser/blob/blob_data_snapshot.h"
@@ -43,7 +42,6 @@ BlobReader::BlobReader(
     base::SequencedTaskRunner* file_task_runner)
     : file_stream_provider_(file_stream_provider.Pass()),
       file_task_runner_(file_task_runner),
-      net_error_(net::OK),
       weak_factory_(this) {
   if (blob_handle) {
     blob_data_ = blob_handle->CreateSnapshot().Pass();
@@ -51,7 +49,6 @@ BlobReader::BlobReader(
 }
 
 BlobReader::~BlobReader() {
-  STLDeleteValues(&index_to_reader_);
 }
 
 BlobReader::Status BlobReader::CalculateSize(
@@ -62,6 +59,7 @@ BlobReader::Status BlobReader::CalculateSize(
     return ReportError(net::ERR_FILE_NOT_FOUND);
   }
 
+  reader_used_ = true;
   net_error_ = net::OK;
   total_size_ = 0;
   const auto& items = blob_data_->items();
@@ -187,6 +185,21 @@ BlobReader::Status BlobReader::Read(net::IOBuffer* buffer,
 void BlobReader::Kill() {
   DeleteCurrentFileReader();
   weak_factory_.InvalidateWeakPtrs();
+  read_buf_ = nullptr;
+  io_pending_ = false;
+}
+
+void BlobReader::Reset() {
+  weak_factory_.InvalidateWeakPtrs();
+  io_pending_ = false;
+  reader_used_ = false;
+  total_size_calculated_ = false;
+  current_item_index_ = 0;
+  current_item_offset_ = false;
+  // We keep the map of file readers to be efficient.

[michaeln, 2015/12/11 00:22:18]

i think the readers maintain state like the current read position, and what if
there's a pending io call when reset is called, you've snipped the completion
callbacks to this class here, but a reader could still be busy internally

another approach could be for the uploader to Kill() its blob reader and create
an entirely one if it's being reinitted?

[michaeln, 2015/12/11 00:48:16]

It's real easy to miss resetting a particular member, more than one is not be
reset in here currently. I also see the BlobReader ctor is protected and its not
super easy to constuct a new one.

Maybe a reader->is_dirty() method and a newreader = oldreader->Recreate() method
to get everything reliably reinitted.

[dmurph, 2015/12/11 01:32:04]

If we do that, then we'll need to make the old one invalid, as we have a couple
scoped_ptr's we'd have to move. That seems more confusing than just having an
init method.  What if we did InitAndCalculateSize?

+  size_callback_.Reset();
+  read_callback_.Reset();
+  read_buf_ = nullptr;
 }
 
 bool BlobReader::IsInMemory() const {
@@ -505,14 +518,14 @@ FileStreamReader* BlobReader::GetOrCreateFileReaderAtIndex(size_t index) {
     return nullptr;
   auto it = index_to_reader_.find(index);
   if (it != index_to_reader_.end()) {
-    DCHECK(it->second);
-    return it->second;
+    DCHECK(it->second.get());
+    return it->second.get();
   }
   scoped_ptr<FileStreamReader> reader = CreateFileStreamReader(item, 0);
   FileStreamReader* ret_value = reader.get();
   if (!ret_value)
     return nullptr;
-  index_to_reader_[index] = reader.release();
+  index_to_reader_[index] = std::move(reader);
   return ret_value;
 }
 
@@ -551,19 +564,11 @@ scoped_ptr<FileStreamReader> BlobReader::CreateFileStreamReader(
 
 void BlobReader::SetFileReaderAtIndex(size_t index,

[michaeln, 2015/12/11 00:48:16]

The old impl completely ignored |index|, the rewrite uses it in one place but
not the other.

[dmurph, 2015/12/11 01:32:04]

Old was incorrect then.

                                       scoped_ptr<FileStreamReader> reader) {
-  auto found = index_to_reader_.find(current_item_index_);
-  if (found != index_to_reader_.end()) {
-    if (found->second) {
-      delete found->second;
-    }
-    if (!reader.get()) {
-      index_to_reader_.erase(found);
-      return;
-    }
-    found->second = reader.release();
-  } else if (reader.get()) {
-    index_to_reader_[current_item_index_] = reader.release();
+  if (!reader.get()) {
+    index_to_reader_.erase(index);
+    return;
   }
+  index_to_reader_[current_item_index_] = std::move(reader);
 }
 
 }  // namespace storage