Bug 1047124 - Clean up memory leaks. r=BenWa

build/sanitizers/lsan_suppressions.txt

@@ -15,9 +15,6 @@ leak:nsComponentManagerImpl
 leak:mozJSComponentLoader::LoadModule
 leak:nsNativeModuleLoader::LoadModule
 
-# Bug 980109 - Freeing this properly on shutdown is hard.
-leak:profiler_init
-
 # Bug 981220 - Pixman fails to free TLS memory.
 leak:pixman_implementation_lookup_composite
 

tools/profiler/GeckoProfilerImpl.h

@@ -16,6 +16,7 @@
 #include "GeckoProfilerFunc.h"
 #include "PseudoStack.h"
 #include "nsISupports.h"
+#include "ProfilerBacktrace.h"
 
 #ifdef MOZ_TASK_TRACER
 #include "GeckoTaskTracer.h"
@@ -248,12 +249,10 @@ static inline void profiler_tracing(const char* aCategory, const char* aInfo,
                                     ProfilerBacktrace* aCause,
                                     TracingMetadata aMetaData = TRACING_DEFAULT)
 {
-  if (!stack_key_initialized)
-    return;
-
   // Don't insert a marker if we're not profiling to avoid
   // the heap copy (malloc).
-  if (!profiler_is_active()) {
+  if (!stack_key_initialized || !profiler_is_active()) {
+    delete aCause;
     return;
   }
 
@@ -456,7 +455,7 @@ inline void mozilla_sampler_call_exit(void *aHandle)
     return;
 
   PseudoStack *stack = (PseudoStack*)aHandle;
-  stack->pop();
+  stack->popAndMaybeDelete();
 }
 
 void mozilla_sampler_add_marker(const char *aMarker, ProfilerMarkerPayload *aPayload);

tools/profiler/PseudoStack.h

@@ -12,6 +12,7 @@
 #include "js/ProfilingStack.h"
 #include <stdlib.h>
 #include "mozilla/Atomics.h"
+#include "nsISupportsImpl.h"
 
 /* we duplicate this code here to avoid header dependencies
  * which make it more difficult to include in other places */
@@ -286,24 +287,10 @@ void ProfilerJSEventMarker(const char *event);
 struct PseudoStack
 {
 public:
-  PseudoStack()
-    : mStackPointer(0)
-    , mSleepId(0)
-    , mSleepIdObserved(0)
-    , mSleeping(false)
-    , mRuntime(nullptr)
-    , mStartJSSampling(false)
-    , mPrivacyMode(false)
-  { }
-
-  ~PseudoStack() {
-    if (mStackPointer != 0) {
-      // We're releasing the pseudostack while it's still in use.
-      // The label macros keep a non ref counted reference to the
-      // stack to avoid a TLS. If these are not all cleared we will
-      // get a use-after-free so better to crash now.
-      abort();
-    }
+  // Create a new PseudoStack and acquire a reference to it.
+  static PseudoStack *create()
+  {
+    return new PseudoStack();
   }
 
   // This is called on every profiler restart. Put things that should happen at that time here.
@@ -356,6 +343,13 @@ public:
       return;
     }
 
+    // In order to ensure this object is kept alive while it is
+    // active, we acquire a reference at the outermost push.  This is
+    // released by the corresponding pop.
+    if (mStackPointer == 0) {
+      ref();
+    }
+
     volatile StackEntry &entry = mStack[mStackPointer];
 
     // Make sure we increment the pointer after the name has
@@ -381,9 +375,20 @@ public:
     STORE_SEQUENCER();
     mStackPointer++;
   }
-  void pop()
+
+  // Pop the stack.  If the stack is empty and all other references to
+  // this PseudoStack have been dropped, then the PseudoStack is
+  // deleted and "false" is returned.  Otherwise "true" is returned.
+  bool popAndMaybeDelete()
   {
     mStackPointer--;
+    if (mStackPointer == 0) {
+      // Release our self-owned reference count.  See 'push'.
+      deref();
+      return false;
+    } else {
+      return true;
+    }
   }
   bool isEmpty()
   {
@@ -432,6 +437,34 @@ public:
   // Keep a list of active checkpoints
   StackEntry volatile mStack[1024];
  private:
+
+  // A PseudoStack can only be created via the "create" method.
+  PseudoStack()
+    : mStackPointer(0)
+    , mSleepId(0)
+    , mSleepIdObserved(0)
+    , mSleeping(false)
+    , mRefCnt(1)
+    , mRuntime(nullptr)
+    , mStartJSSampling(false)
+    , mPrivacyMode(false)
+  { }
+
+  // A PseudoStack can only be deleted via deref.
+  ~PseudoStack() {
+    if (mStackPointer != 0) {
+      // We're releasing the pseudostack while it's still in use.
+      // The label macros keep a non ref counted reference to the
+      // stack to avoid a TLS. If these are not all cleared we will
+      // get a use-after-free so better to crash now.
+      abort();
+    }
+  }
+
+  // No copying.
+  PseudoStack(const PseudoStack&) MOZ_DELETE;
+  void operator=(const PseudoStack&) MOZ_DELETE;
+
   // Keep a list of pending markers that must be moved
   // to the circular buffer
   PendingMarkers mPendingMarkers;
@@ -446,6 +479,11 @@ public:
   mozilla::Atomic<int> mSleepIdObserved;
   // Keeps tack of whether the thread is sleeping or not (1 when sleeping 0 when awake)
   mozilla::Atomic<int> mSleeping;
+  // This class is reference counted because it must be kept alive by
+  // the ThreadInfo, by the reference from tlsPseudoStack, and by the
+  // current thread when callbacks are in progress.
+  mozilla::Atomic<int> mRefCnt;
+
  public:
   // The runtime which is being sampled
   JSRuntime *mRuntime;
@@ -478,6 +516,17 @@ public:
     mSleepId++;
     mSleeping = sleeping;
   }
+
+  void ref() {
+    ++mRefCnt;
+  }
+
+  void deref() {
+    int newValue = --mRefCnt;
+    if (newValue == 0) {
+      delete this;
+    }
+  }
 };
 
 #endif

