855662 - SPS breakpad: free breakpad-allocated memory when unwinder thread shuts down

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Comment 1

[Julian Seward [:jseward]]

tools/profiler/UnwinderThread2.cpp: sSymbolizer and sModules are never freed,
resulting in ~200MB of leaked space when the unwinder thread shuts down.

Comment 2

[Julian Seward [:jseward]]

Attachment: Patch

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 730630 [details] [diff] [review]
Patch

Review of attachment 730630 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/UnwinderThread2.cpp
@@ +117,5 @@
>  // sampled.  So that we know the max safe stack address for each
>  // registered thread.
>  static void thread_register_for_profiling ( void* stackTop );
>  
> +// Frees some memory when the unwinder thread is shut down.

s/some memory/the singleton

@@ +823,5 @@
> +         down.  The buffers themselves are not protected by the
> +         spinlock, so we have no way to stop them being accessed
> +         whilst we free them.  It doesn't matter much since they will
> +         not be reallocated if a new unwinder thread is restarted
> +         later. */

Why don't we release these when we return the full buffer?

Comment 4

[Julian Seward [:jseward]]

(In reply to Benoit Girard (:BenWa) from comment #3)
> @@ +823,5 @@
> > +         down.  The buffers themselves are not protected by the
> > +         spinlock, so we have no way to stop them being accessed
> > +         whilst we free them.  It doesn't matter much since they will
> > +         not be reallocated if a new unwinder thread is restarted
> > +         later. */
> 
> Why don't we release these when we return the full buffer?

The problem case is where the sampling thread has taken a buffer and
is filling it up, and at the same time the unwinder thread exits.
Then it is impossible to know when the sampling thread has finished
with the buffer so it can be deallocated.

This is a side effect of the design choice that the sampling thread
doesn't know or care if the unwinder thread is dead or alive.  If the
unwinder thread stops emptying out buffers, that might be because it
is busy doing other stuff (reading libxul.so's CFI) or because it has
exited.  The sampling thread can't find out which.

It might be possible to fix this, but I think it would add the need
for more synchronisation and more difficulty in proving it correct.
In any case this one-time leak is about 350KB and only occurs when
profiling, so it's not too bad.

Comment 5

[Benoit Girard (:BenWa)]

I see. That's fine for now.

Comment 6

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7aaa967ad3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a7aaa967ad3