Backed out changeset a90edecdce6f (bug 1713415) for module checking crashes (bug 1717059). a=backout

toolkit/xre/UntrustedModulesData.cpp

@@ -321,8 +321,21 @@ 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,10 +169,6 @@ 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,23 +553,7 @@ void UntrustedModulesProcessor::ProcessModuleLoadQueue() {
   {  // Scope for lock
     MutexAutoLock lock(mUnprocessedMutex);
     CancelScheduledProcessing(lock);
-
-    // 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 = newDataLength - UntrustedModulesData::kMaxEvents;
-      auto moveRangeBegin = mUnprocessedModuleLoads.begin();
-      auto moveRangeEnd = moveRangeBegin + capacity;
-      Unused << loadsToProcess.moveAppend(moveRangeBegin, moveRangeEnd);
-      mUnprocessedModuleLoads.erase(moveRangeBegin, moveRangeEnd);
-    }
+    loadsToProcess.swap(mUnprocessedModuleLoads);
   }
 
   if (!mAllowProcessing || loadsToProcess.empty()) {

toolkit/xre/test/gtest/TestUntrustedModules.cpp

@@ -135,6 +135,64 @@ 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;
@@ -171,8 +229,6 @@ 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);
@@ -245,81 +301,10 @@ class UntrustedModulesFixture : public TelemetryTestFixture {
 };
 
 const nsString UntrustedModulesFixture::kTestModules[] = {
-    // Sorted for binary-search
-    u"TestUntrustedModules_Dll1.dll"_ns,
-    u"TestUntrustedModules_Dll2.dll"_ns,
-};
-
+    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) {