Mismatched free() of MessageLoop

RESOLVED FIXED

Status

()

Core
IPC
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

({valgrind})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

==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.
Created attachment 431253 [details] [diff] [review]
Use mozalloc in chromium code.

base/basictypes.h r? to bent, jscntxt.h r? to brendan.
Assignee: nobody → jones.chris.g
Attachment #431253 - Flags: review?(bent.mozilla)
Attachment #431253 - Flags: review?(brendan)
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
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?
Macros are nasty. Is there no linker way?

/be
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.
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
(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
(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.
Must you use macros, though? Or could you use global ::malloc, etc. inlines that call _weak names?

Macros are nasty.

/be
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.
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.
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.
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.
Created attachment 433101 [details]
Abstraction of force-inline wrapper scheme

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.
Confirmed that the test program works fine in gcc, with s/__declspec(dllexport)// and s/__forceinline/__attribute__((always_inline)) inline/.
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.
Created attachment 433111 [details] [diff] [review]
WIP to use force-inline malloc et al. instead of macro hackery

Hopefully someone can find the right incantations to resurrect this.
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!
Created attachment 433132 [details] [diff] [review]
Allow mozalloc macros to be undef'd, use that in jscntxt.h, and use mozalloc in chromium

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

Comment 23

8 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+
http://hg.mozilla.org/mozilla-central/rev/402192edce61
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Attachment #433132 - Flags: review?(brendan)

Comment 25

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