Deprecate remove_rewritelength flag.

This cl merges server change 154325175 by yasong.

BUG=488484

Review-Url: https://codereview.chromium.org/2861893002
Cr-Commit-Position: refs/heads/master@{#469463}

net/spdy/chromium/spdy_flags.cc

@@ -9,9 +9,6 @@ namespace net {
 // Log compressed size of HTTP/2 requests.
 bool FLAGS_chromium_http2_flag_log_compressed_size = true;
 
-// If true, remove use of SpdyFrameBuilder::OverwriteLength().
-bool FLAGS_chromium_http2_flag_remove_rewritelength = true;
-
 // Use //net/http2/hpack/decoder as complete HPACK decoder.
 bool FLAGS_chromium_http2_flag_spdy_use_hpack_decoder3 = true;
 

net/spdy/chromium/spdy_flags.h

@@ -10,7 +10,6 @@
 namespace net {
 
 NET_EXPORT_PRIVATE extern bool FLAGS_chromium_http2_flag_log_compressed_size;
-NET_EXPORT_PRIVATE extern bool FLAGS_chromium_http2_flag_remove_rewritelength;
 NET_EXPORT_PRIVATE extern bool
     FLAGS_chromium_http2_flag_spdy_use_hpack_decoder3;
 NET_EXPORT_PRIVATE extern bool FLAGS_use_nested_spdy_framer_decoder;

net/spdy/core/spdy_frame_builder.cc

@@ -72,52 +72,13 @@ bool SpdyFrameBuilder::BeginNewFrame(const SpdyFramer& framer,
   DCHECK_EQ(0u, stream_id & ~kStreamIdMask);
   bool success = true;
   if (length_ > 0) {
-    // Update length field for previous frame.
-    OverwriteLength(framer, length_ - kFrameHeaderSize);
-    SPDY_BUG_IF(framer.GetFrameMaximumSize() < length_)
-        << "Frame length  " << length_
-        << " is longer than the maximum allowed length.";
+    SPDY_BUG << "SpdyFrameBuilder doesn't have a clean state when BeginNewFrame"
+             << "is called. Leftover length_ is " << length_;
+    offset_ += length_;
+    length_ = 0;
   }
 
-  offset_ += length_;
-  length_ = 0;
-
-  // TODO(yasong): remove after OverwriteLength() is deleted.
-  bool length_written = false;
-  // Remember where the length field is written. Used for OverwriteLength().
-  if (output_ != nullptr && CanWrite(kLengthFieldLength)) {
-    // Can write the length field.
-    char* dest = nullptr;
-    // |size| is the available bytes in the current memory block.
-    int size = 0;
-    output_->Next(&dest, &size);
-    start_of_current_frame_ = dest;
-    bytes_of_length_written_in_first_block_ =
-        size > (int)kLengthFieldLength ? kLengthFieldLength : size;
-    // If the current block is not enough for the length field, write the
-    // length field here, and remember the pointer to the next block.
-    if (size < (int)kLengthFieldLength) {
-      // Write the first portion of the length field.
-      int value = base::HostToNet32(capacity_ - offset_ - kFrameHeaderSize);
-      memcpy(dest, reinterpret_cast<char*>(&value) + 1, size);
-      Seek(size);
-      output_->Next(&dest, &size);
-      start_of_current_frame_in_next_block_ = dest;
-      int size_left =
-          kLengthFieldLength - bytes_of_length_written_in_first_block_;
-      memcpy(dest, reinterpret_cast<char*>(&value) + 1 + size, size_left);
-      Seek(size_left);
-      length_written = true;
-    }
-  }
-
-  // Assume all remaining capacity will be used for this frame. If not,
-  // the length will get overwritten when we begin the next frame.
-  // Don't check for length limits here because this may be larger than the
-  // actual frame length.
-  if (!length_written) {
-    success &= WriteUInt24(capacity_ - offset_ - kFrameHeaderSize);
-  }
+  success &= WriteUInt24(capacity_ - offset_ - kFrameHeaderSize);
   success &= WriteUInt8(raw_frame_type);
   success &= WriteUInt8(flags);
   success &= WriteUInt32(stream_id);
@@ -220,43 +181,6 @@ bool SpdyFrameBuilder::WriteBytes(const void* data, uint32_t data_len) {
   return true;
 }
 
-bool SpdyFrameBuilder::OverwriteLength(const SpdyFramer& framer,
-                                       size_t length) {
-  if (output_ != nullptr) {
-    size_t value = base::HostToNet32(length);
-    if (start_of_current_frame_ != nullptr &&
-        bytes_of_length_written_in_first_block_ == kLengthFieldLength) {
-      // Length field of the current frame is within one memory block.
-      memcpy(start_of_current_frame_, reinterpret_cast<char*>(&value) + 1,
-             kLengthFieldLength);
-      return true;
-    } else if (start_of_current_frame_ != nullptr &&
-               start_of_current_frame_in_next_block_ != nullptr &&
-               bytes_of_length_written_in_first_block_ < kLengthFieldLength) {
-      // Length field of the current frame crosses two memory blocks.
-      memcpy(start_of_current_frame_, reinterpret_cast<char*>(&value) + 1,
-             bytes_of_length_written_in_first_block_);
-      memcpy(start_of_current_frame_in_next_block_,
-             reinterpret_cast<char*>(&value) + 1 +
-                 bytes_of_length_written_in_first_block_,
-             kLengthFieldLength - bytes_of_length_written_in_first_block_);
-      return true;
-    } else {
-      return false;
-    }
-  }
-
-  DCHECK_GE(framer.GetFrameMaximumSize(), length);
-  bool success = false;
-  const size_t old_length = length_;
-
-  length_ = 0;
-  success = WriteUInt24(length);
-
-  length_ = old_length;
-  return success;
-}
-
 bool SpdyFrameBuilder::CanWrite(size_t length) const {
   if (length > kLengthMask) {
     DCHECK(false);

net/spdy/core/spdy_frame_builder.h

@@ -110,13 +110,6 @@ class SPDY_EXPORT_PRIVATE SpdyFrameBuilder {
   bool WriteStringPiece32(const SpdyStringPiece& value);
   bool WriteBytes(const void* data, uint32_t data_len);
 
-  // Update (in-place) the length field in the frame being built to reflect the
-  // given length.
-  // The framer parameter is used to determine version-specific location and
-  // size information of the length field to be written, and must be initialized
-  // with the correct version for the frame being written.
-  bool OverwriteLength(const SpdyFramer& framer, size_t length);
-
  private:
   FRIEND_TEST_ALL_PREFIXES(SpdyFrameBuilderTest, GetWritableBuffer);
   FRIEND_TEST_ALL_PREFIXES(SpdyFrameBuilderTest, GetWritableOutput);
@@ -153,14 +146,6 @@ class SPDY_EXPORT_PRIVATE SpdyFrameBuilder {
   size_t capacity_;  // Allocation size of payload, set by constructor.
   size_t length_;    // Length of the latest frame in the buffer.
   size_t offset_;    // Position at which the latest frame begins.
-
-  // Remove all four below after
-  // FLAGS_chromium_http2_flag_remove_rewritelength deprecates.
-  const size_t kLengthFieldLength = 3;
-  char* start_of_current_frame_ = nullptr;
-  size_t bytes_of_length_written_in_first_block_ = kLengthFieldLength;
-  // In case length of a new frame is cross blocks.
-  char* start_of_current_frame_in_next_block_ = nullptr;
 };
 
 }  // namespace net

net/spdy/core/spdy_framer.cc

@@ -143,7 +143,6 @@ SpdyFramer::SpdyFramer(SpdyFramer::DecoderAdapterFactoryFn adapter_factory,
   if (adapter_factory != nullptr) {
     decoder_adapter_ = adapter_factory(this);
   }
-  skip_rewritelength_ = FLAGS_chromium_http2_flag_remove_rewritelength;
 }
 
 SpdyFramer::SpdyFramer(CompressionOption option)
@@ -1877,22 +1876,11 @@ SpdySerializedFrame SpdyFramer::SerializeDataFrameHeaderWithPaddingLengthField(
       data_ir, &flags, &frame_size, &num_padding_fields);
 
   SpdyFrameBuilder builder(frame_size);
-  if (!skip_rewritelength_) {
-    builder.BeginNewFrame(*this, SpdyFrameType::DATA, flags,
-                          data_ir.stream_id());
-    if (data_ir.padded()) {
-      builder.WriteUInt8(data_ir.padding_payload_len() & 0xff);
-    }
-    builder.OverwriteLength(*this, num_padding_fields + data_ir.data_len() +
-                                       data_ir.padding_payload_len());
-  } else {
-    builder.BeginNewFrame(*this, SpdyFrameType::DATA, flags,
-                          data_ir.stream_id(),
-                          num_padding_fields + data_ir.data_len() +
-                              data_ir.padding_payload_len());
-    if (data_ir.padded()) {
-      builder.WriteUInt8(data_ir.padding_payload_len() & 0xff);
-    }
+  builder.BeginNewFrame(
+      *this, SpdyFrameType::DATA, flags, data_ir.stream_id(),
+      num_padding_fields + data_ir.data_len() + data_ir.padding_payload_len());
+  if (data_ir.padded()) {
+    builder.WriteUInt8(data_ir.padding_payload_len() & 0xff);
   }
   DCHECK_EQ(frame_size, builder.length());
   return builder.take();
@@ -2054,14 +2042,9 @@ SpdySerializedFrame SpdyFramer::SerializeHeaders(const SpdyHeadersIR& headers) {
                                 &weight, &length_field);
 
   SpdyFrameBuilder builder(size);
+  builder.BeginNewFrame(*this, SpdyFrameType::HEADERS, flags,
+                        headers.stream_id(), length_field);
 
-  if (!skip_rewritelength_) {
-    builder.BeginNewFrame(*this, SpdyFrameType::HEADERS, flags,
-                          headers.stream_id());
-  } else {
-    builder.BeginNewFrame(*this, SpdyFrameType::HEADERS, flags,
-                          headers.stream_id(), length_field);
-  }
   DCHECK_EQ(GetHeadersMinimumSize(), builder.length());
 
   int padding_payload_len = 0;
@@ -2137,14 +2120,9 @@ SpdySerializedFrame SpdyFramer::SerializePushPromise(
                                     &size);
 
   SpdyFrameBuilder builder(size);
-  if (!skip_rewritelength_) {
-    builder.BeginNewFrame(*this, SpdyFrameType::PUSH_PROMISE, flags,
-                          push_promise.stream_id());
-  } else {
-    size_t length = std::min(size, kMaxControlFrameSize) - GetFrameHeaderSize();
-    builder.BeginNewFrame(*this, SpdyFrameType::PUSH_PROMISE, flags,
-                          push_promise.stream_id(), length);
-  }
+  size_t length = std::min(size, kMaxControlFrameSize) - GetFrameHeaderSize();
+  builder.BeginNewFrame(*this, SpdyFrameType::PUSH_PROMISE, flags,
+                        push_promise.stream_id(), length);
   int padding_payload_len = 0;
   if (push_promise.padded()) {
     builder.WriteUInt8(push_promise.padding_payload_len());
@@ -2448,23 +2426,12 @@ bool SpdyFramer::SerializeDataFrameHeaderWithPaddingLengthField(
 
   SpdyFrameBuilder builder(frame_size, output);
   bool ok = true;
-  if (!skip_rewritelength_) {
-    ok = builder.BeginNewFrame(*this, SpdyFrameType::DATA, flags,
-                               data_ir.stream_id());
-    if (data_ir.padded()) {
-      ok = ok && builder.WriteUInt8(data_ir.padding_payload_len() & 0xff);
-    }
-    ok = ok && builder.OverwriteLength(*this,
-                                       num_padding_fields + data_ir.data_len() +
-                                           data_ir.padding_payload_len());
-  } else {
-    ok = ok && builder.BeginNewFrame(*this, SpdyFrameType::DATA, flags,
-                                     data_ir.stream_id(),
-                                     num_padding_fields + data_ir.data_len() +
-                                         data_ir.padding_payload_len());
-    if (data_ir.padded()) {
-      ok = ok && builder.WriteUInt8(data_ir.padding_payload_len() & 0xff);
-    }
+  ok = ok && builder.BeginNewFrame(*this, SpdyFrameType::DATA, flags,
+                                   data_ir.stream_id(),
+                                   num_padding_fields + data_ir.data_len() +
+                                       data_ir.padding_payload_len());
+  if (data_ir.padded()) {
+    ok = ok && builder.WriteUInt8(data_ir.padding_payload_len() & 0xff);
   }
   DCHECK_EQ(frame_size, builder.length());
   return ok;
@@ -2561,13 +2528,8 @@ bool SpdyFramer::SerializeHeaders(const SpdyHeadersIR& headers,
 
   bool ok = true;
   SpdyFrameBuilder builder(size, output);
-  if (!skip_rewritelength_) {
-    ok = builder.BeginNewFrame(*this, SpdyFrameType::HEADERS, flags,
-                               headers.stream_id());
-  } else {
-    ok = ok && builder.BeginNewFrame(*this, SpdyFrameType::HEADERS, flags,
-                                     headers.stream_id(), length_field);
-  }
+  ok = ok && builder.BeginNewFrame(*this, SpdyFrameType::HEADERS, flags,
+                                   headers.stream_id(), length_field);
   DCHECK_EQ(GetHeadersMinimumSize(), builder.length());
 
   int padding_payload_len = 0;
@@ -2619,14 +2581,9 @@ bool SpdyFramer::SerializePushPromise(const SpdyPushPromiseIR& push_promise,
 
   bool ok = true;
   SpdyFrameBuilder builder(size, output);
-  if (!skip_rewritelength_) {
-    ok = builder.BeginNewFrame(*this, SpdyFrameType::PUSH_PROMISE, flags,
-                               push_promise.stream_id());
-  } else {
-    size_t length = std::min(size, kMaxControlFrameSize) - GetFrameHeaderSize();
-    ok = builder.BeginNewFrame(*this, SpdyFrameType::PUSH_PROMISE, flags,
-                               push_promise.stream_id(), length);
-  }
+  size_t length = std::min(size, kMaxControlFrameSize) - GetFrameHeaderSize();
+  ok = builder.BeginNewFrame(*this, SpdyFrameType::PUSH_PROMISE, flags,
+                             push_promise.stream_id(), length);
 
   int padding_payload_len = 0;
   if (push_promise.padded()) {
@@ -2892,10 +2849,6 @@ bool SpdyFramer::WritePayloadWithContinuation(SpdyFrameBuilder* builder,
     SpdyString padding = SpdyString(padding_payload_len, 0);
     ret &= builder->WriteBytes(padding.data(), padding.length());
   }
-  if (bytes_remaining > 0 && !skip_rewritelength_) {
-    ret &= builder->OverwriteLength(
-        *this, kMaxControlFrameSize - GetFrameHeaderSize());
-  }
 
   // Tack on CONTINUATION frames for the overflow.
   while (bytes_remaining > 0 && ret) {
@@ -2905,13 +2858,8 @@ bool SpdyFramer::WritePayloadWithContinuation(SpdyFrameBuilder* builder,
     if (bytes_remaining == bytes_to_write) {
       flags |= end_flag;
     }
-    if (!skip_rewritelength_) {
-      ret &= builder->BeginNewFrame(*this, SpdyFrameType::CONTINUATION, flags,
-                                    stream_id);
-    } else {
-      ret &= builder->BeginNewFrame(*this, SpdyFrameType::CONTINUATION, flags,
-                                    stream_id, bytes_to_write);
-    }
+    ret &= builder->BeginNewFrame(*this, SpdyFrameType::CONTINUATION, flags,
+                                  stream_id, bytes_to_write);
     // Write payload fragment.
     ret &= builder->WriteBytes(
         &hpack_encoding[hpack_encoding.size() - bytes_remaining],

net/spdy/core/spdy_framer.h

@@ -979,9 +979,6 @@ class SPDY_EXPORT_PRIVATE SpdyFramer {
   // If true, then ProcessInput returns after processing a full frame,
   // rather than reading all available input.
   bool process_single_input_frame_ = false;
-
-  // Latched value of FLAGS_chromium_http2_flag_remove_rewritelength.
-  bool skip_rewritelength_ = false;
 };
 
 }  // namespace net

net/spdy/core/spdy_framer_test.cc

@@ -866,7 +866,6 @@ TEST_P(SpdyFramerTest, RejectUpperCaseHeaderBlockValue) {
   frame.WriteUInt32(1);
   frame.WriteStringPiece32("Name1");
   frame.WriteStringPiece32("value1");
-  frame.OverwriteLength(framer, frame.length() - framer.GetFrameHeaderSize());
 
   SpdyFrameBuilder frame2(1024);
   frame2.BeginNewFrame(framer, SpdyFrameType::HEADERS, 0, 1);
@@ -875,7 +874,6 @@ TEST_P(SpdyFramerTest, RejectUpperCaseHeaderBlockValue) {
   frame2.WriteStringPiece32("value1");
   frame2.WriteStringPiece32("nAmE2");
   frame2.WriteStringPiece32("value2");
-  frame.OverwriteLength(framer, frame2.length() - framer.GetFrameHeaderSize());
 
   SpdySerializedFrame control_frame(frame.take());
   SpdyStringPiece serialized_headers =
@@ -1312,8 +1310,6 @@ TEST_P(SpdyFramerTest, DuplicateHeader) {
   frame.WriteStringPiece32("value1");
   frame.WriteStringPiece32("name");
   frame.WriteStringPiece32("value2");
-  // write the length
-  frame.OverwriteLength(framer, frame.length() - framer.GetFrameHeaderSize());
 
   SpdyHeaderBlock new_headers;
   SpdySerializedFrame control_frame(frame.take());
@@ -1326,13 +1322,6 @@ TEST_P(SpdyFramerTest, DuplicateHeader) {
 
 TEST_P(SpdyFramerTest, MultiValueHeader) {
   SpdyFramer framer(SpdyFramer::DISABLE_COMPRESSION);
-  // Frame builder with plentiful buffer size.
-  SpdyFrameBuilder frame(1024);
-  frame.BeginNewFrame(framer, SpdyFrameType::HEADERS,
-                      HEADERS_FLAG_PRIORITY | HEADERS_FLAG_END_HEADERS, 3);
-  frame.WriteUInt32(0);   // Priority exclusivity and dependent stream.
-  frame.WriteUInt8(255);  // Priority weight.
-
   SpdyString value("value1\0value2", 13);
   // TODO(jgraettinger): If this pattern appears again, move to test class.
   SpdyHeaderBlock header_set;
@@ -1341,9 +1330,14 @@ TEST_P(SpdyFramerTest, MultiValueHeader) {
   HpackEncoder encoder(ObtainHpackHuffmanTable());
   encoder.DisableCompression();
   encoder.EncodeHeaderSet(header_set, &buffer);
+  // Frame builder with plentiful buffer size.
+  SpdyFrameBuilder frame(1024);
+  frame.BeginNewFrame(framer, SpdyFrameType::HEADERS,
+                      HEADERS_FLAG_PRIORITY | HEADERS_FLAG_END_HEADERS, 3,
+                      buffer.size() + 5 /* priority */);
+  frame.WriteUInt32(0);   // Priority exclusivity and dependent stream.
+  frame.WriteUInt8(255);  // Priority weight.
   frame.WriteBytes(&buffer[0], buffer.size());
-  // write the length
-  frame.OverwriteLength(framer, frame.length() - framer.GetFrameHeaderSize());
 
   SpdySerializedFrame control_frame(frame.take());