551071 - Mismatched free() of MessageLoop

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

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

Comment 1

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

Attachment: Use mozalloc in chromium code.

base/basictypes.h r? to bent, jscntxt.h r? to brendan.

Comment 2

[Brendan Eich [: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

Comment 3

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

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

[Brendan Eich [:brendan]]

Macros are nasty. Is there no linker way?

/be

Comment 5

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

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

[Brendan Eich [:brendan]]

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

[Brendan Eich [:brendan]]

(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

Comment 8

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

(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

[Brendan Eich [:brendan]]

Must you use macros, though? Or could you use global ::malloc, etc. inlines that call _weak names?

Macros are nasty.

/be

Comment 10

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

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.

Comment 11

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

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.

Comment 12

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

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.

Comment 13

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

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.

Comment 14

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

Attachment: 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.

Comment 15

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

Confirmed that the test program works fine in gcc, with s/__declspec(dllexport)// and s/__forceinline/__attribute__((always_inline)) inline/.

Comment 16

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

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.

Comment 17

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

Attachment: WIP to use force-inline malloc et al. instead of macro hackery

Hopefully someone can find the right incantations to resurrect this.

Comment 18

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

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!

Comment 19

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

Attachment: Allow mozalloc macros to be undef'd, use that in jscntxt.h, and use mozalloc in chromium

Sigh.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 21

[Brendan Eich [: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 just threw up in my mouth a little.

Waiting for Benjamin to r+ or save us with a better hack.

/be

Comment 22

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

Review ping.

Comment 23

[Benjamin Smedberg]

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

Comment 24

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

http://hg.mozilla.org/mozilla-central/rev/402192edce61

Comment 25

[Radek 'sysKin' Czyz]

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