Closed Bug 1329929 Opened 3 years ago Closed 3 years ago

Fix memory leaking for TaskTracer

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- disabled
firefox53 --- disabled
firefox54 --- fixed

People

(Reporter: sinker, Assigned: sinker)

References

Details

(Keywords: memory-leak)

Attachments

(1 file, 9 obsolete files)

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.
Attached patch Fix memory leaking (obsolete) — Splinter Review
Attachment #8825327 - Flags: review?(cyu)
Attachment #8825327 - Flags: review?(cyu) → review+
Attached patch Fix memory leaking, v2 (obsolete) — Splinter Review
--
Attachment #8825327 [details] [diff] - Attachment is obsolete: true
Attachment #8826841 - Flags: review?(cyu)
Attachment #8825327 - Attachment is obsolete: true
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.
Attached patch Fix memory leaking of TaskTracer (obsolete) — Splinter Review
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)
Attachment #8835445 - Flags: review?(cyu)
Attachment #8835445 - Attachment is obsolete: true
Attachment #8836027 - Flags: review?(cyu)
remove spaces.
Attachment #8836027 - Attachment is obsolete: true
Attachment #8836027 - Flags: review?(cyu)
Attachment #8837481 - Flags: review?(cyu)
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-
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.
hold |sMutex| and release |mLogsMutex| during shutdown.
Attachment #8837481 - Attachment is obsolete: true
Attachment #8839313 - Flags: review?(cyu)
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)
remove std::move
Attachment #8839313 - Attachment is obsolete: true
Attachment #8839332 - Flags: review?(cyu)
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+
fix the problem mentioned above.
Attachment #8839332 - Attachment is obsolete: true
Attachment #8839423 - Flags: review+
Minor changes for potential dangling pointer error on |sTraceInfos|.
Attachment #8839423 - Attachment is obsolete: true
Attachment #8839823 - Flags: review?(cyu)
Attachment #8839823 - Attachment is obsolete: true
Attachment #8839823 - Flags: review?(cyu)
Attachment #8839825 - Flags: review?(cyu)
Attachment #8839825 - Flags: review?(cyu) → review+
Assignee: nobody → tlee
Keywords: mlk
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05fcaf50a7b9
Fix memory leaking of TaskTracer. r=cervantes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/05fcaf50a7b9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(tlee)
TaskTracer is not default enabled yet. So, we don't need to up-lift it.
Flags: needinfo?(tlee)
You need to log in before you can comment on or make changes to this bug.