529134 - Change the way tracemalloc is initialized

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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().

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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()

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: "Start" trace-malloc in LogInit(), it hasn't already been started

Partial implementation of comment 0 ... frankly, the most important part.  The other steps can be completed later.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The new deadlock detector uses trace-malloc to get backtraces in DEBUG builds.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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).

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Oh, I guess calltree actually needs the lock.  So I guess this is the way to go.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/mozilla-central/rev/7c5e126880cf