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

RESOLVED FIXED in Firefox 53

Status

()

Core
Gecko Profiler
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: dholbert, Assigned: froydnj)

Tracking

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

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?
OS: Linux → All
Hardware: x86_64 → All
No objections at all.
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).)
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.
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.)
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!

Updated

11 months ago
Blocks: 1322863
I have a patch for this.
Assignee: nobody → nfroyd
Created attachment 8823392 [details] [diff] [review]
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.
Attachment #8823392 - Flags: review?(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>(...)?
(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 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.
Attachment #8823392 - Flags: review?(mstange) → review+

Comment 11

11 months ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/603bdc33779f
manage ProfilerBacktrace with UniquePtr; r=mstange

Updated

11 months ago
Depends on: 1329291
https://hg.mozilla.org/mozilla-central/rev/603bdc33779f
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.