Closed
Bug 551071
Opened 15 years ago
Closed 15 years ago
Mismatched free() of MessageLoop
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: cjones, Assigned: cjones)
Details
(Keywords: valgrind)
Attachments
(3 files, 1 obsolete file)
1.56 KB,
application/zip
|
Details | |
17.19 KB,
patch
|
Details | Diff | Splinter Review | |
4.35 KB,
patch
|
benjamin
:
review+
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
==14735== Thread 2:
==14735== Mismatched free() / delete / delete []
==14735== at 0x4C24A7A: operator delete(void*) (vg_replace_malloc.c:346)
==14735== by 0x6FFF680: base::MessagePumpLibevent::~MessagePumpLibevent() (message_pump_libevent.cc:144)
==14735== by 0x6FA7B25: base::RefCountedThreadSafe<base::MessagePump>::Release() (ref_counted.h:107)
==14735== by 0x6FA6E8A: scoped_refptr<base::MessagePump>::~scoped_refptr() (ref_counted.h:196)
==14735== by 0x6FA54DA: MessageLoop::~MessageLoop() (message_loop.cc:159)
==14735== by 0x6FCCC8D: base::Thread::ThreadMain() (thread.cc:175)
==14735== by 0x70004D6: ThreadFunc(void*) (platform_thread_posix.cc:26)
==14735== by 0x4E30A03: start_thread (pthread_create.c:300)
==14735== by 0x970080C: clone (clone.S:112)
==14735== Address 0x118fc6d0 is 0 bytes inside a block of size 48 alloc'd
==14735== at 0x4C25153: malloc (vg_replace_malloc.c:195)
==14735== by 0x524CECD: moz_xmalloc (mozalloc.cpp:75)
==14735== by 0x6FA4FA3: MessageLoop::MessageLoop(MessageLoop::Type) (mozalloc.h:216)
==14735== by 0x6FCCB27: base::Thread::ThreadMain() (thread.cc:140)
==14735== by 0x70004D6: ThreadFunc(void*) (platform_thread_posix.cc:26)
==14735== by 0x4E30A03: start_thread (pthread_create.c:300)
==14735== by 0x970080C: clone (clone.S:112)
==14735==
This could happen a fair bit with memory going across the chromium/Gecko barrier. I think we should be using libmozalloc in chromium code.
Assignee | ||
Comment 1•15 years ago
|
||
base/basictypes.h r? to bent, jscntxt.h r? to brendan.
Assignee: nobody → jones.chris.g
Attachment #431253 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•15 years ago
|
Attachment #431253 -
Flags: review?(brendan)
Comment 2•15 years ago
|
||
The C preprocessor rescans macros bodies only when expanding macro calls, so that won't work:
$ /usr/bin/cpp
#define malloc foo
#define save_malloc malloc
#undef malloc
#define malloc bar
save_malloc
malloc
# 1 "<stdin>"
# 1 "<built-in>"
# 1 "<command line>"
# 1 "<stdin>"
bar
bar
Can we get some better support in jscntxt.h for what you want?
/be
Assignee | ||
Comment 3•15 years ago
|
||
Oops, what I wanted was pragma push/pop_macro().
The only other option I see (given these sucky #define's) is globally removing the use of malloc/calloc/free identifiers in js/src, which seems like a bigger and more annoying patch. Is that preferable?
Comment 4•15 years ago
|
||
Macros are nasty. Is there no linker way?
/be
Assignee | ||
Comment 5•15 years ago
|
||
ld's --wrap is exactly what we want, but OS X gcc doesn't support it and MSVC doesn't have an equivalent. LD_PRELOAD isn't right because that overrides the malloc that JS/NSPR/etc. use, and MSVC doesn't have that either. Then there's the myriad of other squirrelly compilers we support to various degrees. I'm open to ideas, but bug 441324 is the only common-denominator solution I'm aware of.
Comment 6•15 years ago
|
||
Perhaps there's a non-macro way for JS, at least, that doesn't touch too many lines. Something like js::malloc, etc. (with enough "using namespace js;"), calling ::malloc.
/be
Comment 7•15 years ago
|
||
(In reply to comment #3)
> Oops, what I wanted was pragma push/pop_macro().
>
> The only other option I see (given these sucky #define's) is globally removing
> the use of malloc/calloc/free identifiers in js/src, which seems like a bigger
> and more annoying patch. Is that preferable?
In other words, "remove" the use of malloc/calloc/free by namespacing. If the patch is small enough and using namespace js; is done well (top of each .cpp file after the #includes? not sure), this could work.
/be
Assignee | ||
Comment 8•15 years ago
|
||
(In reply to comment #6)
> Perhaps there's a non-macro way for JS, at least, that doesn't touch too many
> lines. Something like js::malloc, etc. (with enough "using namespace js;"),
> calling ::malloc.
>
> /be
The problem is that class ContextAllocPolicy has members named malloc/free/realloc, so when those identifiers are #define'd by mozalloc.g cpp tries to expand them and finds the "wrong number of arguments". There was something similar in a nanojit header IIRC.
Comment 9•15 years ago
|
||
Must you use macros, though? Or could you use global ::malloc, etc. inlines that call _weak names?
Macros are nasty.
/be
Assignee | ||
Comment 10•15 years ago
|
||
Hm. g++ -Wall didn't complain and did the right thing with
-----
#include <stdlib.h>
extern void* moz_malloc(size_t);
inline void* malloc(size_t n) {
return moz_malloc(n);
}
int main()
{
char* p = (char*)malloc(10);
p[0] = 'a';
return 0;
}
-----
Maybe this approach *can* work, if we can get all our linkage ducks in a row on all platforms. I'll cook up a patch later and see what tryserver has to say.
Assignee | ||
Comment 11•15 years ago
|
||
I remember now why we didn't take this approach: force-inline functions in C sources are still defined in their object files, which leads to multiple definition errors. We have few enough C sources that it's probably worth just special-casing them.
Assignee | ||
Comment 12•15 years ago
|
||
Got this working on the interesting linux configs (debug/non-libxul/trace-malloc, opt/libxul/jemalloc). Windows/mac/wince tomorrow :(.
Biggest obstacle was a nasty gotcha. In C++ code using the force-inline "free" as a function pointer, gcc emitted a reference to a special weak "W free" symbol (in nm terms). For firefox-bin/libxul/jemalloc, this was OK, because firefox-bin itself defines "free" and the weak dependent apparently resolved to that definition. In other binaries (i.e. unit tests), we don't use jemalloc, and so that "free" definition wasn't available. However, instead of resolving to libc free, this weak free was somehow resolving to the force-inline mozalloc free, and globally too! So the dynamic linker immediately went into an infinite loop on free()->moz_free()->free()->... Luckily we can check for this in the future with a simple script, on linux.
The fix was to directly use moz_free instead of free, but this makes me very nervous about the other platforms. Guess we'll see what shakes out.
Assignee | ||
Comment 13•15 years ago
|
||
Update 2: OS X seems to be OK.
MSVC doesn't :(. My force-inline malloc wrapper is being defined in the .obj's, and its definition is being used instead of CTR malloc at runtime (leading to iloop). Checking with people who know more about MSVC than I for possible workarounds.
Assignee | ||
Comment 14•15 years ago
|
||
This is what's ilooping for MSVC. Going to poke around the docs for workarounds, but help from someone more knowledgeable would be very welcome.
Assignee | ||
Comment 15•15 years ago
|
||
Confirmed that the test program works fine in gcc, with s/__declspec(dllexport)// and s/__forceinline/__attribute__((always_inline)) inline/.
Assignee | ||
Comment 16•15 years ago
|
||
According to http://msdn.microsoft.com/en-us/library/z8y1yy88.aspx, there are cases where MSVC still won't force-inline malloc/free et al., but it's not clear yet whether any mozilla code or build configuration falls into those categories.
Unfortunately, I confirmed that it's the earlier declaration of malloc() in stdlib.h that leads to force-inline malloc() being defined in the .obj files. If I drop the stdlib.h include (except in mozalloc.cpp), all works fine. That's still a deal breaker IMHO, because it would mean we couldn't include any header that potentially includes <stdlib.h> before or after mozalloc.h (there's a different error if <stdlib.h> is included after mozalloc.h). Please tell me someone has a better idea!
In the mean time, I'll do the macro hackery fix.
Assignee | ||
Comment 17•15 years ago
|
||
Hopefully someone can find the right incantations to resurrect this.
Assignee | ||
Comment 18•15 years ago
|
||
So, more fun ... my linux gcc 4.4 claims not to recognize #pragma push/pop_macro(), though gcc claims to support it (http://gcc.gnu.org/onlinedocs/gcc/Push_002fPop-Macro-Pragmas.html#Push_002fPop-Macro-Pragmas). Maybe this is some sort of mingw-only extension. Grosser hacks coming up!
Assignee | ||
Comment 19•15 years ago
|
||
Sigh.
Attachment #431253 -
Attachment is obsolete: true
Attachment #433132 -
Flags: review?(bent.mozilla)
Attachment #433132 -
Flags: review?(benjamin)
Attachment #431253 -
Flags: review?(brendan)
Attachment #431253 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•15 years ago
|
Attachment #433132 -
Flags: review?(brendan)
Comment on attachment 433132 [details] [diff] [review]
Allow mozalloc macros to be undef'd, use that in jscntxt.h, and use mozalloc in chromium
I guess I'm ok with this if we can't do it some other way...
Attachment #433132 -
Flags: review?(bent.mozilla) → review+
Comment 21•15 years ago
|
||
Comment on attachment 433132 [details] [diff] [review]
Allow mozalloc macros to be undef'd, use that in jscntxt.h, and use mozalloc in chromium
I just threw up in my mouth a little.
Waiting for Benjamin to r+ or save us with a better hack.
/be
Assignee | ||
Comment 22•15 years ago
|
||
Review ping.
Comment 23•15 years ago
|
||
Comment on attachment 433132 [details] [diff] [review]
Allow mozalloc macros to be undef'd, use that in jscntxt.h, and use mozalloc in chromium
Ugly, but I can't think of anything better right now :-(
Attachment #433132 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 24•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Attachment #433132 -
Flags: review?(brendan)
Comment 25•15 years ago
|
||
(In reply to comment #11)
> force-inline functions in C
> sources are still defined in their object files
They shouldn't be if they're also static.
You need to log in
before you can comment on or make changes to this bug.
Description
•