1142197 - ProfilerBacktrace instances should be managed via UniquePtr, to make ownership clearer & prevent leaks

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Daniel Holbert [:dholbert]]

Right now the profiler manages ProfilerBacktrace instances like this:
 (A) "Code A" calls profiler_get_backtrace() and sticks the result in a raw ProfilerBacktrace pointer.
 (B) Later on, "Code B" calls profiler_tracing() and passes in that raw pointer. We *must* now null out the raw pointer, because profiler_tracing is now implicitly taking ownership.
 (C) We also need "Code C" cleanup-code, to call profiler_free_backtrace() on the raw pointer, in case "Code B" was never hit.

This is complex, and it's hard to sanity-check that the pointer is being managed correctly here.

Example scenarios:
 - in current mozilla-central, we have raw-pointers mStyleCause & mReflowCause. "Code A" is AddStyleFlushObserver / AddReflowFlushObserver (in RefreshDriver.h). "Code B" is a deeply-nested conditional in nsRefreshDriver::Tick(). "Code C" is ~RefreshDriver
 - In attachment 8575801 [details] [diff] [review] for bug 1129249, "Code A" is RestyleTracker::AddPendingRestyleToTable in RestyleTracker.h. "Code B" is ElementRestyler::ComputeStyleChangeFor() in RestyleManager.cpp. And right now there is no "Code C" for this example, which may be a bug. (I think it's because of an implicit assumption that Code B must always be hit, but I'm not sure yet if that's valid or not.)


If we used UniquePtr instead of raw pointers here, then we'd get the following benefits:
 - Ownership transfers would be immediate & enforced by the type system. (No need for any explicit nulling out in "Code B"; we can make that happen automatically when we pass in our UniquePtr to profiler_free_backtrace().)
 - Cleanup would happen automatically. (No need to worry about "Code C" at all.)
 - As a result, it's easier to skim the code & convince yourself it's correct.

Any thoughts/objections?

Comment 1

[Markus Stange [:mstange]]

No objections at all.

Comment 2

[Daniel Holbert [:dholbert]]

Note that UniquePtr uses "delete" by default, whereas we seem to clean up ProfilerBacktrace instances via "profiler_free_backtrace" (which is really just 'delete' under the hood).

We have the option of using the second UniquePtr template-argument to provide a custom deletion policy (using profiler_free_backtrace), though if possible, my preference would be to just do away with profiler_free_backtrace and just let UniquePtr call "delete" directly.

(AFAICT, the only reason we have profiler_free_backtrace is so we can make it a no-op in builds that have profiling disabled.  This doesn't seem to be a big benefit, though,  because "delete myNullPtr" is also practically a no-op. (just a null-check, presumably).)

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

I'm the original author of the profiler_*_backtrace() functions. I support the conversion to UniquePtr. Also, the profile_free_backtrace was essentially there to enforce a convention allocation and deallocation be abstracted away. OTOH by returning a UniquePtr from profiler_get_backtrace, the deallocation mechanism is already encapsulated.

r+ from me to eliminate profiler_free_backtrace when converting to UniquePtr.

Comment 4

[Daniel Holbert [:dholbert]]

Great! (& thanks for the backstory! I was worried there might be a deeper reason we need to keep profiler_free_backtrace(), but I'm glad that's not the case.)

Comment 5

[Daniel Holbert [:dholbert]]

I probably don't have cycles to jump on this at the moment; just wanted to make sure it was a sane idea & that it's on the to-do list.

If anyone else wants to take this, feel free!

Comment 6

[Nathan Froyd [:froydnj]]

I have a patch for this.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: manage ProfilerBacktrace with UniquePtr

This patch applies (for me) on top of the patches in bug 1322863; it's possible
that this patch builds just fine without the other ones, but I have not tested
it.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8823392 [details] [diff] [review]
manage ProfilerBacktrace with UniquePtr

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

Since the destructor just calls delete anyway, is there a reason you're not just using UniquePtr<ProfilerBacktrace> and MakeUnique<ProfileBacktrace>(...)?

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Markus Stange [:mstange] from comment #8)
> Since the destructor just calls delete anyway, is there a reason you're not
> just using UniquePtr<ProfilerBacktrace> and
> MakeUnique<ProfileBacktrace>(...)?

I *think* the reason was that the definition of ProfilerBacktrace isn't necessarily visible in all the places that ProfilerBacktrace gets constructed, and calling |delete| on such instances doesn't work/undefined behavior/triggers warnings/etc.  Doing things this way lets indirection take care of such problems for us.  I am open to making things less complicated, though...

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8823392 [details] [diff] [review]
manage ProfilerBacktrace with UniquePtr

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

Ok, if that is the case, sure. This patch is an improvement either way.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/603bdc33779f
manage ProfilerBacktrace with UniquePtr; r=mstange

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/603bdc33779f