[Blob] Fix for resetting reader.

storage/browser/blob/blob_reader.cc

 // Copyright 2015 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/blob/blob_reader.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
 #include "base/sequenced_task_runner.h"
 #include "base/stl_util.h"
 #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"
 #include "storage/browser/fileapi/file_stream_reader.h"
 #include "storage/browser/fileapi/file_system_context.h"
 #include "storage/browser/fileapi/file_system_url.h"
 #include "storage/common/data_element.h"
 
 namespace storage {
 namespace {
 bool IsFileType(DataElement::Type type) {
   switch (type) {
     case DataElement::TYPE_FILE:
     case DataElement::TYPE_FILE_FILESYSTEM:
       return true;
     default:
       return false;
   }
 }
 }  // namespace
 
 BlobReader::FileStreamReaderProvider::~FileStreamReaderProvider() {}
 
 BlobReader::BlobReader(
     const BlobDataHandle* blob_handle,
     scoped_ptr<FileStreamReaderProvider> file_stream_provider,
     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();
   }
 }
 
 BlobReader::~BlobReader() {
-  STLDeleteValues(&index_to_reader_);
 }
 
 BlobReader::Status BlobReader::CalculateSize(
     const net::CompletionCallback& done) {
   DCHECK(!total_size_calculated_);
   DCHECK(size_callback_.is_null());
   if (!blob_data_.get()) {
     return ReportError(net::ERR_FILE_NOT_FOUND);
   }
 
+  reader_used_ = true;
   net_error_ = net::OK;
   total_size_ = 0;
   const auto& items = blob_data_->items();
   item_length_list_.resize(items.size());
   pending_get_file_info_count_ = 0;
   for (size_t i = 0; i < items.size(); ++i) {
     const BlobDataItem& item = *items.at(i);
     if (IsFileType(item.type())) {
       ++pending_get_file_info_count_;
       storage::FileStreamReader* const reader = GetOrCreateFileReaderAtIndex(i);
@@ skipping 105 matching lines @@
 
   Status status = ReadLoop(bytes_read);
   if (status == Status::IO_PENDING)
     read_callback_ = done;
   return status;
 }
 
 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 {
   if (!blob_data_.get()) {
     return true;
   }
   for (const auto& item : blob_data_->items()) {
     if (item->type() != DataElement::TYPE_BYTES) {
       return false;
     }
@@ skipping 298 matching lines @@
 }
 
 FileStreamReader* BlobReader::GetOrCreateFileReaderAtIndex(size_t index) {
   const auto& items = blob_data_->items();
   DCHECK_LT(index, items.size());
   const BlobDataItem& item = *items.at(index);
   if (!IsFileType(item.type()))
     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;
 }
 
 scoped_ptr<FileStreamReader> BlobReader::CreateFileStreamReader(
     const BlobDataItem& item,
     uint64_t additional_offset) {
   DCHECK(IsFileType(item.type()));
 
   switch (item.type()) {
     case DataElement::TYPE_FILE:
@@ skipping 16 matching lines @@
     case DataElement::TYPE_BYTES_DESCRIPTION:
     case DataElement::TYPE_DISK_CACHE_ENTRY:
     case DataElement::TYPE_UNKNOWN:
       break;
   }
 
   NOTREACHED();
   return nullptr;
 }
 
 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