HttpServer: adjust QueuedWriteIOBuffer to new queue

In particular, don't assume pointer stability.


Bug: 764320
Change-Id: Ie260aa1cef4a42869ae2ab46601d9e1143f1d5d8
Reviewed-on: https://chromium-review.googlesource.com/667730
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502385}

net/server/http_connection.cc

@@ -115,12 +115,12 @@ bool HttpConnection::QueuedWriteIOBuffer::Append(const std::string& data) {
     return false;
   }
 
-  pending_data_.push(data);
+  pending_data_.push(std::make_unique<std::string>(data));
   total_size_ += data.size();
 
   // If new data is the first pending data, updates data_.
   if (pending_data_.size() == 1)
-    data_ = const_cast<char*>(pending_data_.front().data());
+    data_ = const_cast<char*>(pending_data_.front()->data());
   return true;
 }
 
@@ -134,7 +134,7 @@ void HttpConnection::QueuedWriteIOBuffer::DidConsume(int size) {
     data_ += size;
   } else {  // size == GetSizeToWrite(). Updates data_ to next pending data.
     pending_data_.pop();
-    data_ = IsEmpty() ? NULL : const_cast<char*>(pending_data_.front().data());
+    data_ = IsEmpty() ? NULL : const_cast<char*>(pending_data_.front()->data());
   }
   total_size_ -= size;
 }
@@ -144,10 +144,10 @@ int HttpConnection::QueuedWriteIOBuffer::GetSizeToWrite() const {
     DCHECK_EQ(0, total_size_);
     return 0;
   }
-  DCHECK_GE(data_, pending_data_.front().data());
-  int consumed = static_cast<int>(data_ - pending_data_.front().data());
-  DCHECK_GT(static_cast<int>(pending_data_.front().size()), consumed);
-  return pending_data_.front().size() - consumed;
+  DCHECK_GE(data_, pending_data_.front()->data());
+  int consumed = static_cast<int>(data_ - pending_data_.front()->data());
+  DCHECK_GT(static_cast<int>(pending_data_.front()->size()), consumed);
+  return pending_data_.front()->size() - consumed;
 }
 
 HttpConnection::HttpConnection(int id, std::unique_ptr<StreamSocket> socket)

net/server/http_connection.h

@@ -103,7 +103,9 @@ class HttpConnection {
    private:
     ~QueuedWriteIOBuffer() override;
 
-    base::queue<std::string> pending_data_;
+    // This needs to indirect since we need pointer stability for the payload
+    // chunks, as they may be handed out via net::IOBuffer::data().
+    base::queue<std::unique_ptr<std::string>> pending_data_;
     int total_size_;
     int max_buffer_size_;
 

net/server/http_connection_unittest.cc

@@ -327,5 +327,36 @@ TEST(HttpConnectionTest, QueuedWriteIOBuffer_TotalSizeLimit) {
   EXPECT_EQ(kDataLength * 4 - kConsumedLength, buffer->total_size());
 }
 
+TEST(HttpConnectionTest, QueuedWriteIOBuffer_DataPointerStability) {
+  // This is a regression test that makes sure that QueuedWriteIOBuffer deals
+  // with base::queue's semantics differences vs. std::queue right, and still
+  // makes sure our data() pointers are stable.
+  scoped_refptr<HttpConnection::QueuedWriteIOBuffer> buffer(
+      new HttpConnection::QueuedWriteIOBuffer());
+
+  // We append a short string to make it fit within any short string
+  // optimization, so that if the underlying queue moves the std::string,
+  // the data should change.
+  buffer->Append("abcdefgh");
+
+  // Read part of it, to make sure this handles the case of data() pointing
+  // to something other than start of string right.
+  buffer->DidConsume(3);
+  const char* old_data = buffer->data();
+  EXPECT_EQ("defgh", base::StringPiece(buffer->data(), 5));
+
+  // Now append a whole bunch of other things to make the underlying queue
+  // grow, and likely need to move stuff around in memory.
+  for (int i = 0; i < 256; ++i)
+    buffer->Append("some other string data");
+
+  // data() should still be right.
+  EXPECT_EQ("defgh", base::StringPiece(buffer->data(), 5));
+
+  // ... it should also be bitwise the same, since the IOBuffer can get passed
+  // to async calls and then have Append's come in.
+  EXPECT_TRUE(buffer->data() == old_data);
+}
+
 }  // namespace
 }  // namespace net