Bug 1713415 - Don't discard the unprocessed loading events. r=aklotz

We had the constant `kMaxEvents` which limits the maximum number of loading
events in `UntrustedModulesData`.  Because we had a check for the number of
events in `UntrustedModulesData::AddNewLoads` where we already swapped
`UntrustedModulesProcessor::mUnprocessedModuleLoads` with a local variable,
when the array exceeds `kMaxEvents`, we discarded the remaining loading events.

The proposed fix is to check for `kMaxEvents` before swapping the unprocessed
events, so that the remaining events will be processed the next time.

This was caught by the `UntrustedModulesFixture` GTest.  This test had
another test bug that it always expected modules were loaded in the main
thread except the predefined known modules.  This is not correct because
the same process may have loaded a bunch of modules in the earlier GTests
such as graphics drivers in different threads.  This patch includes a fix
for that bug as well.

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

toolkit/xre/UntrustedModulesData.cpp

@@ -321,21 +321,8 @@ void UntrustedModulesData::AddNewLoads(
     Unused << mModules.LookupOrInsert(entry.GetKey(), entry.GetData());
   }
 
-  // This constant matches the maximum in Telemetry::CombinedStacks
-  const size_t kMaxEvents = 50;
   MOZ_ASSERT(mEvents.length() <= kMaxEvents);
 
