Set error on WebDataConsumerHandle while loading body.

content/child/shared_memory_data_consumer_handle.cc

Index: content/child/shared_memory_data_consumer_handle.cc
diff --git a/content/child/shared_memory_data_consumer_handle.cc b/content/child/shared_memory_data_consumer_handle.cc
index 24db0590a39ba9ab05ae041478950666de398a2a..9fef531e2bd76a00b69a9cbaf734b284e2390381 100644
--- a/content/child/shared_memory_data_consumer_handle.cc
+++ b/content/child/shared_memory_data_consumer_handle.cc
@@ -53,7 +53,8 @@ class SharedMemoryDataConsumerHandle::Context final
       : result_(Ok),
         first_offset_(0),
         client_(nullptr),
-        is_handle_active_(true) {}
+        is_handle_active_(true),
+        is_two_phase_read_in_progress_(false) {}
 
   bool IsEmpty() const { return queue_.empty(); }
   void ClearIfNecessary() {
@@ -62,6 +63,13 @@ class SharedMemoryDataConsumerHandle::Context final
       Clear();
     }
   }
+  void ClearQueue() {
+    for (auto& data : queue_) {
+      delete data;
+    }
+    queue_.clear();
+    first_offset_ = 0;
+  }
   RequestPeer::ThreadSafeReceivedData* Top() { return queue_.front(); }
   void Push(scoped_ptr<RequestPeer::ThreadSafeReceivedData> data) {
     queue_.push_back(data.release());
@@ -103,6 +111,12 @@ class SharedMemoryDataConsumerHandle::Context final
       first_offset_ = 0;
     }
   }
+  bool is_two_phase_read_in_progress() const {
+    return is_two_phase_read_in_progress_;
+  }
+  void set_is_two_phase_read_in_progress(bool b) {
+    is_two_phase_read_in_progress_ = b;
+  }
   base::Lock& lock() { return lock_; }
 
  private:
@@ -154,6 +168,7 @@ class SharedMemoryDataConsumerHandle::Context final
   Client* client_;
   scoped_refptr<base::SingleThreadTaskRunner> notification_task_runner_;
   bool is_handle_active_;
+  bool is_two_phase_read_in_progress_;
 
   DISALLOW_COPY_AND_ASSIGN(Context);
 };
@@ -212,6 +227,29 @@ void SharedMemoryDataConsumerHandle::Writer::Close() {
     context_->Notify();
 }
 
+void SharedMemoryDataConsumerHandle::Writer::Error() {
+  bool needs_notification = false;
+
+  {
+    base::AutoLock lock(context_->lock());
+    if (context_->result() == Ok) {
+      // TODO(yhirano): Use an appropriate error code other than
+      // UnexpectedError.
+      context_->set_result(UnexpectedError);
+
+      if (!context_->is_two_phase_read_in_progress()) {
+        // If we are in two-phase read session, we cannot discard the data.
+        // Otherwise, clear the data.

[hiroshige, 2015/06/18 07:01:20]

Please mention in the comment when/how ClearQueue() is called when
is_two_phase_read_in_progress() is true.
(In my understandings, if two-phase read is ongoing then ClearQueue() is called
in endRead() because we set_result(UnexpectedError) here.)

[yhirano, 2015/06/18 08:13:33]

Done.

+        context_->ClearQueue();
+      }
+
+      needs_notification = true;
+    }
+  }
+  if (needs_notification)
+    context_->Notify();
+}
+
 SharedMemoryDataConsumerHandle::ReaderImpl::ReaderImpl(
     scoped_refptr<Context> context,
     Client* client)
@@ -236,6 +274,10 @@ Result SharedMemoryDataConsumerHandle::ReaderImpl::read(
 
   size_t total_read_size = 0;
   *read_size_to_return = 0;
+
+  if (context_->is_two_phase_read_in_progress())
+    context_->set_result(UnexpectedError);

[hiroshige, 2015/06/18 07:01:21]

Is this change for setting errors on read()/beginRead() during two-phase read
(which was previously unchecked) and thus independent from error notification to
WebDataConsumerHandle?
If so, please mention this in the CL description.

[yhirano, 2015/06/18 08:13:33]

Done. By the way, I added a condition to stabilize the result.

+
   if (context_->result() != Ok && context_->result() != Done)
     return context_->result();
 
@@ -263,12 +305,16 @@ Result SharedMemoryDataConsumerHandle::ReaderImpl::beginRead(
 
   base::AutoLock lock(context_->lock());
 
+  if (context_->is_two_phase_read_in_progress())
+    context_->set_result(UnexpectedError);

[hiroshige, 2015/06/18 07:01:21]

ditto.

[yhirano, 2015/06/18 08:13:33]

Done.

+
   if (context_->result() != Ok && context_->result() != Done)
     return context_->result();
 
   if (context_->IsEmpty())
     return context_->result() == Done ? Done : ShouldWait;
 
+  context_->set_is_two_phase_read_in_progress(true);
   const auto& top = context_->Top();
   *buffer = top->payload() + context_->first_offset();
   *available = top->length() - context_->first_offset();
@@ -279,10 +325,17 @@ Result SharedMemoryDataConsumerHandle::ReaderImpl::beginRead(
 Result SharedMemoryDataConsumerHandle::ReaderImpl::endRead(size_t read_size) {
   base::AutoLock lock(context_->lock());
 
-  if (context_->IsEmpty())
+  if (!context_->is_two_phase_read_in_progress())
     return UnexpectedError;

[hiroshige, 2015/06/18 07:01:20]

Can/should we call ClearQueue() in this then clause? (not sure)

[yhirano, 2015/06/18 08:13:33]

I do not want to do so, because it may complicate the code. Clearing the queue
is not mandatory in general: it should be cleared in the destruction process. I
added early-clear when closed / errored for performance. In L328 case, a user
does something wrong. I don't want to implement such optimization for bad users.

[yhirano, 2015/06/18 08:14:51]

Correction: I added early-clear when errored

[hiroshige, 2015/06/18 08:35:11]

OK.

 
-  context_->Consume(read_size);
+  context_->set_is_two_phase_read_in_progress(false);
+  if (context_->result() != Ok && context_->result() != Done) {
+    // We have an error, so we can discard the stored data.
+    context_->ClearQueue();
+  } else {
+    context_->Consume(read_size);
+  }
+
   return Ok;
 }