tools/profiler/SyncProfile.cpp

@@ -21,7 +21,10 @@ SyncProfile::~SyncProfile()
   if (mUtb) {
     utb__release_sync_buffer(mUtb);
   }
-  Sampler::FreePlatformData(GetPlatformData());
+
+  // SyncProfile owns the ThreadInfo; see NewSyncProfile.
+  ThreadInfo* info = GetThreadInfo();
+  delete info;
 }
 
 bool

tools/profiler/TableTicker.cpp

@@ -915,9 +915,7 @@ void mozilla_sampler_print_location1()
 
   printf_stderr("Backtrace:\n");
   syncProfile->IterateTags(print_callback);
-  ThreadInfo* info = syncProfile->GetThreadInfo();
   delete syncProfile;
-  delete info;
 }
 
 

tools/profiler/moz.build

@@ -16,6 +16,7 @@ if CONFIG['MOZ_ENABLE_PROFILER_SPS']:
         'GeckoProfilerFunc.h',
         'GeckoProfilerImpl.h',
         'JSStreamWriter.h',
+        'ProfilerBacktrace.h',
         'ProfilerMarkers.h',
         'PseudoStack.h',
         'shared-libraries.h',

tools/profiler/platform-linux.cc

@@ -475,7 +475,7 @@ bool Sampler::RegisterCurrentThread(const char* aName,
 
   set_tls_stack_top(stackTop);
 
-  ThreadInfo* info = new ThreadInfo(aName, id,
+  ThreadInfo* info = new StackOwningThreadInfo(aName, id,
     aIsMainThread, aPseudoStack, stackTop);
 
   if (sActiveSampler) {

tools/profiler/platform-macos.cc

@@ -375,7 +375,7 @@ bool Sampler::RegisterCurrentThread(const char* aName,
 
   set_tls_stack_top(stackTop);
 
-  ThreadInfo* info = new ThreadInfo(aName, id,
+  ThreadInfo* info = new StackOwningThreadInfo(aName, id,
     aIsMainThread, aPseudoStack, stackTop);
 
   if (sActiveSampler) {

tools/profiler/platform-win32.cc

@@ -326,7 +326,7 @@ bool Sampler::RegisterCurrentThread(const char* aName,
 
   set_tls_stack_top(stackTop);
 
-  ThreadInfo* info = new ThreadInfo(aName, id,
+  ThreadInfo* info = new StackOwningThreadInfo(aName, id,
     aIsMainThread, aPseudoStack, stackTop);
 
   if (sActiveSampler) {

tools/profiler/platform.cpp

@@ -88,10 +88,6 @@ void Sampler::Startup() {
 
 void Sampler::Shutdown() {
   while (sRegisteredThreads->size() > 0) {
-    // Any stack that's still referenced at this point are
-    // still active and we don't have a way to clean them up
-    // safetly and still handle the pop call on that object.
-    sRegisteredThreads->back()->ForgetStack();
     delete sRegisteredThreads->back();
     sRegisteredThreads->pop_back();
   }
@@ -127,9 +123,6 @@ ThreadInfo::~ThreadInfo() {
     delete mProfile;
 
   Sampler::FreePlatformData(mPlatformData);
-
-  delete mPseudoStack;
-  mPseudoStack = nullptr;
 }
 
 void
@@ -143,6 +136,33 @@ ThreadInfo::SetPendingDelete()
   }
 }
 
+StackOwningThreadInfo::StackOwningThreadInfo(const char* aName, int aThreadId,
+                                             bool aIsMainThread,
+                                             PseudoStack* aPseudoStack,
+                                             void* aStackTop)
+  : ThreadInfo(aName, aThreadId, aIsMainThread, aPseudoStack, aStackTop)
+{
+  aPseudoStack->ref();
+}
+
+StackOwningThreadInfo::~StackOwningThreadInfo()
+{
+  PseudoStack* stack = Stack();
+  if (stack) {
+    stack->deref();
+  }
+}
+
+void
+StackOwningThreadInfo::SetPendingDelete()
+{
+  PseudoStack* stack = Stack();
+  if (stack) {
+    stack->deref();
+  }
+  ThreadInfo::SetPendingDelete();
+}
+
 ProfilerMarker::ProfilerMarker(const char* aMarkerName,
     ProfilerMarkerPayload* aPayload,
     float aTime)
@@ -519,7 +539,7 @@ void mozilla_sampler_init(void* stackTop)
 
   Sampler::Startup();
 
-  PseudoStack *stack = new PseudoStack();
+  PseudoStack *stack = PseudoStack::create();
   tlsPseudoStack.set(stack);
 
   bool isMainThread = true;
@@ -592,9 +612,9 @@ void mozilla_sampler_shutdown()
 
   Sampler::Shutdown();
 
-  // We can't delete the Stack because we can be between a
-  // sampler call_enter/call_exit point.
-  // TODO Need to find a safe time to delete Stack
+  PseudoStack *stack = tlsPseudoStack.get();
+  stack->deref();
+  tlsPseudoStack.set(nullptr);
 }
 
 void mozilla_sampler_save()
@@ -809,7 +829,7 @@ void mozilla_sampler_stop()
   LOG("BEGIN mozilla_sampler_stop");
 
   if (!stack_key_initialized)
-    profiler_init(nullptr);
+    return;
 
   TableTicker *t = tlsTicker.get();
   if (!t) {
@@ -942,7 +962,8 @@ bool mozilla_sampler_register_thread(const char* aName, void* stackTop)
   }
 #endif
 
-  PseudoStack* stack = new PseudoStack();
+  MOZ_ASSERT(tlsPseudoStack.get() == nullptr);
+  PseudoStack* stack = PseudoStack::create();
   tlsPseudoStack.set(stack);
   bool isMainThread = is_main_thread_name(aName);
   return Sampler::RegisterCurrentThread(aName, stack, isMainThread, stackTop);
@@ -950,7 +971,9 @@ bool mozilla_sampler_register_thread(const char* aName, void* stackTop)
 
 void mozilla_sampler_unregister_thread()
 {
-  if (sInitCount == 0) {
+  // Don't check sInitCount count here -- we may be unregistering the
+  // thread after the sampler was shut down.
+  if (!stack_key_initialized) {
     return;
   }
 
@@ -958,6 +981,7 @@ void mozilla_sampler_unregister_thread()
   if (!stack) {
     return;
   }
+  stack->deref();
   tlsPseudoStack.set(nullptr);
 
   Sampler::UnregisterCurrentThread();

tools/profiler/platform.h

@@ -405,11 +405,6 @@ class ThreadInfo {
 
   bool IsMainThread() const { return mIsMainThread; }
   PseudoStack* Stack() const { return mPseudoStack; }
-  PseudoStack* ForgetStack() {
-    PseudoStack* stack = mPseudoStack;
-    mPseudoStack = nullptr;
-    return stack;
-  }
 
   void SetProfile(ThreadProfile* aProfile) { mProfile = aProfile; }
   ThreadProfile* Profile() const { return mProfile; }
@@ -417,7 +412,7 @@ class ThreadInfo {
   PlatformData* GetPlatformData() const { return mPlatformData; }
   void* StackTop() const { return mStackTop; }
 
-  void SetPendingDelete();
+  virtual void SetPendingDelete();
   bool IsPendingDelete() const { return mPendingDelete; }
 
 #ifdef MOZ_NUWA_PROCESS
@@ -440,4 +435,13 @@ class ThreadInfo {
   bool mPendingDelete;
 };
 
+// Just like ThreadInfo, but owns a reference to the PseudoStack.
+class StackOwningThreadInfo : public ThreadInfo {
+ public:
+  StackOwningThreadInfo(const char* aName, int aThreadId, bool aIsMainThread, PseudoStack* aPseudoStack, void* aStackTop);
+  virtual ~StackOwningThreadInfo();
+
+  virtual void SetPendingDelete();
+};
+
 #endif /* ndef TOOLS_PLATFORM_H_ */

tools/profiler/tests/gtest/ThreadProfileTest.cpp

@@ -9,7 +9,7 @@
 
 // Make sure we can initialize our ThreadProfile
 TEST(ThreadProfile, Initialization) {
-  PseudoStack* stack = new PseudoStack();
+  PseudoStack* stack = PseudoStack::create();
   Thread::tid_t tid = 1000;
   ThreadInfo info("testThread", tid, true, stack, nullptr);
   ThreadProfile tp(&info, 10);
@@ -17,7 +17,7 @@ TEST(ThreadProfile, Initialization) {
 
 // Make sure we can record one tag and read it
 TEST(ThreadProfile, InsertOneTag) {
-  PseudoStack* stack = new PseudoStack();
+  PseudoStack* stack = PseudoStack::create();
   Thread::tid_t tid = 1000;
   ThreadInfo info("testThread", tid, true, stack, nullptr);
   ThreadProfile tp(&info, 10);
@@ -29,7 +29,7 @@ TEST(ThreadProfile, InsertOneTag) {
 
 // See if we can insert some tags
 TEST(ThreadProfile, InsertTagsNoWrap) {
-  PseudoStack* stack = new PseudoStack();
+  PseudoStack* stack = PseudoStack::create();
   Thread::tid_t tid = 1000;
   ThreadInfo info("testThread", tid, true, stack, nullptr);
   ThreadProfile tp(&info, 100);
@@ -48,7 +48,7 @@ TEST(ThreadProfile, InsertTagsNoWrap) {
 
 // See if wrapping works as it should in the basic case
 TEST(ThreadProfile, InsertTagsWrap) {
-  PseudoStack* stack = new PseudoStack();
+  PseudoStack* stack = PseudoStack::create();
   Thread::tid_t tid = 1000;
   // we can fit only 24 tags in this buffer because of the empty slot
   int tags = 24;