Bug 1380096 - Avoid non-null terminated strings and heap for main thread runnable name, r=erahm

For some reason, I continuously ran into windows x64 specific failures when
trying to read this heap allocated data while the main thread was paused. I
don't know specifically how this happened, but I am able to avoid it by instead
directly allocating the buffer in a `mozilla::Array` in static storage, and
copying that data instead.

MozReview-Commit-ID: 473d6IpHlc4

xpcom/threads/ThreadStackHelper.cpp

@@ -138,7 +138,8 @@ ThreadStackHelper::GetStacksInternal(Stack* aStack,
   ScopedSetPtr<NativeStack> nativeStackPtr(mNativeStackToFill, aNativeStack);
 #endif
 
-  char nameBuffer[1000] = {0};
+  Array<char, nsThread::kRunnableNameBufSize> runnableName;
+  runnableName[0] = '\0';
   auto callback = [&, this] (void** aPCs, size_t aCount, bool aIsMainThread) {
     // NOTE: We cannot allocate any memory in this callback, as the target
     // thread is suspended, so we first copy it into a stack-allocated buffer,
@@ -148,10 +149,8 @@ ThreadStackHelper::GetStacksInternal(Stack* aStack,
     // Currently we only store the names of runnables which are running on the
     // main thread, so we only want to read sMainThreadRunnableName and copy its
     // value in the case that we are currently suspending the main thread.
-    if (aIsMainThread && nsThread::sMainThreadRunnableName) {
-      strncpy(nameBuffer, nsThread::sMainThreadRunnableName, sizeof(nameBuffer));
-      // Make sure the string is null-terminated.
-      nameBuffer[sizeof(nameBuffer) - 1] = '\0';
+    if (aIsMainThread) {
+      runnableName = nsThread::sMainThreadRunnableName;
     }
 
 #ifdef MOZ_THREADSTACKHELPER_PSEUDO
@@ -176,10 +175,11 @@ ThreadStackHelper::GetStacksInternal(Stack* aStack,
                                        /* aSampleNative = */ !!aNativeStack);
   }
 
-  // Copy the name buffer allocation into the output string.
-  if (nameBuffer[0] != 0) {
-    aRunnableName = nameBuffer;
-  }
+  // Copy the name buffer allocation into the output string. We explicitly set
+  // the last byte to null in case we read in some corrupted data without a null
+  // terminator.
+  runnableName[nsThread::kRunnableNameBufSize - 1] = '\0';
+  aRunnableName.AssignASCII(runnableName.cbegin());
 #endif
 }
 

xpcom/threads/nsThread.cpp

@@ -99,7 +99,7 @@ static LazyLogModule sThreadLog("nsThread");
 
 NS_DECL_CI_INTERFACE_GETTER(nsThread)
 
-const char* nsThread::sMainThreadRunnableName = nullptr;
+Array<char, nsThread::kRunnableNameBufSize> nsThread::sMainThreadRunnableName;
 
 //-----------------------------------------------------------------------------
 // Because we do not have our own nsIFactory, we have to implement nsIClassInfo
@@ -1422,15 +1422,24 @@ nsThread::ProcessNextEvent(bool aMayWait, bool* aResult)
 
       // If we're on the main thread, we want to record our current runnable's
       // name in a static so that BHR can record it.
-      const char* restoreRunnableName = nullptr;
+      Array<char, kRunnableNameBufSize> restoreRunnableName;
+      restoreRunnableName[0] = '\0';
       auto clear = MakeScopeExit([&] {
         if (MAIN_THREAD == mIsMainThread) {
+          MOZ_ASSERT(NS_IsMainThread());
           sMainThreadRunnableName = restoreRunnableName;
         }
       });
       if (MAIN_THREAD == mIsMainThread) {
+        MOZ_ASSERT(NS_IsMainThread());
         restoreRunnableName = sMainThreadRunnableName;
-        sMainThreadRunnableName = name.get();
+
+        // Copy the name into sMainThreadRunnableName's buffer, and append a
+        // terminating null.
+        uint32_t length = std::min((uint32_t) kRunnableNameBufSize - 1,
+                                   (uint32_t) name.Length());
+        memcpy(sMainThreadRunnableName.begin(), name.BeginReading(), length);
+        sMainThreadRunnableName[length] = '\0';
       }
 #endif
 

xpcom/threads/nsThread.h

@@ -21,6 +21,7 @@
 #include "nsAutoPtr.h"
 #include "mozilla/AlreadyAddRefed.h"
 #include "mozilla/UniquePtr.h"
+#include "mozilla/Array.h"
 
 namespace mozilla {
 class CycleCollectedJSContext;
@@ -93,7 +94,8 @@ public:
   static bool SaveMemoryReportNearOOM(ShouldSaveMemoryReport aShouldSave);
 #endif
 
-  static const char* sMainThreadRunnableName;
+  static const uint32_t kRunnableNameBufSize = 1000;
+  static mozilla::Array<char, kRunnableNameBufSize> sMainThreadRunnableName;
 
 private:
   void DoMainThreadSpecificProcessing(bool aReallyWait);