Do not call HpackWholeEntryBuffer::error_detected() in HpackDecoder::error_detected(). There is no point in calling HpackWholeEntryBuffer::error_detected() in HpackDecoder::error_detected(), because HpackWholeEntryBuffer is constructed with HpackDecoder::decoder_state_ as its listener, so when HpackWholeEntryBuffer::error_detected_ is set in HpackWholeEntryBuffer::ReportError(), HpackDecoder::decoder_state_ is also notified via OnHpackDecodeError(), which calls HpackDecoderState::ReportError(), setting HpackDecoderState::error_detected_ which HpackDecoder::error_detected() queries anyway. This simplification is in preparation for introducing an error code enum to plumb error reasons into QuicSpdySession. gfe-relnote: Do not call HpackWholeEntryBuffer::error_detected() in HpackDecoder::error_detected(). Protected by gfe_reloadable_flag_http2_skip_querying_entry_buffer_error. PiperOrigin-RevId: 293574711 Change-Id: I14248de25e164cc97787082553b8226f6a8921dd
diff --git a/http2/hpack/decoder/hpack_decoder.cc b/http2/hpack/decoder/hpack_decoder.cc index 6d1ebbd..b0d5bf8 100644 --- a/http2/hpack/decoder/hpack_decoder.cc +++ b/http2/hpack/decoder/hpack_decoder.cc
@@ -16,7 +16,9 @@ : decoder_state_(listener), entry_buffer_(&decoder_state_, max_string_size), block_decoder_(&entry_buffer_), - error_detected_(false) {} + error_detected_(false), + http2_skip_querying_entry_buffer_error_( + GetHttp2ReloadableFlag(http2_skip_querying_entry_buffer_error)) {} HpackDecoder::~HpackDecoder() = default; @@ -35,8 +37,8 @@ bool HpackDecoder::StartDecodingBlock() { HTTP2_DVLOG(3) << "HpackDecoder::StartDecodingBlock, error_detected=" - << (error_detected() ? "true" : "false"); - if (error_detected()) { + << (DetectError() ? "true" : "false"); + if (DetectError()) { return false; } // TODO(jamessynge): Eliminate Reset(), which shouldn't be necessary @@ -49,9 +51,9 @@ bool HpackDecoder::DecodeFragment(DecodeBuffer* db) { HTTP2_DVLOG(3) << "HpackDecoder::DecodeFragment, error_detected=" - << (error_detected() ? "true" : "false") + << (DetectError() ? "true" : "false") << ", size=" << db->Remaining(); - if (error_detected()) { + if (DetectError()) { HTTP2_CODE_COUNT_N(decompress_failure_3, 3, 23); return false; } @@ -64,7 +66,7 @@ ReportError("HPACK block malformed."); HTTP2_CODE_COUNT_N(decompress_failure_3, 4, 23); return false; - } else if (error_detected()) { + } else if (DetectError()) { HTTP2_CODE_COUNT_N(decompress_failure_3, 5, 23); return false; } @@ -79,8 +81,8 @@ bool HpackDecoder::EndDecodingBlock() { HTTP2_DVLOG(3) << "HpackDecoder::EndDecodingBlock, error_detected=" - << (error_detected() ? "true" : "false"); - if (error_detected()) { + << (DetectError() ? "true" : "false"); + if (DetectError()) { HTTP2_CODE_COUNT_N(decompress_failure_3, 6, 23); return false; } @@ -91,7 +93,7 @@ return false; } decoder_state_.OnHeaderBlockEnd(); - if (error_detected()) { + if (DetectError()) { // HpackDecoderState will have reported the error. HTTP2_CODE_COUNT_N(decompress_failure_3, 8, 23); return false; @@ -99,18 +101,29 @@ return true; } -bool HpackDecoder::error_detected() { - if (!error_detected_) { - if (entry_buffer_.error_detected()) { +bool HpackDecoder::DetectError() { + if (error_detected_) { + return true; + } + + if (decoder_state_.error_detected()) { + HTTP2_DVLOG(2) << "HpackDecoder::error_detected in decoder_state_"; + HTTP2_CODE_COUNT_N(decompress_failure_3, 10, 23); + HTTP2_CODE_COUNT_N(http2_skip_querying_entry_buffer_error, 1, 3); + error_detected_ = true; + } else if (entry_buffer_.error_detected()) { + // This should never happen, because if an error had occured in + // |entry_buffer_|, it would have notified its listener, |decoder_state_|. + if (http2_skip_querying_entry_buffer_error_) { + HTTP2_CODE_COUNT_N(http2_skip_querying_entry_buffer_error, 2, 3); + } else { HTTP2_DVLOG(2) << "HpackDecoder::error_detected in entry_buffer_"; HTTP2_CODE_COUNT_N(decompress_failure_3, 9, 23); - error_detected_ = true; - } else if (decoder_state_.error_detected()) { - HTTP2_DVLOG(2) << "HpackDecoder::error_detected in decoder_state_"; - HTTP2_CODE_COUNT_N(decompress_failure_3, 10, 23); + HTTP2_CODE_COUNT_N(http2_skip_querying_entry_buffer_error, 3, 3); error_detected_ = true; } } + return error_detected_; }
diff --git a/http2/hpack/decoder/hpack_decoder.h b/http2/hpack/decoder/hpack_decoder.h index 0de876a..1b332d9 100644 --- a/http2/hpack/decoder/hpack_decoder.h +++ b/http2/hpack/decoder/hpack_decoder.h
@@ -92,8 +92,9 @@ // and returns true; else returns false. bool EndDecodingBlock(); - // Was an error detected? - bool error_detected(); + // If no error has been detected so far, query |decoder_state_| for errors and + // set |error_detected_| if necessary. + bool DetectError(); // Returns the estimate of dynamically allocated memory in bytes. size_t EstimateMemoryUsage() const; @@ -116,6 +117,9 @@ // Has an error been detected? bool error_detected_; + + // Latched value of reloadable_flag_http2_skip_querying_entry_buffer_error. + const bool http2_skip_querying_entry_buffer_error_; }; } // namespace http2
diff --git a/http2/hpack/decoder/hpack_decoder_test.cc b/http2/hpack/decoder/hpack_decoder_test.cc index 185ada6..1b8cdd9 100644 --- a/http2/hpack/decoder/hpack_decoder_test.cc +++ b/http2/hpack/decoder/hpack_decoder_test.cc
@@ -127,15 +127,15 @@ AssertionResult DecodeBlock(quiche::QuicheStringPiece block) { HTTP2_VLOG(1) << "HpackDecoderTest::DecodeBlock"; - VERIFY_FALSE(decoder_.error_detected()); + VERIFY_FALSE(decoder_.DetectError()); VERIFY_TRUE(error_messages_.empty()); VERIFY_FALSE(saw_start_); VERIFY_FALSE(saw_end_); header_entries_.clear(); - VERIFY_FALSE(decoder_.error_detected()); + VERIFY_FALSE(decoder_.DetectError()); VERIFY_TRUE(decoder_.StartDecodingBlock()); - VERIFY_FALSE(decoder_.error_detected()); + VERIFY_FALSE(decoder_.DetectError()); if (fragment_the_hpack_block_) { // See note in ctor regarding RNG. @@ -151,14 +151,14 @@ VERIFY_TRUE(decoder_.DecodeFragment(&db)); VERIFY_EQ(0u, db.Remaining()); } - VERIFY_FALSE(decoder_.error_detected()); + VERIFY_FALSE(decoder_.DetectError()); VERIFY_TRUE(decoder_.EndDecodingBlock()); if (saw_end_) { - VERIFY_FALSE(decoder_.error_detected()); + VERIFY_FALSE(decoder_.DetectError()); VERIFY_TRUE(error_messages_.empty()); } else { - VERIFY_TRUE(decoder_.error_detected()); + VERIFY_TRUE(decoder_.DetectError()); VERIFY_FALSE(error_messages_.empty()); } @@ -1088,7 +1088,7 @@ EXPECT_TRUE(decoder_.StartDecodingBlock()); DecodeBuffer db("\xff\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x00"); EXPECT_FALSE(decoder_.DecodeFragment(&db)); - EXPECT_TRUE(decoder_.error_detected()); + EXPECT_TRUE(decoder_.DetectError()); EXPECT_FALSE(saw_end_); EXPECT_EQ(1u, error_messages_.size()); EXPECT_THAT(error_messages_[0], HasSubstr("malformed")); @@ -1103,7 +1103,7 @@ EXPECT_TRUE(decoder_.StartDecodingBlock()); DecodeBuffer db("\x80"); EXPECT_FALSE(decoder_.DecodeFragment(&db)); - EXPECT_TRUE(decoder_.error_detected()); + EXPECT_TRUE(decoder_.DetectError()); EXPECT_FALSE(saw_end_); EXPECT_EQ(1u, error_messages_.size()); EXPECT_THAT(error_messages_[0], HasSubstr("Invalid index"));