Closed
Bug 1329929
Opened 7 years ago
Closed 7 years ago
Fix memory leaking for TaskTracer
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: sinker, Assigned: sinker)
References
Details
(Keywords: memory-leak)
Attachments
(1 file, 9 obsolete files)
14.28 KB,
patch
|
cyu
:
review+
|
Details | Diff | Splinter Review |
There is 8 bytes of leaking for 64bits platform, and 4 bytes for 32bits. Once tasktracer is built, mochitests would turn orange for memory leaking.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8825327 -
Flags: review?(cyu)
Updated•7 years ago
|
Attachment #8825327 -
Flags: review?(cyu) → review+
Assignee | ||
Comment 2•7 years ago
|
||
--
Attachment #8825327 [details] [diff] - Attachment is obsolete: true
Attachment #8826841 -
Flags: review?(cyu)
Assignee | ||
Updated•7 years ago
|
Attachment #8825327 -
Attachment is obsolete: true
Comment 3•7 years ago
|
||
Comment on attachment 8826841 [details] [diff] [review] Fix memory leaking, v2 Review of attachment 8826841 [details] [diff] [review]: ----------------------------------------------------------------- The following issue need to be addressed: ::: tools/profiler/tasktracer/GeckoTaskTracer.cpp @@ +201,5 @@ > ShutdownTaskTracer() > { > if (IsStartLogging()) { > SetLogStarted(false); > + delete sTraceInfos; sTraceInfos is deleted without any synchronization. It would be catastrophic if the following happens: 1. Some thread successfully gets an instance of TraceInfo by calling GetOrCreateTraceInfo() 2. ShutdownTaskTracer() is called to obsolete all TraceInfo instances. 3. delete sTraceInfos, which destroys all TraceInfo instances. 4. Context switch back to the thread in 1. The TraceInfo instance is used after free. Unless you are sure when ShutdownTaskTracer() is called without any stray thread running, the above case need to be addressed.
Assignee | ||
Comment 4•7 years ago
|
||
class TraceInfoHolder has a move contructor to pass and hold the per-thread lock.
Attachment #8826841 -
Attachment is obsolete: true
Attachment #8826841 -
Flags: review?(cyu)
Attachment #8835445 -
Flags: review?(cyu)
Assignee | ||
Updated•7 years ago
|
Attachment #8835445 -
Flags: review?(cyu)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8835445 -
Attachment is obsolete: true
Attachment #8836027 -
Flags: review?(cyu)
Assignee | ||
Comment 6•7 years ago
|
||
remove spaces.
Attachment #8836027 -
Attachment is obsolete: true
Attachment #8836027 -
Flags: review?(cyu)
Assignee | ||
Updated•7 years ago
|
Attachment #8837481 -
Flags: review?(cyu)
Comment 7•7 years ago
|
||
Comment on attachment 8837481 [details] [diff] [review] Fix memory leaking of TaskTracer, v4 Review of attachment 8837481 [details] [diff] [review]: ----------------------------------------------------------------- 1. I like the idea of using TraceInfoHolder, but holding an rvalue reference to a function-returned instance is nontrivial. 2. The problem of destroying unlocked mutexed needs to be fixed. ::: tools/profiler/tasktracer/GeckoTaskTracer.cpp @@ +85,5 @@ > { > // Create a new unique task id. > uint64_t newId = GenNewUniqueTaskId(); > + { > + TraceInfoHolder &&info = GetOrCreateTraceInfo(); nit: the convention is TraceInfoHolder&& info = ... Also consider using TraceInfoHolder info = ... @@ +118,5 @@ > static void > DestroySourceEvent() > { > // Log a fake end for this source event. > + TraceInfoHolder &&info = GetOrCreateTraceInfo(); Same here. @@ +132,5 @@ > +inline static void > +ObsoleteCurrentTraceInfos() > +{ > + // Note that we can't and don't need to acquire sMutex here because this > + // function is called before the other threads are recreated. This comment is no longer valid. Please remove it. @@ +204,5 @@ > ShutdownTaskTracer() > { > if (IsStartLogging()) { > SetLogStarted(false); > + for (auto &traceinfo: *sTraceInfos) { Nit: auto& traceinfo: ... @@ +208,5 @@ > + for (auto &traceinfo: *sTraceInfos) { > + // Block all threads from the access. > + traceinfo->mLogsMutex.Lock(); > + } > + delete sTraceInfos; I don't think it a good idea to destroy a locked mutex. Now that you want to delete sTraceInfos here, you are sure that it won't be accessed via the TLS variable other thread, it would make sense that you unlock all mutexes before destroying them. @@ +222,5 @@ > UniquePtr<TraceInfo> traceinfo(aTraceInfo); > mozilla::DebugOnly<bool> removed = > sTraceInfos->RemoveElement(traceinfo); > MOZ_ASSERT(removed); > Unused << traceinfo.release(); // A dirty hack to prevent double free. aTraceInfo is destroyed when it's mLogsMutex is locked. Now that this function is called by passing the TLS value, this is the owning thread of aTraceInfo. I think it would be safe to remove the instance without locking mLogsMutex. @@ +256,5 @@ > > uint64_t > GenNewUniqueTaskId() > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); Nit: rvalue reference to returned value. @@ +278,5 @@ > void > SetCurTraceInfo(uint64_t aSourceEventId, uint64_t aParentTaskId, > SourceEventType aSourceEventType) > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); Same here. @@ +290,5 @@ > void > GetCurTraceInfo(uint64_t* aOutSourceEventId, uint64_t* aOutParentTaskId, > SourceEventType* aOutSourceEventType) > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); And here. @@ +309,5 @@ > void > LogDispatch(uint64_t aTaskId, uint64_t aParentTaskId, uint64_t aSourceEventId, > SourceEventType aSourceEventType, int aDelayTimeMs) > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); And here. @@ +333,5 @@ > > void > LogBegin(uint64_t aTaskId, uint64_t aSourceEventId) > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); And here. @@ +351,5 @@ > > void > LogEnd(uint64_t aTaskId, uint64_t aSourceEventId) > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); And here. @@ +367,5 @@ > > void > LogVirtualTablePtr(uint64_t aTaskId, uint64_t aSourceEventId, uintptr_t* aVptr) > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); And here. @@ +419,5 @@ > } > > void AddLabel(const char* aFormat, ...) > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); And here. ::: tools/profiler/tasktracer/GeckoTaskTracerImpl.h @@ +106,5 @@ > int mLogsSize; > nsTArray<nsCString> mStrs; > }; > > +class TraceInfoHolder { Consider annotating this with MOZ_STACK_CLASS ::: tools/profiler/tasktracer/TracedTaskCommon.cpp @@ +33,5 @@ > > void > TracedTaskCommon::Init() > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); Binding a reference to the non-reference return value of a function is nontrivial. Consider just use TraceInfoHolder info = GetOrCreateTraceInfo() and let move constructor do the magic. @@ +53,5 @@ > > void > TracedTaskCommon::GetTLSTraceInfo() > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); Same here. @@ +65,5 @@ > > void > TracedTaskCommon::SetTLSTraceInfo() > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); And here. @@ +78,5 @@ > > void > TracedTaskCommon::ClearTLSTraceInfo() > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); And here.
Attachment #8837481 -
Flags: review?(cyu) → review-
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8837481 [details] [diff] [review] Fix memory leaking of TaskTracer, v4 Review of attachment 8837481 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/tasktracer/GeckoTaskTracer.cpp @@ +208,5 @@ > + for (auto &traceinfo: *sTraceInfos) { > + // Block all threads from the access. > + traceinfo->mLogsMutex.Lock(); > + } > + delete sTraceInfos; Without doing so, it is possible to destroy traceinfos while some other threads are holding mLogsMutex and accessing its traceinfo. mLogsMutex was acquired immediately after sMutex was acquired. But, once mLogsMutex was acquired, sMutex would be released immediately. It is for performance consideration. I would add a line to acquire sMutex before going here to make FreeTraceInfo() more reasonable. @@ +222,5 @@ > UniquePtr<TraceInfo> traceinfo(aTraceInfo); > mozilla::DebugOnly<bool> removed = > sTraceInfos->RemoveElement(traceinfo); > MOZ_ASSERT(removed); > Unused << traceinfo.release(); // A dirty hack to prevent double free. Once I had add an acquiring of |sMutex| in |ShutdownTaskTracer()|, it is no necessary to acquire the |mLogsMutex| here.
Assignee | ||
Comment 9•7 years ago
|
||
hold |sMutex| and release |mLogsMutex| during shutdown.
Attachment #8837481 -
Attachment is obsolete: true
Attachment #8839313 -
Flags: review?(cyu)
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8839313 [details] [diff] [review] Fix memory leaking of TaskTracer, v5 sad... error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
Attachment #8839313 -
Flags: review?(cyu)
Assignee | ||
Comment 11•7 years ago
|
||
remove std::move
Attachment #8839313 -
Attachment is obsolete: true
Attachment #8839332 -
Flags: review?(cyu)
Comment 12•7 years ago
|
||
Comment on attachment 8839332 [details] [diff] [review] Fix memory leaking of TaskTracer, v6 Review of attachment 8839332 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. r+ with the minor changes: ::: tools/profiler/tasktracer/GeckoTaskTracer.cpp @@ +130,5 @@ > +} > + > +inline static void > +ObsoleteCurrentTraceInfos() > +{ Also please assert that sMutex is locked in this function to make it clear. ::: tools/profiler/tasktracer/TracedTaskCommon.cpp @@ +33,5 @@ > > void > TracedTaskCommon::Init() > { > + TraceInfoHolder&& info = GetOrCreateTraceInfo(); In GeckoTaskTracer.cpp, you use "TraceInfoHolder info = GetOrCreateTraceInfo();". Please make it consistent in this file.
Attachment #8839332 -
Flags: review?(cyu) → review+
Assignee | ||
Comment 13•7 years ago
|
||
fix the problem mentioned above.
Attachment #8839332 -
Attachment is obsolete: true
Attachment #8839423 -
Flags: review+
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e8ce45d5536eb6a1cf8e86058fee5b2d337897b
Assignee | ||
Comment 15•7 years ago
|
||
Minor changes for potential dangling pointer error on |sTraceInfos|.
Attachment #8839423 -
Attachment is obsolete: true
Attachment #8839823 -
Flags: review?(cyu)
Assignee | ||
Comment 16•7 years ago
|
||
Attachment #8839823 -
Attachment is obsolete: true
Attachment #8839823 -
Flags: review?(cyu)
Attachment #8839825 -
Flags: review?(cyu)
Updated•7 years ago
|
Attachment #8839825 -
Flags: review?(cyu) → review+
Assignee | ||
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=091a14b50053b61a346d043694b125e2b3f5ba49&selectedJob=79289535 Please checkin attachment 8839825 [details] [diff] [review]. Thank you!
Keywords: checkin-needed
Updated•7 years ago
|
Assignee: nobody → tlee
status-firefox52:
--- → wontfix
status-firefox54:
--- → affected
Keywords: mlk
Comment 18•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/05fcaf50a7b9 Fix memory leaking of TaskTracer. r=cervantes
Keywords: checkin-needed
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05fcaf50a7b9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 20•7 years ago
|
||
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(tlee)
Assignee | ||
Comment 21•7 years ago
|
||
TaskTracer is not default enabled yet. So, we don't need to up-lift it.
Flags: needinfo?(tlee)
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•