Closed Bug 529134 Opened 11 years ago Closed 11 years ago

Change the way tracemalloc is initialized

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file)

According to bsmedberg, it should be

 (1) initialized as part of NS_LogInit()
 (2) choose its log files through the environment rather than command-line flags
 (3) have its symbols be libxul-internal rather than exported

Also it should be closed down with NS_LogTerm().
I just hit bug 525573 for an e10s unit test (IPDL), for the first time.  This is pretty scary; it means all the non-firefox-bin e10s programs, including child processes, are susceptible to this threadsafety problem in tracemalloc builds and we're playing with fire.  Definitely need to hook this into LogInit()/LogTerm()
Partial implementation of comment 0 ... frankly, the most important part.  The other steps can be completed later.
Assignee: nobody → jones.chris.g
Attachment #413463 - Flags: review?(dbaron)
Comment on attachment 413463 [details] [diff] [review]
"Start" trace-malloc in LogInit(), it hasn't already been started

Not sure whose realm this falls into.
Attachment #413463 - Flags: review?(benjamin)
Duplicate of this bug: 525573
After looking at the existing code, I'm curious what threadsafety problems you're running into that result from not having started trace-malloc.

If you don't call anything in trace-malloc, then:
 * we should never hook malloc at all on Mac and Windows (StartupHooker should never be called)
 * the Linux malloc hooks should all return almost immediately on discovering tracing_enabled is false

Is that not actually the case?  Why is it necessary to start trace-malloc?
The new deadlock detector uses trace-malloc to get backtraces in DEBUG builds.
I should add that the old deadlock detector also used trace-malloc to get backtraces.  It was just by luck that a multi-threaded XPCOM unit test never went into an infinite loop like I've seen.
I'd actually written this before seeing your last two comments:

Or is the threadsafety problem you're hitting related to callers of NS_TraceMallocGetStackTrace() ?  Perhaps a better fix for those issues would be to make the tm_thread* argument to backtrace() be optional, since there's no need to suppress tracing or log if trace-malloc is disabled (I'm not particularly happy with what I did in bug 442192, and that would probably be a better solution).
Oh, I guess calltree actually needs the lock.  So I guess this is the way to go.
Comment on attachment 413463 [details] [diff] [review]
"Start" trace-malloc in LogInit(), it hasn't already been started

r=dbaron

I guess the idea of exposing NS_TraceMallocStackTrace() to callers outside trace-malloc was a clever idea that wasn't actually as simple as it looked.  Oh well; I think it's still useful.
Attachment #413463 - Flags: review?(dbaron) → review+
Attachment #413463 - Flags: review?(benjamin) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/7c5e126880cf
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.