Bug 741540 - Add AvailableMemoryTracker's hooks before any threads have started up. r=bsmedberg

--HG--
extra : rebase_source : 5135df39b3ae022e75f8651b2e10a378edbae158

xpcom/base/AvailableMemoryTracker.cpp

@@ -152,8 +152,11 @@ PRUint32 sNumLowPhysicalMemEvents = 0;
 WindowsDllInterceptor sKernel32Intercept;
 WindowsDllInterceptor sGdi32Intercept;
 
-// Have we installed the kernel intercepts above?
+// Has Init() been called?
-bool sHooksInstalled = false;
+bool sInitialized = false;
+
+// Has Activate() been called?  The hooks don't do anything until this happens.
+bool sHooksActive = false;
 
 // Alas, we'd like to use mozilla::TimeStamp, but we can't, because it acquires
 // a lock!
@@ -208,6 +211,10 @@ bool MaybeScheduleMemoryPressureEvent()
 
 void CheckMemAvailable()
 {
+  if (!sHooksActive) {
+    return;
+  }
+
   MEMORYSTATUSEX stat;
   stat.dwLength = sizeof(stat);
   bool success = GlobalMemoryStatusEx(&stat);
@@ -263,7 +270,7 @@ VirtualAllocHook(LPVOID aAddress, SIZE_T aSize,
   // virtual memory.  Similarly, don't call CheckMemAvailable for MEM_COMMIT if
   // we're not tracking low physical memory.
   if ((sLowVirtualMemoryThreshold != 0 && aAllocationType & MEM_RESERVE) ||
-      (sLowPhysicalMemoryThreshold != 0 &&  aAllocationType & MEM_COMMIT)) {
+      (sLowPhysicalMemoryThreshold != 0 && aAllocationType & MEM_COMMIT)) {
     LOG3("VirtualAllocHook(size=", aSize, ")");
     CheckMemAvailable();
   }
@@ -300,7 +307,7 @@ CreateDIBSectionHook(HDC aDC,
 
   // If aSection is non-null, CreateDIBSection won't allocate any new memory.
   bool doCheck = false;
-  if (!aSection && aBitmapInfo) {
+  if (sHooksActive && !aSection && aBitmapInfo) {
     PRUint16 bitCount = aBitmapInfo->bmiHeader.biBitCount;
     if (bitCount == 0) {
       // MSDN says bitCount == 0 means that it figures out how many bits each
@@ -382,12 +389,11 @@ public:
     aDescription.AssignLiteral(
       "Number of low-virtual-memory events fired since startup. ");
 
-    if (sLowVirtualMemoryThreshold == 0 || !sHooksInstalled) {
+    if (sLowVirtualMemoryThreshold == 0) {
-      aDescription.Append(nsPrintfCString(1024,
+      aDescription.AppendLiteral(
         "Tracking low-virtual-memory events is disabled, but you can enable it "
         "by giving the memory.low_virtual_mem_threshold_mb pref a non-zero "
-        "value%s.",
+        "value.");
-        sHooksInstalled ? "" : " and restarting"));
     }
     else {
       aDescription.Append(nsPrintfCString(1024,
@@ -425,12 +431,11 @@ public:
     aDescription.AssignLiteral(
       "Number of low-commit-space events fired since startup. ");
 
-    if (sLowCommitSpaceThreshold == 0 || !sHooksInstalled) {
+    if (sLowCommitSpaceThreshold == 0) {
-      aDescription.Append(nsPrintfCString(1024,
+      aDescription.Append(
         "Tracking low-commit-space events is disabled, but you can enable it "
         "by giving the memory.low_commit_space_threshold_mb pref a non-zero "
-        "value%s.",
+        "value.");
-        sHooksInstalled ? "" : " and restarting"));
     }
     else {
       aDescription.Append(nsPrintfCString(1024,
@@ -468,12 +473,11 @@ public:
     aDescription.AssignLiteral(
       "Number of low-physical-memory events fired since startup. ");
 
-    if (sLowPhysicalMemoryThreshold == 0 || !sHooksInstalled) {
+    if (sLowPhysicalMemoryThreshold == 0) {
-      aDescription.Append(nsPrintfCString(1024,
+      aDescription.Append(
         "Tracking low-physical-memory events is disabled, but you can enable it "
         "by giving the memory.low_physical_memory_threshold_mb pref a non-zero "
-        "value%s.",
+        "value.");
-        sHooksInstalled ? "" : " and restarting"));
     }
     else {
       aDescription.Append(nsPrintfCString(1024,
@@ -495,8 +499,11 @@ NS_IMPL_ISUPPORTS1(NumLowPhysicalMemoryEventsMemoryReporter, nsIMemoryReporter)
 namespace mozilla {
 namespace AvailableMemoryTracker {
 
-void Init()
+void Activate()
 {
+  MOZ_ASSERT(sInitialized);
+  MOZ_ASSERT(!sHooksActive);
+
   // On 64-bit systems, hardcode sLowVirtualMemoryThreshold to 0 -- we assume
   // we're not going to run out of virtual memory!
   if (sizeof(void*) > 4) {
@@ -514,14 +521,27 @@ void Init()
   Preferences::AddUintVarCache(&sLowMemoryNotificationIntervalMS,
       "memory.low_memory_notification_interval_ms", 10000);
 
-  // Don't register the hooks if we're a build instrumented for PGO or if both
+  NS_RegisterMemoryReporter(new NumLowCommitSpaceEventsMemoryReporter());
-  // thresholds are 0.  (If we're an instrumented build, the compiler adds
+  NS_RegisterMemoryReporter(new NumLowPhysicalMemoryEventsMemoryReporter());
-  // function calls all over the place which may call VirtualAlloc; this makes
+  if (sizeof(void*) == 4) {
-  // it hard to prevent VirtualAllocHook from reentering itself.)
+    NS_RegisterMemoryReporter(new NumLowVirtualMemoryEventsMemoryReporter());
+  }
+  sHooksActive = true;
+}
+
+void Init()
+{
+  // Don't register the hooks if we're a build instrumented for PGO: If we're
+  // an instrumented build, the compiler adds function calls all over the place
+  // which may call VirtualAlloc; this makes it hard to prevent
+  // VirtualAllocHook from reentering itself.
+
+  if (!PR_GetEnv("MOZ_PGO_INSTRUMENTED")) {
+    // Careful, this is not thread-safe!  AddHook sets up the trampoline before
+    // writing to the out param, and anyway, it writes to the function
+    // non-atomically.  So this must happen before any other threads which
+    // might call VirtualAlloc, MapViewOfFile, or CreateDIBSection start up.
 
-  if (!PR_GetEnv("MOZ_PGO_INSTRUMENTED") &&
-      (sLowVirtualMemoryThreshold != 0 || sLowPhysicalMemoryThreshold != 0)) {
-    sHooksInstalled = true;
     sKernel32Intercept.Init("Kernel32.dll");
     sKernel32Intercept.AddHook("VirtualAlloc",
       reinterpret_cast<intptr_t>(VirtualAllocHook),
@@ -535,15 +555,8 @@ void Init()
       reinterpret_cast<intptr_t>(CreateDIBSectionHook),
       (void**) &sCreateDIBSectionOrig);
   }
-  else {
-    sHooksInstalled = false;
-  }
 
-  NS_RegisterMemoryReporter(new NumLowCommitSpaceEventsMemoryReporter());
+  sInitialized = true;
-  NS_RegisterMemoryReporter(new NumLowPhysicalMemoryEventsMemoryReporter());
-  if (sizeof(void*) == 4) {
-    NS_RegisterMemoryReporter(new NumLowVirtualMemoryEventsMemoryReporter());
-  }
 }
 
 } // namespace AvailableMemoryTracker

xpcom/base/AvailableMemoryTracker.h

@@ -44,13 +44,23 @@ namespace mozilla {
 namespace AvailableMemoryTracker {
 
 // The AvailableMemoryTracker is implemented only on Windows.  But to make
-// callers' lives easier, we stub out an empty Init() call.  So you can always
+// callers' lives easier, we stub out empty calls for all its public functions.
-// initialize the AvailableMemoryTracker; it just might not do anything.
+// So you can always initialize the AvailableMemoryTracker; it just might not
+// do anything.
+//
+// Init() must be called before any other threads have started, because it
+// modifies the in-memory implementations of some DLL functions in
+// non-thread-safe ways.
+//
+// The hooks don't do anything until Activate() is called.  It's an error to
+// call Activate() without first calling Init().
 
 #if defined(XP_WIN)
 void Init();
+void Activate();
 #else
 void Init() {}
+void Activate() {}
 #endif
 
 } // namespace AvailableMemoryTracker

xpcom/build/nsXPComInit.cpp

@@ -350,6 +350,12 @@ NS_InitXPCOM2(nsIServiceManager* *result,
      // We are not shutting down
     gXPCOMShuttingDown = false;
 
+    NS_TIME_FUNCTION_MARK("Next: AvailableMemoryTracker Init()");
+
+    // Initialize the available memory tracker before other threads have had a
+    // chance to start up, because the initialization is not thread-safe.
+    mozilla::AvailableMemoryTracker::Init();
+
     NS_TIME_FUNCTION_MARK("Next: log init");
 
     NS_LogInit();
@@ -514,7 +520,7 @@ NS_InitXPCOM2(nsIServiceManager* *result,
     nsDirectoryService::gService->RegisterCategoryProviders();
 
     mozilla::scache::StartupCache::GetSingleton();
-    mozilla::AvailableMemoryTracker::Init();
+    mozilla::AvailableMemoryTracker::Activate();
 
     NS_TIME_FUNCTION_MARK("Next: create services from category");