Processing for new SPDY4/HTTP2 SETTINGS frame payload format.

This lands server change 62089770 by mlavan.

BUG=345769

Review URL: https://codereview.chromium.org/183743007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@254233 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 195 insertions, 101 deletions

diff --git a/net/spdy/spdy_framer.cc b/net/spdy/spdy_framer.cc
index 62f5f16d0..a6b3961 100644
--- a/net/spdy/spdy_framer.cc
+++ b/net/spdy/spdy_framer.cc
@@ -748,9 +748,11 @@ void SpdyFramer::ProcessControlFrameHeader(uint16 control_frame_type_field) {
       // Make sure that we have an integral number of 8-byte key/value pairs,
       // plus a 4-byte length field in SPDY3 and below.
       size_t values_prefix_size = (protocol_version() < 4 ? 4 : 0);
+      // Size of each key/value pair in bytes.
+      size_t setting_size = (protocol_version() < 4 ? 8 : 5);
       if (current_frame_length_ < GetSettingsMinimumSize() ||
-          (current_frame_length_ - GetControlFrameHeaderSize()) % 8 !=
-           values_prefix_size) {
+          (current_frame_length_ - GetControlFrameHeaderSize())
+          % setting_size != values_prefix_size) {
         DLOG(WARNING) << "Invalid length for SETTINGS frame: "
                       << current_frame_length_;
         set_error(SPDY_INVALID_CONTROL_FRAME);
@@ -1367,15 +1369,17 @@ size_t SpdyFramer::ProcessSettingsFramePayload(const char* data,
   size_t unprocessed_bytes = std::min(data_len, remaining_data_length_);
   size_t processed_bytes = 0;
 
+  size_t setting_size = spdy_version_ < 4 ? 8 : 5;
+
   // Loop over our incoming data.
   while (unprocessed_bytes > 0) {
     // Process up to one setting at a time.
     size_t processing = std::min(
         unprocessed_bytes,
-        static_cast<size_t>(8 - settings_scratch_.setting_buf_len));
+        static_cast<size_t>(setting_size - settings_scratch_.setting_buf_len));
 
     // Check if we have a complete setting in our input.
-    if (processing == 8) {
+    if (processing == setting_size) {
       // Parse the setting directly out of the input without buffering.
       if (!ProcessSetting(data + processed_bytes)) {
         set_error(SPDY_INVALID_CONTROL_FRAME);
@@ -1389,7 +1393,7 @@ size_t SpdyFramer::ProcessSettingsFramePayload(const char* data,
       settings_scratch_.setting_buf_len += processing;
 
       // Check if we have a complete setting buffered.
-      if (settings_scratch_.setting_buf_len == 8) {
+      if (settings_scratch_.setting_buf_len == setting_size) {
         if (!ProcessSetting(settings_scratch_.setting_buf)) {
           set_error(SPDY_INVALID_CONTROL_FRAME);
           return processed_bytes;
@@ -1415,16 +1419,26 @@ size_t SpdyFramer::ProcessSettingsFramePayload(const char* data,
 }
 
 bool SpdyFramer::ProcessSetting(const char* data) {
+  SpdySettingsIds id;
+  uint8 flags = 0;
+  uint32 value;
+
   // Extract fields.
   // Maintain behavior of old SPDY 2 bug with byte ordering of flags/id.
-  const uint32 id_and_flags_wire = *(reinterpret_cast<const uint32*>(data));
-  SettingsFlagsAndId id_and_flags =
+  if (spdy_version_ < 4) {
+    const uint32 id_and_flags_wire = *(reinterpret_cast<const uint32*>(data));
+    SettingsFlagsAndId id_and_flags =
       SettingsFlagsAndId::FromWireFormat(spdy_version_, id_and_flags_wire);
-  uint8 flags = id_and_flags.flags();
-  uint32 value = ntohl(*(reinterpret_cast<const uint32*>(data + 4)));
+    id = static_cast<SpdySettingsIds>(id_and_flags.id());
+    flags = id_and_flags.flags();
+    value = ntohl(*(reinterpret_cast<const uint32*>(data + 4)));
+  } else {
+    id = static_cast<SpdySettingsIds>(*(reinterpret_cast<const uint8*>(data)));
+    value = ntohl(*(reinterpret_cast<const uint32*>(data + 1)));
+  }
 
   // Validate id.
-  switch (id_and_flags.id()) {
+  switch (id) {
     case SETTINGS_UPLOAD_BANDWIDTH:
     case SETTINGS_DOWNLOAD_BANDWIDTH:
     case SETTINGS_ROUND_TRIP_TIME:
@@ -1435,27 +1449,28 @@ bool SpdyFramer::ProcessSetting(const char* data) {
       // Valid values.
       break;
     default:
-      DLOG(WARNING) << "Unknown SETTINGS ID: " << id_and_flags.id();
+      DLOG(WARNING) << "Unknown SETTINGS ID: " << id;
       return false;
   }
-  SpdySettingsIds id = static_cast<SpdySettingsIds>(id_and_flags.id());
 
-  // Detect duplicates.
-  if (static_cast<uint32>(id) <= settings_scratch_.last_setting_id) {
-    DLOG(WARNING) << "Duplicate entry or invalid ordering for id " << id
-                  << " in " << display_protocol_ << " SETTINGS frame "
-                  << "(last settikng id was "
-                  << settings_scratch_.last_setting_id << ").";
-    return false;
-  }
-  settings_scratch_.last_setting_id = id;
+  if (spdy_version_ < 4) {
+    // Detect duplicates.
+    if (static_cast<uint32>(id) <= settings_scratch_.last_setting_id) {
+      DLOG(WARNING) << "Duplicate entry or invalid ordering for id " << id
+                    << " in " << display_protocol_ << " SETTINGS frame "
+                    << "(last setting id was "
+                    << settings_scratch_.last_setting_id << ").";
+      return false;
+    }
+    settings_scratch_.last_setting_id = id;
 
-  // Validate flags.
-  uint8 kFlagsMask = SETTINGS_FLAG_PLEASE_PERSIST | SETTINGS_FLAG_PERSISTED;
-  if ((flags & ~(kFlagsMask)) != 0) {
-    DLOG(WARNING) << "Unknown SETTINGS flags provided for id " << id << ": "
-                  << flags;
-    return false;
+    // Validate flags.
+    uint8 kFlagsMask = SETTINGS_FLAG_PLEASE_PERSIST | SETTINGS_FLAG_PERSISTED;
+    if ((flags & ~(kFlagsMask)) != 0) {
+      DLOG(WARNING) << "Unknown SETTINGS flags provided for id " << id << ": "
+                    << flags;
+      return false;
+    }
   }
 
   // Validation succeeded. Pass on to visitor.
@@ -1931,17 +1946,22 @@ SpdySerializedFrame* SpdyFramer::SerializeRstStream(
 SpdySerializedFrame* SpdyFramer::SerializeSettings(
     const SpdySettingsIR& settings) const {
   uint8 flags = 0;
-  if (spdy_version_ < 4 && settings.clear_settings()) {
-    flags |= SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS;
-  }
-  if (spdy_version_ >= 4 && settings.is_ack()) {
-    flags |= SETTINGS_FLAG_ACK;
+
+  if (spdy_version_ < 4) {
+    if (settings.clear_settings()) {
+      flags |= SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS;
+    }
+  } else {
+    if (settings.is_ack()) {
+      flags |= SETTINGS_FLAG_ACK;
+    }
   }
   const SpdySettingsIR::ValueMap* values = &(settings.values());
 
+  size_t setting_size = (protocol_version() < 4 ? 8 : 5);
   // Size, in bytes, of this SETTINGS frame.
-  const size_t size = GetSettingsMinimumSize() + (values->size() * 8);
-
+  const size_t size = GetSettingsMinimumSize() +
+                      (values->size() * setting_size);
   SpdyFrameBuilder builder(size);
   if (spdy_version_ < 4) {
     builder.WriteControlFrameHeader(*this, SETTINGS, flags);
@@ -1961,16 +1981,20 @@ SpdySerializedFrame* SpdyFramer::SerializeSettings(
   for (SpdySettingsIR::ValueMap::const_iterator it = values->begin();
        it != values->end();
        ++it) {
-    uint8 setting_flags = 0;
-    if (it->second.persist_value) {
-      setting_flags |= SETTINGS_FLAG_PLEASE_PERSIST;
-    }
-    if (it->second.persisted) {
-      setting_flags |= SETTINGS_FLAG_PERSISTED;
+    if (spdy_version_ < 4) {
+      uint8 setting_flags = 0;
+      if (it->second.persist_value) {
+        setting_flags |= SETTINGS_FLAG_PLEASE_PERSIST;
+      }
+      if (it->second.persisted) {
+        setting_flags |= SETTINGS_FLAG_PERSISTED;
+      }
+      SettingsFlagsAndId flags_and_id(setting_flags, it->first);
+      uint32 id_and_flags_wire = flags_and_id.GetWireFormat(protocol_version());
+      builder.WriteBytes(&id_and_flags_wire, 4);
+    } else {
+      builder.WriteUInt8(static_cast<uint8>(it->first));
     }
-    SettingsFlagsAndId flags_and_id(setting_flags, it->first);
-    uint32 id_and_flags_wire = flags_and_id.GetWireFormat(protocol_version());
-    builder.WriteBytes(&id_and_flags_wire, 4);
     builder.WriteUInt32(it->second.value);
   }
   DCHECK_EQ(size, builder.length());
diff --git a/net/spdy/spdy_framer.h b/net/spdy/spdy_framer.h
index ab463c5..04938bf 100644
--- a/net/spdy/spdy_framer.h
+++ b/net/spdy/spdy_framer.h
@@ -527,7 +527,8 @@ class NET_EXPORT_PRIVATE SpdyFramer {
 
   // Helpers for above internal breakouts from ProcessInput.
   void ProcessControlFrameHeader(uint16 control_frame_type_field);
-  bool ProcessSetting(const char* data);  // Always passed exactly 8 bytes.
+  // Always passed exactly 1 setting's worth of data.
+  bool ProcessSetting(const char* data);
 
   // Retrieve serialized length of SpdyHeaderBlock. If compression is enabled, a
   // maximum estimate is returned.
diff --git a/net/spdy/spdy_framer_test.cc b/net/spdy/spdy_framer_test.cc
index 25b0db4..c2fb45f 100644
--- a/net/spdy/spdy_framer_test.cc
+++ b/net/spdy/spdy_framer_test.cc
@@ -2228,16 +2228,6 @@ TEST_P(SpdyFramerTest, CreateSettings) {
   {
     const char kDescription[] = "Network byte order SETTINGS frame";
 
-    uint32 kValue = 0x0a0b0c0d;
-    SpdySettingsFlags kFlags = static_cast<SpdySettingsFlags>(0x01);
-    SpdySettingsIds kId = static_cast<SpdySettingsIds>(0x020304);
-
-    SettingsMap settings;
-    settings[kId] = SettingsFlagsAndValue(kFlags, kValue);
-
-    EXPECT_EQ(kFlags, settings[kId].first);
-    EXPECT_EQ(kValue, settings[kId].second);
-
     const unsigned char kV2FrameData[] = {
       0x80, spdy_version_ch_, 0x00, 0x04,
       0x00, 0x00, 0x00, 0x0c,
@@ -2253,17 +2243,29 @@ TEST_P(SpdyFramerTest, CreateSettings) {
       0x0a, 0x0b, 0x0c, 0x0d,
     };
     const unsigned char kV4FrameData[] = {
-      0x00, 0x10, 0x04, 0x00,
+      0x00, 0x0d, 0x04, 0x00,
       0x00, 0x00, 0x00, 0x00,
-      0x01, 0x02, 0x03, 0x04,
-      0x0a, 0x0b, 0x0c, 0x0d,
+      0x01, 0x0a, 0x0b, 0x0c,
+      0x0d,
     };
 
+    uint32 kValue = 0x0a0b0c0d;
     SpdySettingsIR settings_ir;
+
+    SpdySettingsFlags kFlags = static_cast<SpdySettingsFlags>(0x01);
+    SpdySettingsIds kId = static_cast<SpdySettingsIds>(0x020304);
+    if (IsSpdy4()) {
+      kId = static_cast<SpdySettingsIds>(0x01);
+    }
+    SettingsMap settings;
+    settings[kId] = SettingsFlagsAndValue(kFlags, kValue);
+    EXPECT_EQ(kFlags, settings[kId].first);
+    EXPECT_EQ(kValue, settings[kId].second);
     settings_ir.AddSetting(kId,
                            kFlags & SETTINGS_FLAG_PLEASE_PERSIST,
                            kFlags & SETTINGS_FLAG_PERSISTED,
                            kValue);
+
     scoped_ptr<SpdyFrame> frame(framer.SerializeSettings(settings_ir));
     if (IsSpdy2()) {
       CompareFrame(kDescription, *frame, kV2FrameData, arraysize(kV2FrameData));
@@ -2301,25 +2303,36 @@ TEST_P(SpdyFramerTest, CreateSettings) {
       0xff, 0x00, 0x00, 0x04,
     };
     const unsigned char kV4FrameData[] = {
-      0x00, 0x28, 0x04, 0x00,
+      0x00, 0x1c, 0x04, 0x00,
       0x00, 0x00, 0x00, 0x00,
-      0x00, 0x00, 0x00, 0x00,  // 1st Setting
+      0x01,                    // 1st Setting
       0x00, 0x00, 0x00, 0x01,
-      0x01, 0x00, 0x00, 0x01,  // 2nd Setting
+      0x02,                    // 2nd Setting
       0x00, 0x00, 0x00, 0x02,
-      0x02, 0x00, 0x00, 0x02,  // 3rd Setting
+      0x03,                    // 3rd Setting
       0x00, 0x00, 0x00, 0x03,
-      0x03, 0x00, 0x00, 0x03,  // 4th Setting
+      0x04,                    // 4th Setting
       0xff, 0x00, 0x00, 0x04,
     };
     SpdySettingsIR settings_ir;
-    for (SettingsMap::const_iterator it = settings.begin();
-         it != settings.end();
-         ++it) {
-      settings_ir.AddSetting(it->first,
-                             it->second.first & SETTINGS_FLAG_PLEASE_PERSIST,
-                             it->second.first & SETTINGS_FLAG_PERSISTED,
-                             it->second.second);
+    if (!IsSpdy4()) {
+      for (SettingsMap::const_iterator it = settings.begin();
+           it != settings.end();
+           ++it) {
+        settings_ir.AddSetting(it->first,
+                               it->second.first & SETTINGS_FLAG_PLEASE_PERSIST,
+                               it->second.first & SETTINGS_FLAG_PERSISTED,
+                               it->second.second);
+      }
+    } else {
+      SpdySettingsIds kId = static_cast<SpdySettingsIds>(0x01);
+      settings_ir.AddSetting(kId, 0, 0, 0x00000001);
+      kId = static_cast<SpdySettingsIds>(0x02);
+      settings_ir.AddSetting(kId, 0, 0, 0x00000002);
+      kId = static_cast<SpdySettingsIds>(0x03);
+      settings_ir.AddSetting(kId, 0, 0, 0x00000003);
+      kId = static_cast<SpdySettingsIds>(0x04);
+      settings_ir.AddSetting(kId, 0, 0, 0xff000004);
     }
     scoped_ptr<SpdyFrame> frame(framer.SerializeSettings(settings_ir));
     if (IsSpdy4()) {
@@ -3154,39 +3167,42 @@ TEST_P(SpdyFramerTest, ReadZeroLenSettingsFrame) {
 // Tests handling of SETTINGS frames with invalid length.
 TEST_P(SpdyFramerTest, ReadBogusLenSettingsFrame) {
   SpdyFramer framer(spdy_version_);
-  SettingsMap settings;
+  SpdySettingsIR settings_ir;
+
   // Add a setting to pad the frame so that we don't get a buffer overflow when
   // calling SimulateInFramer() below.
+  SettingsMap settings;
   settings[SETTINGS_UPLOAD_BANDWIDTH] =
       SettingsFlagsAndValue(SETTINGS_FLAG_PLEASE_PERSIST, 0x00000002);
-  SpdySettingsIR settings_ir;
   settings_ir.AddSetting(SETTINGS_UPLOAD_BANDWIDTH,
                          true,  // please persist
                          false,
                          0x00000002);
   scoped_ptr<SpdyFrame> control_frame(framer.SerializeSettings(settings_ir));
-  const size_t kNewLength = 5;
+  const size_t kNewLength = 14;
   SetFrameLength(control_frame.get(), kNewLength, spdy_version_);
   TestSpdyVisitor visitor(spdy_version_);
   visitor.use_compression_ = false;
   visitor.SimulateInFramer(
       reinterpret_cast<unsigned char*>(control_frame->data()),
       framer.GetControlFrameHeaderSize() + kNewLength);
-  // Should generate an error, since zero-len settings frames are unsupported.
+  // Should generate an error, since its not possible to have a
+  // settings frame of length kNewLength.
   EXPECT_EQ(1, visitor.error_count_);
 }
 
 // Tests handling of SETTINGS frames larger than the frame buffer size.
 TEST_P(SpdyFramerTest, ReadLargeSettingsFrame) {
   SpdyFramer framer(spdy_version_);
+  SpdySettingsIR settings_ir;
   SettingsMap settings;
+
   SpdySettingsFlags flags = SETTINGS_FLAG_PLEASE_PERSIST;
   settings[SETTINGS_UPLOAD_BANDWIDTH] =
-      SettingsFlagsAndValue(flags, 0x00000002);
+    SettingsFlagsAndValue(flags, 0x00000002);
   settings[SETTINGS_DOWNLOAD_BANDWIDTH] =
-      SettingsFlagsAndValue(flags, 0x00000003);
+    SettingsFlagsAndValue(flags, 0x00000003);
   settings[SETTINGS_ROUND_TRIP_TIME] = SettingsFlagsAndValue(flags, 0x00000004);
-  SpdySettingsIR settings_ir;
   for (SettingsMap::const_iterator it = settings.begin();
        it != settings.end();
        ++it) {
@@ -3195,6 +3211,7 @@ TEST_P(SpdyFramerTest, ReadLargeSettingsFrame) {
                            it->second.first & SETTINGS_FLAG_PERSISTED,
                            it->second.second);
   }
+
   scoped_ptr<SpdyFrame> control_frame(framer.SerializeSettings(settings_ir));
   EXPECT_LT(SpdyFramer::kControlFrameBufferSize,
             control_frame->size());
@@ -3206,7 +3223,7 @@ TEST_P(SpdyFramerTest, ReadLargeSettingsFrame) {
       reinterpret_cast<unsigned char*>(control_frame->data()),
       control_frame->size());
   EXPECT_EQ(0, visitor.error_count_);
-  EXPECT_EQ(settings.size(), static_cast<unsigned>(visitor.setting_count_));
+  EXPECT_EQ(3, visitor.setting_count_);
   if (spdy_version_ >= 4) {
     EXPECT_EQ(1, visitor.settings_ack_sent_);
   }
@@ -3224,7 +3241,7 @@ TEST_P(SpdyFramerTest, ReadLargeSettingsFrame) {
     framed_data += to_read;
   }
   EXPECT_EQ(0, visitor.error_count_);
-  EXPECT_EQ(settings.size() * 2, static_cast<unsigned>(visitor.setting_count_));
+  EXPECT_EQ(3 * 2, visitor.setting_count_);
   if (spdy_version_ >= 4) {
     EXPECT_EQ(2, visitor.settings_ack_sent_);
   }
@@ -3257,13 +3274,13 @@ TEST_P(SpdyFramerTest, ReadDuplicateSettings) {
     0x00, 0x00, 0x00, 0x03,
   };
   const unsigned char kV4FrameData[] = {
-    0x00, 0x20, 0x04, 0x00,
+    0x00, 0x17, 0x04, 0x00,
     0x00, 0x00, 0x00, 0x00,
-    0x00, 0x00, 0x00, 0x01,  // 1st Setting
+    0x01,  // 1st Setting
     0x00, 0x00, 0x00, 0x02,
-    0x00, 0x00, 0x00, 0x01,  // 2nd (duplicate) Setting
+    0x01,  // 2nd (duplicate) Setting
     0x00, 0x00, 0x00, 0x03,
-    0x00, 0x00, 0x00, 0x03,  // 3rd (unprocessed) Setting
+    0x03,  // 3rd (unprocessed) Setting
     0x00, 0x00, 0x00, 0x03,
   };
 
@@ -3277,8 +3294,16 @@ TEST_P(SpdyFramerTest, ReadDuplicateSettings) {
     visitor.SimulateInFramer(kV4FrameData, sizeof(kV4FrameData));
   }
 
-  EXPECT_EQ(1, visitor.setting_count_);
-  EXPECT_EQ(1, visitor.error_count_);
+  if (!IsSpdy4()) {
+    EXPECT_EQ(1, visitor.setting_count_);
+    EXPECT_EQ(1, visitor.error_count_);
+  } else {
+    // In SPDY 4+, duplicate settings are allowed;
+    // each setting replaces the previous value for that setting.
+    EXPECT_EQ(3, visitor.setting_count_);
+    EXPECT_EQ(0, visitor.error_count_);
+    EXPECT_EQ(1, visitor.settings_ack_sent_);
+  }
 }
 
 // Tests handling of SETTINGS frame with entries out of order.
@@ -3308,13 +3333,13 @@ TEST_P(SpdyFramerTest, ReadOutOfOrderSettings) {
     0x00, 0x00, 0x00, 0x03,
   };
   const unsigned char kV4FrameData[] = {
-    0x00, 0x20, 0x04, 0x00,
+    0x00, 0x17, 0x04, 0x00,
     0x00, 0x00, 0x00, 0x00,
-    0x00, 0x00, 0x00, 0x02,  // 1st Setting
+    0x02,  // 1st Setting
     0x00, 0x00, 0x00, 0x02,
-    0x00, 0x00, 0x00, 0x01,  // 2nd (out of order) Setting
+    0x01,  // 2nd (out of order) Setting
     0x00, 0x00, 0x00, 0x03,
-    0x00, 0x00, 0x01, 0x03,  // 3rd (unprocessed) Setting
+    0x03,  // 3rd (unprocessed) Setting
     0x00, 0x00, 0x00, 0x03,
   };
 
@@ -3327,8 +3352,16 @@ TEST_P(SpdyFramerTest, ReadOutOfOrderSettings) {
   } else {
     visitor.SimulateInFramer(kV4FrameData, sizeof(kV4FrameData));
   }
-  EXPECT_EQ(1, visitor.error_count_);
-  EXPECT_EQ(1, visitor.setting_count_);
+
+  if (!IsSpdy4()) {
+    EXPECT_EQ(1, visitor.setting_count_);
+    EXPECT_EQ(1, visitor.error_count_);
+  } else {
+    // In SPDY 4+, settings are allowed in any order.
+    EXPECT_EQ(3, visitor.setting_count_);
+    EXPECT_EQ(0, visitor.error_count_);
+    // EXPECT_EQ(1, visitor.settings_ack_count_);
+  }
 }
 
 TEST_P(SpdyFramerTest, ProcessSettingsAckFrame) {
@@ -3830,7 +3863,8 @@ TEST_P(SpdyFramerTest, RstStreamFrameFlags) {
   }
 }
 
-TEST_P(SpdyFramerTest, SettingsFrameFlags) {
+TEST_P(SpdyFramerTest, SettingsFrameFlagsOldFormat) {
+  if (spdy_version_ >= 4) { return; }
   for (int flags = 0; flags < 256; ++flags) {
     SCOPED_TRACE(testing::Message() << "Flags " << flags);
 
@@ -3846,11 +3880,7 @@ TEST_P(SpdyFramerTest, SettingsFrameFlags) {
     scoped_ptr<SpdyFrame> frame(framer.SerializeSettings(settings_ir));
     SetFrameFlags(frame.get(), flags, spdy_version_);
 
-    if ((!IsSpdy4() &&
-         flags & ~SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS) ||
-        (IsSpdy4() && flags & ~SETTINGS_FLAG_ACK)) {
-      EXPECT_CALL(visitor, OnError(_));
-    } else if (IsSpdy4() && flags & SETTINGS_FLAG_ACK) {
+    if (flags & ~SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS) {
       EXPECT_CALL(visitor, OnError(_));
     } else {
       EXPECT_CALL(visitor, OnSettings(
@@ -3861,14 +3891,48 @@ TEST_P(SpdyFramerTest, SettingsFrameFlags) {
     }
 
     framer.ProcessInput(frame->data(), frame->size());
-    if ((!IsSpdy4() &&
-         flags & ~SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS) ||
-        (IsSpdy4() && flags & ~SETTINGS_FLAG_ACK)) {
+    if (flags & ~SETTINGS_FLAG_CLEAR_PREVIOUSLY_PERSISTED_SETTINGS) {
+      EXPECT_EQ(SpdyFramer::SPDY_ERROR, framer.state());
+      EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME_FLAGS,
+                framer.error_code())
+          << SpdyFramer::ErrorCodeToString(framer.error_code());
+    } else {
+      EXPECT_EQ(SpdyFramer::SPDY_RESET, framer.state());
+      EXPECT_EQ(SpdyFramer::SPDY_NO_ERROR, framer.error_code())
+          << SpdyFramer::ErrorCodeToString(framer.error_code());
+    }
+  }
+}
+
+TEST_P(SpdyFramerTest, SettingsFrameFlags) {
+  if (spdy_version_ < 4) { return; }
+  for (int flags = 0; flags < 256; ++flags) {
+    SCOPED_TRACE(testing::Message() << "Flags " << flags);
+
+    testing::StrictMock<test::MockSpdyFramerVisitor> visitor;
+    SpdyFramer framer(spdy_version_);
+    framer.set_visitor(&visitor);
+
+    SpdySettingsIR settings_ir;
+    settings_ir.AddSetting(SETTINGS_INITIAL_WINDOW_SIZE, 0, 0, 16);
+    scoped_ptr<SpdyFrame> frame(framer.SerializeSettings(settings_ir));
+    SetFrameFlags(frame.get(), flags, spdy_version_);
+
+    if (flags != 0) {
+      EXPECT_CALL(visitor, OnError(_));
+    } else {
+      EXPECT_CALL(visitor, OnSettings(flags & SETTINGS_FLAG_ACK));
+      EXPECT_CALL(visitor, OnSetting(SETTINGS_INITIAL_WINDOW_SIZE, 0, 16));
+      EXPECT_CALL(visitor, OnSettingsEnd());
+    }
+
+    framer.ProcessInput(frame->data(), frame->size());
+    if (flags & ~SETTINGS_FLAG_ACK) {
       EXPECT_EQ(SpdyFramer::SPDY_ERROR, framer.state());
       EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME_FLAGS,
                 framer.error_code())
           << SpdyFramer::ErrorCodeToString(framer.error_code());
-    } else if (IsSpdy4() && flags & SETTINGS_FLAG_ACK) {
+    } else if (flags & SETTINGS_FLAG_ACK) {
       // The frame is invalid because ACK frames should have no payload.
       EXPECT_EQ(SpdyFramer::SPDY_ERROR, framer.state());
       EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME,
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index 38fe7ed..ce73744 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -4250,6 +4250,11 @@ TEST_P(SpdyNetworkTransactionTest, BufferedCancelled) {
 // Test that if the server requests persistence of settings, that we save
 // the settings in the HttpServerProperties.
 TEST_P(SpdyNetworkTransactionTest, SettingsSaved) {
+  if (spdy_util_.spdy_version() >= SPDY4) {
+    // SPDY4 doesn't support flags on individual settings, and
+    // has no concept of settings persistence.
+    return;
+  }
   static const SpdyHeaderInfo kSynReplyInfo = {
     SYN_REPLY,                              // Syn Reply
     1,                                      // Stream ID
diff --git a/net/spdy/spdy_protocol.h b/net/spdy/spdy_protocol.h
index 83f59f0..222a408 100644
--- a/net/spdy/spdy_protocol.h
+++ b/net/spdy/spdy_protocol.h
@@ -367,7 +367,7 @@ enum SpdyGoAwayStatus {
 };
 
 // A SPDY priority is a number between 0 and 7 (inclusive).
-// SPDY priority range is version-dependant. For SPDY 2 and below, priority is a
+// SPDY priority range is version-dependent. For SPDY 2 and below, priority is a
 // number between 0 and 3.
 typedef uint8 SpdyPriority;