SPDY: Defer SpdyBufferProducer deletions in SpdyWriteQueue

net/spdy/spdy_write_queue.cc

Index: net/spdy/spdy_write_queue.cc
diff --git a/net/spdy/spdy_write_queue.cc b/net/spdy/spdy_write_queue.cc
index 1d6937449c5d9bea39b88b6e5d122267d07c9e28..44867ef8d4f51e1ee8a3c693bfae2135a54eeb51 100644
--- a/net/spdy/spdy_write_queue.cc
+++ b/net/spdy/spdy_write_queue.cc
@@ -5,8 +5,10 @@
 #include "net/spdy/spdy_write_queue.h"
 
 #include <cstddef>
+#include <vector>
 
 #include "base/logging.h"
+#include "base/stl_util.h"
 #include "net/spdy/spdy_buffer.h"
 #include "net/spdy/spdy_buffer_producer.h"
 #include "net/spdy/spdy_stream.h"
@@ -93,13 +95,17 @@ void SpdyWriteQueue::RemovePendingWritesForStream(
   }
 #endif
 
+  // Defer deletion until queue iteration is complete, as
+  // SpdyBuffer::~SpdyBuffer() can result in callbacks into SpdyWriteQueue.
+  std::vector<SpdyBufferProducer*> erased_buffer_producers;

[willchan no longer on Chromium, 2014/05/06 16:38:14]

You can also use ScopedVector
(https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...)
if you want.

[Johnny, 2014/05/06 16:54:27]

Leaving as-is I think, to preserve explicit control & documentation on when
callbacks are invoked.

+
   // Do the actual deletion and removal, preserving FIFO-ness.
   std::deque<PendingWrite>* queue = &queue_[priority];
   std::deque<PendingWrite>::iterator out_it = queue->begin();
   for (std::deque<PendingWrite>::const_iterator it = queue->begin();
        it != queue->end(); ++it) {
     if (it->stream.get() == stream.get()) {
-      delete it->frame_producer;
+      erased_buffer_producers.push_back(it->frame_producer);
     } else {
       *out_it = *it;
       ++out_it;
@@ -107,12 +113,15 @@ void SpdyWriteQueue::RemovePendingWritesForStream(
   }
   queue->erase(out_it, queue->end());
   removing_writes_ = false;
+  STLDeleteElements(&erased_buffer_producers);  // Invokes callbacks.
 }
 
 void SpdyWriteQueue::RemovePendingWritesForStreamsAfter(
     SpdyStreamId last_good_stream_id) {
   CHECK(!removing_writes_);
   removing_writes_ = true;
+  std::vector<SpdyBufferProducer*> erased_buffer_producers;
+
   for (int i = MINIMUM_PRIORITY; i <= MAXIMUM_PRIORITY; ++i) {
     // Do the actual deletion and removal, preserving FIFO-ness.
     std::deque<PendingWrite>* queue = &queue_[i];
@@ -121,7 +130,7 @@ void SpdyWriteQueue::RemovePendingWritesForStreamsAfter(
          it != queue->end(); ++it) {
       if (it->stream.get() && (it->stream->stream_id() > last_good_stream_id ||
                                it->stream->stream_id() == 0)) {
-        delete it->frame_producer;
+        erased_buffer_producers.push_back(it->frame_producer);
       } else {
         *out_it = *it;
         ++out_it;
@@ -130,19 +139,23 @@ void SpdyWriteQueue::RemovePendingWritesForStreamsAfter(
     queue->erase(out_it, queue->end());
   }
   removing_writes_ = false;
+  STLDeleteElements(&erased_buffer_producers);  // Invokes callbacks.
 }
 
 void SpdyWriteQueue::Clear() {
   CHECK(!removing_writes_);
   removing_writes_ = true;
+  std::vector<SpdyBufferProducer*> erased_buffer_producers;
+
   for (int i = MINIMUM_PRIORITY; i <= MAXIMUM_PRIORITY; ++i) {
     for (std::deque<PendingWrite>::iterator it = queue_[i].begin();
          it != queue_[i].end(); ++it) {
-      delete it->frame_producer;
+      erased_buffer_producers.push_back(it->frame_producer);
     }
     queue_[i].clear();
   }
   removing_writes_ = false;
+  STLDeleteElements(&erased_buffer_producers);  // Invokes callbacks.
 }
 
 }  // namespace net