Bug 1755186 - HpackDynamicTableReporter::CollectReports should hold a mutex r=necko-reviewers,jesup

HpackDynamicTableReporter::mMutex protects access to HpackDynamicTableReporter::mCompressor
nvFIFO::mMutex protects access to mvFIFO::mTable - is aquired when adding or removing elements in the table, and when reporting SizeOfDynamicTable

Differential Revision: https://phabricator.services.mozilla.com/D200814

netwerk/protocol/http/Http2Compression.cpp

@@ -61,6 +61,7 @@ class HpackDynamicTableReporter final : public nsIMemoryReporter {
   NS_IMETHOD
   CollectReports(nsIHandleReportCallback* aHandleReport, nsISupports* aData,
                  bool aAnonymize) override {
+    MutexAutoLock lock(mMutex);
     if (mCompressor) {
       MOZ_COLLECT_REPORT("explicit/network/hpack/dynamic-tables", KIND_HEAP,
                          UNITS_BYTES,
@@ -75,7 +76,8 @@ class HpackDynamicTableReporter final : public nsIMemoryReporter {
 
   ~HpackDynamicTableReporter() = default;
 
-  Http2BaseCompressor* mCompressor;
+  Mutex mMutex{"HpackDynamicTableReporter"};
+  Http2BaseCompressor* mCompressor MOZ_GUARDED_BY(mMutex);
 
   friend class Http2BaseCompressor;
 };
@@ -187,13 +189,18 @@ nvFIFO::~nvFIFO() { Clear(); }
 void nvFIFO::AddElement(const nsCString& name, const nsCString& value) {
   nvPair* pair = new nvPair(name, value);
   mByteCount += pair->Size();
+  MutexAutoLock lock(mMutex);
   mTable.PushFront(pair);
 }
 
 void nvFIFO::AddElement(const nsCString& name) { AddElement(name, ""_ns); }
 
 void nvFIFO::RemoveElement() {
-  nvPair* pair = mTable.Pop();
+  nvPair* pair = nullptr;
+  {
+    MutexAutoLock lock(mMutex);
+    pair = mTable.Pop();
+  }
   if (pair) {
     mByteCount -= pair->Size();
     delete pair;
@@ -212,6 +219,7 @@ size_t nvFIFO::StaticLength() const { return gStaticHeaders->GetSize(); }
 
 void nvFIFO::Clear() {
   mByteCount = 0;
+  MutexAutoLock lock(mMutex);
   while (mTable.GetSize()) {
     delete mTable.Pop();
   }
@@ -244,20 +252,27 @@ Http2BaseCompressor::~Http2BaseCompressor() {
     Telemetry::Accumulate(mPeakCountID, mPeakCount);
   }
   UnregisterStrongMemoryReporter(mDynamicReporter);
-  mDynamicReporter->mCompressor = nullptr;
+  {
+    MutexAutoLock lock(mDynamicReporter->mMutex);
+    mDynamicReporter->mCompressor = nullptr;
+  }
   mDynamicReporter = nullptr;
 }
 
+size_t nvFIFO::SizeOfDynamicTable(mozilla::MallocSizeOf aMallocSizeOf) const {
+  size_t size = 0;
+  MutexAutoLock lock(mMutex);
+  for (const auto elem : mTable) {
+    size += elem->SizeOfIncludingThis(aMallocSizeOf);
+  }
+  return size;
+}
+
 void Http2BaseCompressor::ClearHeaderTable() { mHeaderTable.Clear(); }
 
 size_t Http2BaseCompressor::SizeOfExcludingThis(
     mozilla::MallocSizeOf aMallocSizeOf) const {
-  size_t size = 0;
-  for (uint32_t i = mHeaderTable.StaticLength(); i < mHeaderTable.Length();
-       ++i) {
-    size += mHeaderTable[i]->SizeOfIncludingThis(aMallocSizeOf);
-  }
-  return size;
+  return mHeaderTable.SizeOfDynamicTable(aMallocSizeOf);
 }
 
 void Http2BaseCompressor::MakeRoom(uint32_t amount, const char* direction) {

netwerk/protocol/http/Http2Compression.h

@@ -13,6 +13,7 @@
 #include "nsDeque.h"
 #include "nsString.h"
 #include "mozilla/Telemetry.h"
+#include "mozilla/Mutex.h"
 
 namespace mozilla {
 namespace net {
@@ -47,10 +48,18 @@ class nvFIFO {
   size_t StaticLength() const;
   void Clear();
   const nvPair* operator[](size_t index) const;
+  size_t SizeOfDynamicTable(mozilla::MallocSizeOf aMallocSizeOf) const;
 
  private:
   uint32_t mByteCount{0};
   nsDeque<nvPair> mTable;
+
+  // This mutex is held when adding or removing elements in the table
+  // and when accessing the table from the main thread (in SizeOfDynamicTable)
+  // Since the operator[] and other const methods are always called
+  // on the socket thread, they don't need to lock the mutex.
+  // Mutable so it can be locked in SizeOfDynamicTable which is const
+  mutable Mutex mMutex{"nvFIFO"} MOZ_UNANNOTATED;
 };
 
 class HpackDynamicTableReporter;