From 941025cb9872c4be91d03194e2f42da0ba0d258a Mon Sep 17 00:00:00 2001 From: Tom Tromey Date: Thu, 11 Dec 2014 09:41:00 -0500 Subject: [PATCH] Bug 1047124 - Clean up memory leaks. r=BenWa --- build/sanitizers/lsan_suppressions.txt | 3 - tools/profiler/GeckoProfilerImpl.h | 9 +- tools/profiler/PseudoStack.h | 87 +++++++++++++++---- tools/profiler/SyncProfile.cpp | 5 +- tools/profiler/TableTicker.cpp | 2 - tools/profiler/moz.build | 1 + tools/profiler/platform-linux.cc | 2 +- tools/profiler/platform-macos.cc | 2 +- tools/profiler/platform-win32.cc | 2 +- tools/profiler/platform.cpp | 52 ++++++++--- tools/profiler/platform.h | 16 ++-- .../tests/gtest/ThreadProfileTest.cpp | 8 +- 12 files changed, 132 insertions(+), 57 deletions(-) diff --git a/build/sanitizers/lsan_suppressions.txt b/build/sanitizers/lsan_suppressions.txt index e90728dc2927..60ee2e3460b1 100644 --- a/build/sanitizers/lsan_suppressions.txt +++ b/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 diff --git a/tools/profiler/GeckoProfilerImpl.h b/tools/profiler/GeckoProfilerImpl.h index 833d0a88348b..42d7ccfbd6ee 100644 --- a/tools/profiler/GeckoProfilerImpl.h +++ b/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); diff --git a/tools/profiler/PseudoStack.h b/tools/profiler/PseudoStack.h index fb0836aee870..689910291883 100644 --- a/tools/profiler/PseudoStack.h +++ b/tools/profiler/PseudoStack.h @@ -12,6 +12,7 @@ #include "js/ProfilingStack.h" #include #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 mSleepIdObserved; // Keeps tack of whether the thread is sleeping or not (1 when sleeping 0 when awake) mozilla::Atomic 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 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 diff --git a/tools/profiler/SyncProfile.cpp b/tools/profiler/SyncProfile.cpp index 2d24d63bca47..d7190b280b66 100644 --- a/tools/profiler/SyncProfile.cpp +++ b/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 diff --git a/tools/profiler/TableTicker.cpp b/tools/profiler/TableTicker.cpp index 4a2ee5598ffd..963626d24213 100644 --- a/tools/profiler/TableTicker.cpp +++ b/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; } diff --git a/tools/profiler/moz.build b/tools/profiler/moz.build index 3cd031c319c3..a2ee3a2d6245 100644 --- a/tools/profiler/moz.build +++ b/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', diff --git a/tools/profiler/platform-linux.cc b/tools/profiler/platform-linux.cc index 35848cda6944..e13fbaa153d8 100644 --- a/tools/profiler/platform-linux.cc +++ b/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) { diff --git a/tools/profiler/platform-macos.cc b/tools/profiler/platform-macos.cc index 051d0caca2c5..003d960fd057 100644 --- a/tools/profiler/platform-macos.cc +++ b/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) { diff --git a/tools/profiler/platform-win32.cc b/tools/profiler/platform-win32.cc index 6a19765dff0d..56eb3b10452f 100644 --- a/tools/profiler/platform-win32.cc +++ b/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) { diff --git a/tools/profiler/platform.cpp b/tools/profiler/platform.cpp index c6c910807959..203eadac02b6 100644 --- a/tools/profiler/platform.cpp +++ b/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(); diff --git a/tools/profiler/platform.h b/tools/profiler/platform.h index dd94bd2e2714..16201b6327e5 100644 --- a/tools/profiler/platform.h +++ b/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_ */ diff --git a/tools/profiler/tests/gtest/ThreadProfileTest.cpp b/tools/profiler/tests/gtest/ThreadProfileTest.cpp index 02dc444ab893..d8ac6d08918a 100644 --- a/tools/profiler/tests/gtest/ThreadProfileTest.cpp +++ b/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;