Backed out changeset 53adf5b021fa (bug 1047124) for Huge Memory Leaks on a CLOSED TREE

build/sanitizers/lsan_suppressions.txt

@@ -15,6 +15,9 @@ 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,7 +16,6 @@
 #include "GeckoProfilerFunc.h"
 #include "PseudoStack.h"
 #include "nsISupports.h"
-#include "ProfilerBacktrace.h"
 
 #ifdef MOZ_TASK_TRACER
 #include "GeckoTaskTracer.h"
@@ -249,10 +248,12 @@ 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 (!stack_key_initialized || !profiler_is_active()) {
-    delete aCause;
+  if (!profiler_is_active()) {
     return;
   }
 
@@ -455,7 +456,7 @@ inline void mozilla_sampler_call_exit(void *aHandle)
     return;
 
   PseudoStack *stack = (PseudoStack*)aHandle;
-  stack->popAndMaybeDelete();
+  stack->pop();
 }
 
 void mozilla_sampler_add_marker(const char *aMarker, ProfilerMarkerPayload *aPayload);

tools/profiler/PseudoStack.h

@@ -12,7 +12,6 @@
 #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 */
@@ -287,10 +286,24 @@ void ProfilerJSEventMarker(const char *event);
 struct PseudoStack
 {
 public:
-  // Create a new PseudoStack and acquire a reference to it.
-  static PseudoStack *create()
-  {
-    return new PseudoStack();
+  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();
+    }
   }
 
   // This is called on every profiler restart. Put things that should happen at that time here.
@@ -343,13 +356,6 @@ 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
@@ -375,20 +381,9 @@ public:
     STORE_SEQUENCER();
     mStackPointer++;
   }
-
-  // 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()
+  void pop()
   {
     mStackPointer--;
-    if (mStackPointer == 0) {
-      // Release our self-owned reference count.  See 'push'.
-      deref();
-      return false;
-    } else {
-      return true;
-    }
   }
   bool isEmpty()
   {
@@ -437,35 +432,6 @@ 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() {
-    MOZ_COUNT_DTOR(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;
@@ -480,11 +446,6 @@ 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;
@@ -517,17 +478,6 @@ public:
     mSleepId++;
     mSleeping = sleeping;
   }
-
-  void ref() {
-    ++mRefCnt;
-  }
-
-  void deref() {
-    int newValue = --mRefCnt;
-    if (newValue == 0) {
-      delete this;
-    }
-  }
 };
 
 #endif

tools/profiler/SyncProfile.cpp

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

tools/profiler/TableTicker.cpp

@@ -915,7 +915,9 @@ 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,7 +16,6 @@ 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 StackOwningThreadInfo(aName, id,
+  ThreadInfo* info = new ThreadInfo(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 StackOwningThreadInfo(aName, id,
+  ThreadInfo* info = new ThreadInfo(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 StackOwningThreadInfo(aName, id,
+  ThreadInfo* info = new ThreadInfo(aName, id,
     aIsMainThread, aPseudoStack, stackTop);
 
   if (sActiveSampler) {

tools/profiler/platform.cpp

@@ -88,6 +88,10 @@ 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();
   }
@@ -123,6 +127,9 @@ ThreadInfo::~ThreadInfo() {
     delete mProfile;
 
   Sampler::FreePlatformData(mPlatformData);
+
+  delete mPseudoStack;
+  mPseudoStack = nullptr;
 }
 
 void
@@ -136,33 +143,6 @@ 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)
@@ -539,7 +519,7 @@ void mozilla_sampler_init(void* stackTop)
 
   Sampler::Startup();
 
-  PseudoStack *stack = PseudoStack::create();
+  PseudoStack *stack = new PseudoStack();
   tlsPseudoStack.set(stack);
 
   bool isMainThread = true;
@@ -612,9 +592,9 @@ void mozilla_sampler_shutdown()
 
   Sampler::Shutdown();
 
-  PseudoStack *stack = tlsPseudoStack.get();
-  stack->deref();
-  tlsPseudoStack.set(nullptr);
+  // 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
 }
 
 void mozilla_sampler_save()
@@ -829,7 +809,7 @@ void mozilla_sampler_stop()
   LOG("BEGIN mozilla_sampler_stop");
 
   if (!stack_key_initialized)
-    return;
+    profiler_init(nullptr);
 
   TableTicker *t = tlsTicker.get();
   if (!t) {
@@ -962,8 +942,7 @@ bool mozilla_sampler_register_thread(const char* aName, void* stackTop)
   }
 #endif
 
-  MOZ_ASSERT(tlsPseudoStack.get() == nullptr);
-  PseudoStack* stack = PseudoStack::create();
+  PseudoStack* stack = new PseudoStack();
   tlsPseudoStack.set(stack);
   bool isMainThread = is_main_thread_name(aName);
   return Sampler::RegisterCurrentThread(aName, stack, isMainThread, stackTop);
@@ -971,9 +950,7 @@ bool mozilla_sampler_register_thread(const char* aName, void* stackTop)
 
 void mozilla_sampler_unregister_thread()
 {
-  // Don't check sInitCount count here -- we may be unregistering the
-  // thread after the sampler was shut down.
-  if (!stack_key_initialized) {
+  if (sInitCount == 0) {
     return;
   }
 
@@ -981,7 +958,6 @@ void mozilla_sampler_unregister_thread()
   if (!stack) {
     return;
   }
-  stack->deref();
   tlsPseudoStack.set(nullptr);
 
   Sampler::UnregisterCurrentThread();

tools/profiler/platform.h

@@ -405,6 +405,11 @@ 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; }
@@ -412,7 +417,7 @@ class ThreadInfo {
   PlatformData* GetPlatformData() const { return mPlatformData; }
   void* StackTop() const { return mStackTop; }
 
-  virtual void SetPendingDelete();
+  void SetPendingDelete();
   bool IsPendingDelete() const { return mPendingDelete; }
 
 #ifdef MOZ_NUWA_PROCESS
@@ -435,13 +440,4 @@ 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 = PseudoStack::create();
+  PseudoStack* stack = new PseudoStack();
   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 = PseudoStack::create();
+  PseudoStack* stack = new PseudoStack();
   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 = PseudoStack::create();
+  PseudoStack* stack = new PseudoStack();
   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 = PseudoStack::create();
+  PseudoStack* stack = new PseudoStack();
   Thread::tid_t tid = 1000;
   // we can fit only 24 tags in this buffer because of the empty slot
   int tags = 24;