Trace malloc doesn't work on linux with standalone xpcom glue

RESOLVED FIXED in mozilla7

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla7
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
This was discovered after bug 552864 landed. There are basically two problems:
- Some libxul functions are registered with atexit(), and that is bound to fail after libxul is unloaded. Somehow, this is not a problem on windows and mac.
- malloc/free/... functions aren't diverted with the ones from nsTraceMalloc.c because they are already resolved to libc's when libxul is loaded.
(Assignee)

Comment 1

6 years ago
Created attachment 534438 [details] [diff] [review]
part 1 - Properly divert memory allocation functions for trace malloc with standalone glue on Linux

This makes use of libc's malloc/free/... functions until libxul is loaded, and reverts to these after libxul is unloaded.
When libxul is loaded, use libxul's functions
Attachment #534438 - Flags: review?(benjamin)
(Assignee)

Comment 2

6 years ago
Created attachment 534440 [details] [diff] [review]
part 2 - Use base::AtExitManager instead of atexit()

Use base::AtExitManager it is possible to avoid problems that would arise with atexit() after libxul is unloaded.
The RegisterAtExitCallback is required in nsTraceMalloc.c because we can't call base::AtExitManager::RegisterCallback directly from C code.
We need to initialize an AtExitManager before NS_LogInit() in NS_InitXPCOM2 because NS_LogInit ends up trying to register a callback. We also add an AtExitManager in XRE_main because the profile lock registers a callback too, and it does so before NS_InitXPCOM2. Then, the one in NS_InitXPCOM2 becomes redundant, but we have a bunch of executables that do NS_InitXPCOM2 without ever calling XRE_main.
Attachment #534440 - Flags: review?(benjamin)
(Assignee)

Comment 3

6 years ago
Pushed to try:
http://tbpl.mozilla.org/?tree=Try&rev=a8522d54f80b
(Assignee)

Updated

6 years ago
Attachment #534438 - Flags: review?(benjamin) → review?(dbaron)
Under what conditions do we use standalone XPCOM glue?  i.e., what cases does this bug affect?

When does the AtExitManager stuff run (relative to when main() exits)?  For some of this, we really want it to run after main() is done, or at the very least after XRE_main() is done.

Casting function pointers is a sign that something is wrong.  Why do you need to do that in nsStandardURL.cpp in patch 2?
(Assignee)

Comment 5