-  if (mEvents.length() + aEvents.length() > kMaxEvents) {
-    // Ensure that we will never retain more tha kMaxEvents events
-    size_t newLength = kMaxEvents - mEvents.length();
-    if (!newLength) {
-      return;
-    }
-
-    aEvents.shrinkTo(newLength);
-    aStacks.shrinkTo(newLength);
-  }
-
   if (mEvents.empty()) {
     mEvents = std::move(aEvents);
   } else {

toolkit/xre/UntrustedModulesData.h

@@ -169,6 +169,10 @@ class ModulesMap final
 
 class UntrustedModulesData final {
  public:
+  // Ensure mEvents will never retain more than kMaxEvents events.
+  // This constant matches the maximum in Telemetry::CombinedStacks.
+  static constexpr size_t kMaxEvents = 50;
+
   UntrustedModulesData()
       : mProcessType(XRE_GetProcessType()),
         mPid(::GetCurrentProcessId()),

toolkit/xre/UntrustedModulesProcessor.cpp

@@ -553,7 +553,27 @@ void UntrustedModulesProcessor::ProcessModuleLoadQueue() {
   {  // Scope for lock
     MutexAutoLock lock(mUnprocessedMutex);
     CancelScheduledProcessing(lock);
-    loadsToProcess.swap(mUnprocessedModuleLoads);
+
+    // The potential size of mProcessedModuleLoads if all of the unprocessed
+    // events are from third-party modules.
+    const size_t newDataLength = mProcessedModuleLoads.mEvents.length() +
+                                 mUnprocessedModuleLoads.length();
+    if (newDataLength <= UntrustedModulesData::kMaxEvents) {
+      loadsToProcess.swap(mUnprocessedModuleLoads);
+    } else {
+      // To prevent mProcessedModuleLoads from exceeding |kMaxEvents|,
+      // we process the first items in the mUnprocessedModuleLoads,
+      // leaving the the remaining events for the next time.
+      const size_t capacity = UntrustedModulesData::kMaxEvents >
+                                      mProcessedModuleLoads.mEvents.length()
+                                  ? (UntrustedModulesData::kMaxEvents -
+                                     mProcessedModuleLoads.mEvents.length())
+                                  : 0;
+      auto moveRangeBegin = mUnprocessedModuleLoads.begin();
+      auto moveRangeEnd = moveRangeBegin + capacity;
+      Unused << loadsToProcess.moveAppend(moveRangeBegin, moveRangeEnd);
+      mUnprocessedModuleLoads.erase(moveRangeBegin, moveRangeEnd);
+    }
   }
 
   if (!mAllowProcessing || loadsToProcess.empty()) {

toolkit/xre/test/gtest/TestUntrustedModules.cpp

@@ -135,64 +135,6 @@ class UntrustedModulesCollector {
   }
 };
 
-static void ValidateUntrustedModules(const UntrustedModulesData& aData) {
-  // This defines a list of modules which are listed on our blocklist and
-  // thus its loading status is not expected to be Status::Loaded.
-  // Although the UntrustedModulesFixture test does not touch any of them,
-  // the current process might have run a test like TestDllBlocklist where
-  // we try to load and block them.
-  const struct {
-    const wchar_t* mName;
-    ModuleLoadInfo::Status mStatus;
-  } kKnownModules[] = {
-      // Sorted by mName for binary-search
-      {L"TestDllBlocklist_MatchByName.dll", ModuleLoadInfo::Status::Blocked},
-      {L"TestDllBlocklist_MatchByVersion.dll", ModuleLoadInfo::Status::Blocked},
-      {L"TestDllBlocklist_NoOpEntryPoint.dll",
-       ModuleLoadInfo::Status::Redirected},
-  };
-
-  EXPECT_EQ(aData.mProcessType, GeckoProcessType_Default);
-  EXPECT_EQ(aData.mPid, ::GetCurrentProcessId());
-
-  nsTHashtable<nsPtrHashKey<void>> moduleSet;
-  for (const RefPtr<ModuleRecord>& module : aData.mModules.Values()) {
-    moduleSet.PutEntry(module);
-  }
-
-  for (const auto& evt : aData.mEvents) {
-    const nsDependentSubstring leafName =
-        nt::GetLeafName(evt.mModule->mResolvedNtName);
-    const nsAutoString leafNameStr(leafName.Data(), leafName.Length());
-    size_t match;
-    if (BinarySearchIf(
-            kKnownModules, 0, ArrayLength(kKnownModules),
-            [&leafNameStr](const auto& aVal) {
-              return _wcsicmp(leafNameStr.get(), aVal.mName);
-            },
-            &match)) {
-      // No check for mThreadId because a known module may be loaded
-      // in a different thread.
-      EXPECT_EQ(evt.mLoadStatus,
-                static_cast<uint32_t>(kKnownModules[match].mStatus));
-    } else {
-      EXPECT_EQ(evt.mThreadId, ::GetCurrentThreadId());
-      EXPECT_EQ(evt.mLoadStatus, 0);
-    }
-
-    // Make sure mModule is pointing to an entry of mModules.
-    EXPECT_TRUE(moduleSet.Contains(evt.mModule));
-    EXPECT_FALSE(evt.mIsDependent);
-  }
-
-  // No check for the mXULLoadDurationMS field because the field has a value
-  // in CCov build GTest, but it is empty in non-CCov build (bug 1681936).
-  EXPECT_GT(aData.mEvents.length(), 0);
-  EXPECT_GT(aData.mStacks.GetModuleCount(), 0);
-  EXPECT_EQ(aData.mSanitizationFailures, 0);
-  EXPECT_EQ(aData.mTrustTestFailures, 0);
-}
-
 class UntrustedModulesFixture : public TelemetryTestFixture {
   static constexpr int kLoadCountBeforeDllServices = 5;
   static constexpr int kLoadCountAfterDllServices = 5;
@@ -229,6 +171,8 @@ class UntrustedModulesFixture : public TelemetryTestFixture {
       kLoadCountBeforeDllServices + kLoadCountAfterDllServices;
   static const nsString kTestModules[];
 
+  static void ValidateUntrustedModules(const UntrustedModulesData& aData);
+
   static void LoadAndFree(const nsAString& aLeaf) {
     nsModuleHandle dll(::LoadLibraryW(PrependWorkingDir(aLeaf).get()));
     EXPECT_TRUE(!!dll);
@@ -301,10 +245,81 @@ class UntrustedModulesFixture : public TelemetryTestFixture {
 };
 
 const nsString UntrustedModulesFixture::kTestModules[] = {
-    u"TestUntrustedModules_Dll1.dll"_ns, u"TestUntrustedModules_Dll2.dll"_ns};
+    // Sorted for binary-search
+    u"TestUntrustedModules_Dll1.dll"_ns,
+    u"TestUntrustedModules_Dll2.dll"_ns,
+};
+
 INIT_ONCE UntrustedModulesFixture::sInitLoadOnce = INIT_ONCE_STATIC_INIT;
 UntrustedModulesCollector UntrustedModulesFixture::sInitLoadDataCollector;
 
+void UntrustedModulesFixture::ValidateUntrustedModules(
+    const UntrustedModulesData& aData) {
+  // This defines a list of modules which are listed on our blocklist and
+  // thus its loading status is not expected to be Status::Loaded.
+  // Although the UntrustedModulesFixture test does not touch any of them,
+  // the current process might have run a test like TestDllBlocklist where
+  // we try to load and block them.
+  const struct {
+    const wchar_t* mName;
+    ModuleLoadInfo::Status mStatus;
+  } kKnownModules[] = {
+      // Sorted by mName for binary-search
+      {L"TestDllBlocklist_MatchByName.dll", ModuleLoadInfo::Status::Blocked},
+      {L"TestDllBlocklist_MatchByVersion.dll", ModuleLoadInfo::Status::Blocked},
+      {L"TestDllBlocklist_NoOpEntryPoint.dll",
+       ModuleLoadInfo::Status::Redirected},
+  };
+
+  EXPECT_EQ(aData.mProcessType, GeckoProcessType_Default);
+  EXPECT_EQ(aData.mPid, ::GetCurrentProcessId());
+
+  nsTHashtable<nsPtrHashKey<void>> moduleSet;
+  for (const RefPtr<ModuleRecord>& module : aData.mModules.Values()) {
+    moduleSet.PutEntry(module);
+  }
+
+  for (const auto& evt : aData.mEvents) {
+    const nsDependentSubstring leafName =
+        nt::GetLeafName(evt.mModule->mResolvedNtName);
+    const nsAutoString leafNameStr(leafName.Data(), leafName.Length());
+    size_t match;
+    if (BinarySearchIf(
+            kKnownModules, 0, ArrayLength(kKnownModules),
+            [&leafNameStr](const auto& aVal) {
+              return _wcsicmp(leafNameStr.get(), aVal.mName);
+            },
+            &match)) {
+      EXPECT_EQ(evt.mLoadStatus,
+                static_cast<uint32_t>(kKnownModules[match].mStatus));
+    } else {
+      EXPECT_EQ(evt.mLoadStatus, 0);
+    }
+
+    if (BinarySearchIf(
+            kTestModules, 0, ArrayLength(kTestModules),
+            [&leafNameStr](const auto& aVal) {
+              return _wcsicmp(leafNameStr.get(), aVal.get());
+            },
+            &match)) {
+      // We know the test modules are loaded in the main thread,
+      // but we don't know about other modules.
+      EXPECT_EQ(evt.mThreadId, ::GetCurrentThreadId());
+    }
+
+    // Make sure mModule is pointing to an entry of mModules.
+    EXPECT_TRUE(moduleSet.Contains(evt.mModule));
+    EXPECT_FALSE(evt.mIsDependent);
+  }
+
+  // No check for the mXULLoadDurationMS field because the field has a value
+  // in CCov build GTest, but it is empty in non-CCov build (bug 1681936).
+  EXPECT_GT(aData.mEvents.length(), 0);
+  EXPECT_GT(aData.mStacks.GetModuleCount(), 0);
+  EXPECT_EQ(aData.mSanitizationFailures, 0);
+  EXPECT_EQ(aData.mTrustTestFailures, 0);
+}
+
 BOOL CALLBACK UntrustedModulesFixture::InitialModuleLoadOnce(PINIT_ONCE, void*,
                                                              void**) {
   for (int i = 0; i < kLoadCountBeforeDllServices; ++i) {