Fix QPACK crash by unregistering observers with QpackHeaderTable.
QpackHeaderTable is owned by QpackDecoder, owned by QuicSpdySession. Each
observer is a QpackProgressiveDecoder, owned by QpackHeaderAccumulator, owned by
QuicSpdyStream. If a stream is deleted (local cancellation, reset received from
peer), then the observer must be deregistered with QpackHeaderTable in order to
avoid a crash.
gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 272562981
Change-Id: Ia49f2ef9aeeebcccb83405107f0224ee7d23694f
diff --git a/quic/core/qpack/qpack_header_table.cc b/quic/core/qpack/qpack_header_table.cc
index 30f15a5..7274ce4 100644
--- a/quic/core/qpack/qpack_header_table.cc
+++ b/quic/core/qpack/qpack_header_table.cc
@@ -197,6 +197,21 @@
observers_.insert({required_insert_count, observer});
}
+void QpackHeaderTable::UnregisterObserver(uint64_t required_insert_count,
+ Observer* observer) {
+ auto it = observers_.lower_bound(required_insert_count);
+ while (it != observers_.end() && it->first == required_insert_count) {
+ if (it->second == observer) {
+ observers_.erase(it);
+ return;
+ }
+ ++it;
+ }
+
+ // |observer| must have been registered.
+ QUIC_NOTREACHED();
+}
+
uint64_t QpackHeaderTable::draining_index(float draining_fraction) const {
DCHECK_LE(0.0, draining_fraction);
DCHECK_LE(draining_fraction, 1.0);
diff --git a/quic/core/qpack/qpack_header_table.h b/quic/core/qpack/qpack_header_table.h
index 919a4eb..34dfd1c 100644
--- a/quic/core/qpack/qpack_header_table.h
+++ b/quic/core/qpack/qpack_header_table.h
@@ -99,6 +99,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.
+ void UnregisterObserver(uint64_t required_insert_count, Observer* observer);
+
// Used on request streams to encode and decode Required Insert Count.
uint64_t max_entries() const { return max_entries_; }
diff --git a/quic/core/qpack/qpack_header_table_test.cc b/quic/core/qpack/qpack_header_table_test.cc
index 5a97c20..907c2db 100644
--- a/quic/core/qpack/qpack_header_table_test.cc
+++ b/quic/core/qpack/qpack_header_table_test.cc
@@ -94,6 +94,11 @@
table_.RegisterObserver(required_insert_count, observer);
}
+ void UnregisterObserver(uint64_t required_insert_count,
+ QpackHeaderTable::Observer* observer) {
+ table_.UnregisterObserver(required_insert_count, observer);
+ }
+
uint64_t max_entries() const { return table_.max_entries(); }
uint64_t inserted_entry_count() const {
return table_.inserted_entry_count();
@@ -416,7 +421,7 @@
table.MaxInsertSizeWithoutEvictingGivenEntry(1));
}
-TEST_F(QpackHeaderTableTest, Observer) {
+TEST_F(QpackHeaderTableTest, RegisterObserver) {
StrictMock<MockObserver> observer1;
RegisterObserver(1, &observer1);
EXPECT_CALL(observer1, OnInsertCountReachedThreshold);
@@ -455,6 +460,27 @@
Mock::VerifyAndClearExpectations(&observer5);
}
+TEST_F(QpackHeaderTableTest, UnregisterObserver) {
+ StrictMock<MockObserver> observer1;
+ StrictMock<MockObserver> observer2;
+ StrictMock<MockObserver> observer3;
+ StrictMock<MockObserver> observer4;
+ RegisterObserver(1, &observer1);
+ RegisterObserver(2, &observer2);
+ RegisterObserver(2, &observer3);
+ RegisterObserver(3, &observer4);
+
+ UnregisterObserver(2, &observer3);
+
+ EXPECT_CALL(observer1, OnInsertCountReachedThreshold);
+ EXPECT_CALL(observer2, OnInsertCountReachedThreshold);
+ EXPECT_CALL(observer4, OnInsertCountReachedThreshold);
+ InsertEntry("foo", "bar");
+ InsertEntry("foo", "bar");
+ InsertEntry("foo", "bar");
+ EXPECT_EQ(3u, inserted_entry_count());
+}
+
TEST_F(QpackHeaderTableTest, DrainingIndex) {
QpackHeaderTable table;
table.SetMaximumDynamicTableCapacity(kMaximumDynamicTableCapacityForTesting);
diff --git a/quic/core/qpack/qpack_progressive_decoder.cc b/quic/core/qpack/qpack_progressive_decoder.cc
index 5632286..7037f15 100644
--- a/quic/core/qpack/qpack_progressive_decoder.cc
+++ b/quic/core/qpack/qpack_progressive_decoder.cc
@@ -38,6 +38,12 @@
decoding_(true),
error_detected_(false) {}
+QpackProgressiveDecoder::~QpackProgressiveDecoder() {
+ if (blocked_) {
+ header_table_->UnregisterObserver(required_insert_count_, this);
+ }
+}
+
void QpackProgressiveDecoder::Decode(QuicStringPiece data) {
DCHECK(decoding_);
@@ -290,11 +296,11 @@
prefix_decoded_ = true;
if (required_insert_count_ > header_table_->inserted_entry_count()) {
- blocked_ = true;
if (!enforcer_->OnStreamBlocked(stream_id_)) {
OnError("Limit on number of blocked streams exceeded.");
return false;
}
+ blocked_ = true;
header_table_->RegisterObserver(required_insert_count_, this);
}
diff --git a/quic/core/qpack/qpack_progressive_decoder.h b/quic/core/qpack/qpack_progressive_decoder.h
index 5c116b1..dbb4093 100644
--- a/quic/core/qpack/qpack_progressive_decoder.h
+++ b/quic/core/qpack/qpack_progressive_decoder.h
@@ -74,7 +74,7 @@
HeadersHandlerInterface* handler);
QpackProgressiveDecoder(const QpackProgressiveDecoder&) = delete;
QpackProgressiveDecoder& operator=(const QpackProgressiveDecoder&) = delete;
- ~QpackProgressiveDecoder() override = default;
+ ~QpackProgressiveDecoder() override;
// Provide a data fragment to decode.
void Decode(QuicStringPiece data);