6 years ago
(In reply to comment #4)
> Under what conditions do we use standalone XPCOM glue?  i.e., what cases
> does this bug affect?

bug 552864 switches Firefox to use the standlone XPCOM glue to allow preloading of the libraries (bug 	632404) for massive cold startup improvement.

> When does the AtExitManager stuff run (relative to when main() exits)?  For
> some of this, we really want it to run after main() is done, or at the very
> least after XRE_main() is done.

The way the AtExitManager stuff works, it runs the callbacks when destroying the AtExitManager instance. In the most common case, it means when exiting XRE_main.

> Casting function pointers is a sign that something is wrong.  Why do you
> need to do that in nsStandardURL.cpp in patch 2?

Because AtExitManager callbacks take a void * parameter we don't use. I could modify the callbacks to take one, though.
(Assignee)

Comment 6

6 years ago
(In reply to comment #5)
> The way the AtExitManager stuff works, it runs the callbacks when destroying
> the AtExitManager instance. In the most common case, it means when exiting
> XRE_main.

I'm wondering if using a static initializer wouldn't make more sense, actually...
Comment on attachment 534440 [details] [diff] [review]
part 2 - Use base::AtExitManager instead of atexit()

Ugh, more chromium code we'll have to rip out later :-(
Attachment #534440 - Flags: review?(benjamin) → review+
(Assignee)

Comment 8

6 years ago
Apparently, MSVC doesn't like what chromium-config.mk adds:

nsDebugHelpWin32.cpp

e:/builds/moz2_slave/try-w32-dbg/build/tools/trace-malloc/lib/nsDebugHelpWin32.cpp(73) : error C2664: 'LoadLibraryW' : cannot convert parameter 1 from 'const char [12]' to 'LPCWSTR'

        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

e:/builds/moz2_slave/try-w32-dbg/build/tools/trace-malloc/lib/nsDebugHelpWin32.cpp(275) : error C2664: 'lstrcmpiW' : cannot convert parameter 1 from 'const char *' to 'LPCWSTR'

        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
chromium-config.mk defined UNICODE ... you can just change those calls from ::Foo() to :FooA.
(In reply to comment #5)
> > Casting function pointers is a sign that something is wrong.  Why do you
> > need to do that in nsStandardURL.cpp in patch 2?
> 
> Because AtExitManager callbacks take a void * parameter we don't use. I
> could modify the callbacks to take one, though.

Please do.  That's much better than casting function pointers and making assumptions about calling conventions.
Comment on attachment 534438 [details] [diff] [review]
part 1 - Properly divert memory allocation functions for trace malloc with standalone glue on Linux

ok, r=dbaron
Attachment #534438 - Flags: review?(dbaron) → review+
(Assignee)

Comment 12

6 years ago
Created attachment 534730 [details] [diff] [review]
part 2 - Use static destructors instead of atexit()

This one doesn't rely on base::AtExitManager at all. It relies on static destructors.
While this approach works very well, it's however not very obvious when reading the code what it is doing. I'd like to find a way to make that into nice templates or macros so that documentation could lie in a single location, but that might not be possible at all.
Attachment #534730 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Attachment #534440 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Corresponding try build:
http://tbpl.mozilla.org/?tree=Try&rev=900ee2027179
Comment on attachment 534730 [details] [diff] [review]
part 2 - Use static destructors instead of atexit()

Ugh, but ok.
Attachment #534730 - Flags: review?(benjamin) → review+
(Assignee)

Comment 15

6 years ago
Created attachment 539435 [details] [diff] [review]
part 1 - Properly divert memory allocation functions for trace malloc with standalone glue on Linux.

Refreshed to apply before bug 632404
(Assignee)

Updated

6 years ago
Attachment #534438 - Attachment is obsolete: true
(Assignee)

Comment 16

6 years ago
Created attachment 539436 [details] [diff] [review]
part 2 - Use static destructors instead of atexit().

As will land (added comments)
(Assignee)

Updated

6 years ago
Attachment #534730 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/6b23b52e68d6
http://hg.mozilla.org/integration/mozilla-inbound/rev/6e230988614f
Assignee: nobody → mh+mozilla
Whiteboard: [inbound]
I merged this from m-i to m-c, but it will be backed out shortly while we investigate Talos regressions:
http://hg.mozilla.org/mozilla-central/rev/6e230988614f
http://hg.mozilla.org/mozilla-central/rev/6b23b52e68d6
(Assignee)

Comment 19

6 years ago
Created attachment 539739 [details] [diff] [review]
part 1 - Properly divert memory allocation functions for trace malloc with standalone glue on Linux.

I figured what went wrong with the leak reports: posix_memalign wasn't diverted. I thus added posix_memalign and cfree, which are the two functions that nsTraceMalloc.c exports and that we weren't. I don't think we use cfree from anywhere, but at least, we on par with what nsTraceMalloc.c diverts. The interdiff with the previous version is simple enough (and mostly copied from nsTraceMalloc.c) that I don't need review.
(Assignee)

Updated

6 years ago
Attachment #539435 - Attachment is obsolete: true
(Assignee)

Comment 20

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/eef5d644e17e
http://hg.mozilla.org/integration/mozilla-inbound/rev/f4ddad2c0eb7
http://hg.mozilla.org/mozilla-central/rev/eef5d644e17e
http://hg.mozilla.org/mozilla-central/rev/f4ddad2c0eb7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.