Avoid invoking ExtensionVisitorInterface::OnSetting() for known SETTINGS parameters.
This CL changes Http2DecoderAdapter::OnSetting() to avoid calling
extension_->OnSetting() for known SETTINGS parameters (defined as
SpdyKnownSettingsId [1]). This change allows classes (e.g., OgHttp2Session) to
implement SpdyFramerVisitorInterface and ExtensionVisitorInterface without
having known SETTINGS parameters passed to them twice.
Archaeology shows prior state as:
1. SpdyFramerVisitorInterface::OnSetting() only accepted known SETTINGS;
ExtensionVisitorInterface::OnSetting() only accepted unknown SETTINGS.
2. Both OnSetting()s accepted known and unknown SETTINGS as of cl/186393706.
3. This CL causes ExtensionVisitorInterface::OnSetting() to only accept
unknown SETTINGS; SpdyFramerVisitorInterface::OnSetting() still accepts
known and unknown SETTINGS.
Note that Http2SettingsFields has a method IsSupportedParameter() [2], but that
would return false for certain SETTINGS parameters like
SETTINGS_ENABLE_CONNECT_PROTOCOL that we do not want to send to the
ExtensionVisitorInterface. Thus, this CL opts for ParseSettingsId() instead and
adds coverage to SpdyFramerTest.ReadKnownAndUnknownSettingsWithExtension.
This CL should be a functional no-op because production
ExtensionVisitorInterface::OnSetting() implementations currently check only for
the METADATA SETTINGS parameter, which would still be considered unknown.
[1] http://google3/third_party/spdy/core/spdy_protocol.h;l=139-162;rcl=387836338
[2] http://google3/third_party/http2/http2_structures.h;l=214;rcl=434624268
PiperOrigin-RevId: 435345949
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index 1925173..2aa0255 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -113,12 +113,10 @@
EXPECT_CALL(server_visitor, OnFrameHeader(0, 12, SETTINGS, 0x0));
EXPECT_CALL(server_visitor, OnSettingsStart());
EXPECT_CALL(server_visitor,
- OnSetting(Http2Setting{Http2KnownSettingsId::ENABLE_PUSH, 0u}))
- .Times(2);
+ OnSetting(Http2Setting{Http2KnownSettingsId::ENABLE_PUSH, 0u}));
EXPECT_CALL(
server_visitor,
- OnSetting(Http2Setting{Http2KnownSettingsId::MAX_HEADER_LIST_SIZE, 42u}))
- .Times(2);
+ OnSetting(Http2Setting{Http2KnownSettingsId::MAX_HEADER_LIST_SIZE, 42u}));
EXPECT_CALL(server_visitor, OnSettingsEnd());
{
const int64_t result = server_adapter->ProcessBytes(client_visitor.data());
@@ -167,8 +165,7 @@
EXPECT_CALL(client_visitor, OnSettingsStart());
EXPECT_CALL(client_visitor,
OnSetting(Http2Setting{
- Http2KnownSettingsId::ENABLE_CONNECT_PROTOCOL, 1u}))
- .Times(2);
+ Http2KnownSettingsId::ENABLE_CONNECT_PROTOCOL, 1u}));
EXPECT_CALL(client_visitor, OnSettingsEnd());
{
const int64_t result = client_adapter->ProcessBytes(server_visitor.data());
@@ -179,12 +176,10 @@
EXPECT_CALL(server_visitor, OnFrameHeader(0, 12, SETTINGS, 0x0));
EXPECT_CALL(server_visitor, OnSettingsStart());
EXPECT_CALL(server_visitor,
- OnSetting(Http2Setting{Http2KnownSettingsId::ENABLE_PUSH, 0u}))
- .Times(2);
+ OnSetting(Http2Setting{Http2KnownSettingsId::ENABLE_PUSH, 0u}));
EXPECT_CALL(
server_visitor,
- OnSetting(Http2Setting{Http2KnownSettingsId::MAX_HEADER_LIST_SIZE, 42u}))
- .Times(2);
+ OnSetting(Http2Setting{Http2KnownSettingsId::MAX_HEADER_LIST_SIZE, 42u}));
EXPECT_CALL(server_visitor, OnSettingsEnd());
{
const int64_t result = server_adapter->ProcessBytes(client_visitor.data());
@@ -1603,8 +1598,6 @@
EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0));
EXPECT_CALL(visitor, OnSettingsStart());
EXPECT_CALL(visitor, OnSetting(Http2Setting{HEADER_TABLE_SIZE, 100u}));
- // Duplicate setting callback due to the way extensions work.
- EXPECT_CALL(visitor, OnSetting(Http2Setting{HEADER_TABLE_SIZE, 100u}));
EXPECT_CALL(visitor, OnSettingsEnd());
const int64_t stream_result = adapter->ProcessBytes(stream_frames);
@@ -1636,8 +1629,6 @@
EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0));
EXPECT_CALL(visitor, OnSettingsStart());
EXPECT_CALL(visitor, OnSetting(Http2Setting{HEADER_TABLE_SIZE, 40960u}));
- // Duplicate setting callback due to the way extensions work.
- EXPECT_CALL(visitor, OnSetting(Http2Setting{HEADER_TABLE_SIZE, 40960u}));
EXPECT_CALL(visitor, OnSettingsEnd());
const int64_t stream_result = adapter->ProcessBytes(stream_frames);
@@ -2077,9 +2068,6 @@
EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0));
EXPECT_CALL(visitor, OnSettingsStart());
EXPECT_CALL(visitor, OnSetting);
- // TODO(diannahu): Remove this duplicate call with a separate
- // ExtensionVisitorInterface implementation.
- EXPECT_CALL(visitor, OnSetting);
EXPECT_CALL(visitor, OnSettingsEnd());
const int64_t initial_result = adapter->ProcessBytes(initial_frames);
@@ -2189,10 +2177,7 @@
// Server preface (SETTINGS with INITIAL_STREAM_WINDOW)
EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0));
EXPECT_CALL(visitor, OnSettingsStart());
- // TODO(diannahu): Remove the duplicate call with a separate
- // ExtensionVisitorInterface implementation.
- EXPECT_CALL(visitor, OnSetting(Http2Setting{INITIAL_WINDOW_SIZE, 80000u}))
- .Times(2);
+ EXPECT_CALL(visitor, OnSetting(Http2Setting{INITIAL_WINDOW_SIZE, 80000u}));
EXPECT_CALL(visitor, OnSettingsEnd());
EXPECT_CALL(visitor, OnFrameHeader(0, 4, WINDOW_UPDATE, 0));
EXPECT_CALL(visitor, OnWindowUpdate(0, 65536));
@@ -2310,10 +2295,7 @@
// SETTINGS with INITIAL_STREAM_WINDOW
EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0));
EXPECT_CALL(visitor, OnSettingsStart());
- // TODO(diannahu): Remove the duplicate call with a separate
- // ExtensionVisitorInterface implementation.
- EXPECT_CALL(visitor, OnSetting(Http2Setting{INITIAL_WINDOW_SIZE, 80000u}))
- .Times(2);
+ EXPECT_CALL(visitor, OnSetting(Http2Setting{INITIAL_WINDOW_SIZE, 80000u}));
EXPECT_CALL(visitor, OnSettingsEnd());
const int64_t settings_result = adapter->ProcessBytes(settings_frame);
@@ -2345,12 +2327,9 @@
// Server preface (SETTINGS with INITIAL_STREAM_WINDOW)
EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0));
EXPECT_CALL(visitor, OnSettingsStart());
- // TODO(diannahu): Remove the duplicate call with a separate
- // ExtensionVisitorInterface implementation.
- EXPECT_CALL(
- visitor,
- OnInvalidFrame(0, Http2VisitorInterface::InvalidFrameError::kFlowControl))
- .Times(2);
+ EXPECT_CALL(visitor,
+ OnInvalidFrame(
+ 0, Http2VisitorInterface::InvalidFrameError::kFlowControl));
EXPECT_CALL(visitor, OnConnectionError(ConnectionError::kFlowControlError));
const int64_t initial_result = adapter->ProcessBytes(initial_frames);
@@ -2429,9 +2408,8 @@
EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0));
EXPECT_CALL(visitor, OnSettingsStart());
- EXPECT_CALL(visitor,
- OnSetting(Http2Setting{INITIAL_WINDOW_SIZE, kLargeInitialWindow}))
- .Times(2);
+ EXPECT_CALL(visitor, OnSetting(Http2Setting{INITIAL_WINDOW_SIZE,
+ kLargeInitialWindow}));
EXPECT_CALL(visitor, OnSettingsEnd());
const int64_t read_result = adapter->ProcessBytes(frames);
@@ -3062,8 +3040,7 @@
EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0x0));
EXPECT_CALL(visitor, OnSettingsStart());
EXPECT_CALL(visitor, OnSetting(Http2Setting{
- Http2KnownSettingsId::MAX_CONCURRENT_STREAMS, 2u}))
- .Times(2);
+ Http2KnownSettingsId::MAX_CONCURRENT_STREAMS, 2u}));
EXPECT_CALL(visitor, OnSettingsEnd());
EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0x1));
EXPECT_CALL(visitor, OnSettingsAck());
@@ -3102,8 +3079,7 @@
EXPECT_CALL(visitor, OnFrameHeader(0, 6, SETTINGS, 0x0));
EXPECT_CALL(visitor, OnSettingsStart());
EXPECT_CALL(visitor, OnSetting(Http2Setting{
- Http2KnownSettingsId::MAX_CONCURRENT_STREAMS, 5u}))
- .Times(2);
+ Http2KnownSettingsId::MAX_CONCURRENT_STREAMS, 5u}));
EXPECT_CALL(visitor, OnSettingsEnd());
adapter->ProcessBytes(update_streams);
@@ -5248,7 +5224,7 @@
// Client preface (empty SETTINGS)
EXPECT_CALL(server_visitor, OnFrameHeader(0, _, SETTINGS, 0));
EXPECT_CALL(server_visitor, OnSettingsStart());
- EXPECT_CALL(server_visitor, OnSetting(_)).Times(testing::AnyNumber());
+ EXPECT_CALL(server_visitor, OnSetting).Times(testing::AnyNumber());
EXPECT_CALL(server_visitor, OnSettingsEnd());
// Stream 1
EXPECT_CALL(server_visitor, OnFrameHeader(1, _, HEADERS, 5));
diff --git a/spdy/core/http2_frame_decoder_adapter.cc b/spdy/core/http2_frame_decoder_adapter.cc
index f9e0f40..0f6da60 100644
--- a/spdy/core/http2_frame_decoder_adapter.cc
+++ b/spdy/core/http2_frame_decoder_adapter.cc
@@ -556,7 +556,8 @@
QUICHE_DVLOG(1) << "OnSetting: " << setting_fields;
const auto parameter = static_cast<SpdySettingsId>(setting_fields.parameter);
visitor()->OnSetting(parameter, setting_fields.value);
- if (extension_ != nullptr) {
+ SpdyKnownSettingsId known_id;
+ if (extension_ != nullptr && !spdy::ParseSettingsId(parameter, &known_id)) {
extension_->OnSetting(parameter, setting_fields.value);
}
}
diff --git a/spdy/core/http2_frame_decoder_adapter.h b/spdy/core/http2_frame_decoder_adapter.h
index 0c21972..5664470 100644
--- a/spdy/core/http2_frame_decoder_adapter.h
+++ b/spdy/core/http2_frame_decoder_adapter.h
@@ -527,7 +527,7 @@
public:
virtual ~ExtensionVisitorInterface() {}
- // Called when SETTINGS are received, including non-standard SETTINGS.
+ // Called when non-standard SETTINGS are received.
virtual void OnSetting(SpdySettingsId id, uint32_t value) = 0;
// Called when non-standard frames are received.
diff --git a/spdy/core/spdy_framer_test.cc b/spdy/core/spdy_framer_test.cc
index 80e6256..236a02f 100644
--- a/spdy/core/spdy_framer_test.cc
+++ b/spdy/core/spdy_framer_test.cc
@@ -3238,7 +3238,7 @@
TEST_P(SpdyFramerTest, ReadKnownAndUnknownSettingsWithExtension) {
const unsigned char kH2FrameData[] = {
- 0x00, 0x00, 0x12, // Length: 18
+ 0x00, 0x00, 0x18, // Length: 24
0x04, // Type: SETTINGS
0x00, // Flags: none
0x00, 0x00, 0x00, 0x00, // Stream: 0
@@ -3248,6 +3248,8 @@
0x00, 0x01, 0x00, 0x02, // Value: 65538
0x00, 0x02, // Param: ENABLE_PUSH
0x00, 0x00, 0x00, 0x01, // Value: 1
+ 0x00, 0x08, // Param: ENABLE_CONNECT_PROTOCOL
+ 0x00, 0x00, 0x00, 0x01, // Value: 1
};
TestSpdyVisitor visitor(SpdyFramer::DISABLE_COMPRESSION);
@@ -3257,14 +3259,13 @@
// In HTTP/2, we ignore unknown settings because of extensions. However, we
// pass the SETTINGS to the visitor, which can decide how to handle them.
- EXPECT_EQ(3, visitor.setting_count_);
+ EXPECT_EQ(4, visitor.setting_count_);
EXPECT_EQ(0, visitor.error_count_);
- // The extension receives all SETTINGS, including the non-standard SETTINGS.
+ // The extension receives only the non-standard SETTINGS.
EXPECT_THAT(
extension.settings_received_,
- testing::ElementsAre(testing::Pair(16, 2), testing::Pair(95, 65538),
- testing::Pair(2, 1)));
+ testing::ElementsAre(testing::Pair(16, 2), testing::Pair(95, 65538)));
}
// Tests handling of SETTINGS frame with entries out of order.
