Fix crasher in QpackProgressiveDecoder destructor if QpackHeaderTable is already destroyed.
gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 272892449
Change-Id: Ib79e6c1227e4f4cc2fc2480c22126f647dbac95f
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc
index 3e0a6f8..931924f 100644
--- a/quic/core/http/quic_spdy_session_test.cc
+++ b/quic/core/http/quic_spdy_session_test.cc
@@ -2290,6 +2290,39 @@
session_.qpack_decoder()->OnInsertWithoutNameReference("foo", "bar");
}
+// Regression test for https://crbug.com/1011294.
+TEST_P(QuicSpdySessionTestServer, SessionDestroyedWhileHeaderDecodingBlocked) {
+ if (!VersionUsesQpack(transport_version())) {
+ return;
+ }
+
+ session_.qpack_decoder()->OnSetDynamicTableCapacity(1024);
+
+ QuicStreamId stream_id = GetNthClientInitiatedBidirectionalId(0);
+ TestStream* stream = session_.CreateIncomingStream(stream_id);
+
+ // HEADERS frame referencing first dynamic table entry.
+ std::string headers_payload = QuicTextUtils::HexDecode("020080");
+ std::unique_ptr<char[]> headers_buffer;
+ HttpEncoder encoder;
+ QuicByteCount headers_frame_header_length =
+ encoder.SerializeHeadersFrameHeader(headers_payload.length(),
+ &headers_buffer);
+ QuicStringPiece headers_frame_header(headers_buffer.get(),
+ headers_frame_header_length);
+ std::string headers = QuicStrCat(headers_frame_header, headers_payload);
+ stream->OnStreamFrame(QuicStreamFrame(stream_id, false, 0, headers));
+
+ // Decoding is blocked because dynamic table entry has not been received yet.
+ EXPECT_FALSE(stream->headers_decompressed());
+
+ // |session_| gets destoyed. That destroys QpackDecoder, a member of
+ // QuicSpdySession (derived class), which destroys QpackHeaderTable.
+ // Then |*stream|, owned by QuicSession (base class) get destroyed, which
+ // destroys QpackProgessiveDecoder, a registered Observer of QpackHeaderTable.
+ // This must not cause a crash.
+}
+
TEST_P(QuicSpdySessionTestClient, ResetAfterInvalidIncomingStreamType) {
if (!VersionHasStreamType(transport_version())) {
return;
diff --git a/quic/core/qpack/qpack_header_table.cc b/quic/core/qpack/qpack_header_table.cc
index 7274ce4..ff9be9a 100644
--- a/quic/core/qpack/qpack_header_table.cc
+++ b/quic/core/qpack/qpack_header_table.cc
@@ -19,7 +19,11 @@
max_entries_(0),
dropped_entry_count_(0) {}
-QpackHeaderTable::~QpackHeaderTable() = default;
+QpackHeaderTable::~QpackHeaderTable() {
+ for (auto& entry : observers_) {
+ entry.second->Cancel();
+ }
+}
const QpackEntry* QpackHeaderTable::LookupEntry(bool is_static,
uint64_t index) const {
diff --git a/quic/core/qpack/qpack_header_table.h b/quic/core/qpack/qpack_header_table.h
index 34dfd1c..78d85f6 100644
--- a/quic/core/qpack/qpack_header_table.h
+++ b/quic/core/qpack/qpack_header_table.h
@@ -48,6 +48,10 @@
// registered with. After this call the Observer automatically gets
// deregistered.
virtual void OnInsertCountReachedThreshold() = 0;
+
+ // Called when QpackHeaderTable is destroyed to let the Observer know that
+ // it must not call UnregisterObserver().
+ virtual void Cancel() = 0;
};
QpackHeaderTable();
@@ -99,9 +103,11 @@
// gets unregistered. Each observer must only be registered at most once.
void RegisterObserver(uint64_t required_insert_count, Observer* observer);
- // Unregister previously registered observer. Must be called before an
- // observer still waiting for notification is destroyed. Must be called with
- // the same |required_insert_count| value that |observer| was registered with.
+ // Unregister previously registered observer. Must be called with the same
+ // |required_insert_count| value that |observer| was registered with. Must be
+ // called before an observer still waiting for notification is destroyed,
+ // unless QpackHeaderTable already called Observer::Cancel(), in which case
+ // this method must not be called.
void UnregisterObserver(uint64_t required_insert_count, Observer* observer);
// Used on request streams to encode and decode Required Insert Count.
diff --git a/quic/core/qpack/qpack_header_table_test.cc b/quic/core/qpack/qpack_header_table_test.cc
index 907c2db..96e4d0a 100644
--- a/quic/core/qpack/qpack_header_table_test.cc
+++ b/quic/core/qpack/qpack_header_table_test.cc
@@ -23,6 +23,7 @@
~MockObserver() override = default;
MOCK_METHOD0(OnInsertCountReachedThreshold, void());
+ MOCK_METHOD0(Cancel, void());
};
class QpackHeaderTableTest : public QuicTest {
@@ -519,6 +520,15 @@
EXPECT_EQ(4u, table.draining_index(1.0));
}
+TEST_F(QpackHeaderTableTest, Cancel) {
+ StrictMock<MockObserver> observer;
+ auto table = std::make_unique<QpackHeaderTable>();
+ table->RegisterObserver(1, &observer);
+
+ EXPECT_CALL(observer, Cancel);
+ table.reset();
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/qpack/qpack_progressive_decoder.cc b/quic/core/qpack/qpack_progressive_decoder.cc
index 7037f15..34eb2b3 100644
--- a/quic/core/qpack/qpack_progressive_decoder.cc
+++ b/quic/core/qpack/qpack_progressive_decoder.cc
@@ -36,10 +36,11 @@
prefix_decoded_(false),
blocked_(false),
decoding_(true),
- error_detected_(false) {}
+ error_detected_(false),
+ cancelled_(false) {}
QpackProgressiveDecoder::~QpackProgressiveDecoder() {
- if (blocked_) {
+ if (blocked_ && !cancelled_) {
header_table_->UnregisterObserver(required_insert_count_, this);
}
}
@@ -133,6 +134,10 @@
}
}
+void QpackProgressiveDecoder::Cancel() {
+ cancelled_ = true;
+}
+
bool QpackProgressiveDecoder::DoIndexedHeaderFieldInstruction() {
if (!instruction_decoder_.s_bit()) {
uint64_t absolute_index;
diff --git a/quic/core/qpack/qpack_progressive_decoder.h b/quic/core/qpack/qpack_progressive_decoder.h
index dbb4093..91b1e8c 100644
--- a/quic/core/qpack/qpack_progressive_decoder.h
+++ b/quic/core/qpack/qpack_progressive_decoder.h
@@ -89,6 +89,7 @@
// QpackHeaderTable::Observer implementation.
void OnInsertCountReachedThreshold() override;
+ void Cancel() override;
private:
bool DoIndexedHeaderFieldInstruction();
@@ -145,6 +146,10 @@
// True if a decoding error has been detected.
bool error_detected_;
+
+ // True if QpackHeaderTable has been destroyed
+ // while decoding is still blocked.
+ bool cancelled_;
};
} // namespace quic
