507082 - Modify Gecko code to use the "infallible malloc" library

Status: RESOLVED DUPLICATE of bug 441324

(Core :: XPCOM, enhancement)

Description

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

The library (libmozalloc) exports a header that defines moz_* variants of malloc, realloc, et al., along with ::operator new's.  The library will also |#define malloc moz_xmalloc| by default.  See bug 441324.

The work for this bug is to ensure that this header is being included where necessary and libmozalloc is being linked into all Gecko libraries.  No Gecko-originating libraries should directly use the malloc/realloc/etc. symbols; they should instead explicitly use the moz_[sym] or moz_x[sym] variants.  Whether our imported libraries (e.g. libpng) use these allocators should be up to the module owners.

Comment 1

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

Attachment: v1, mozilla-central part

This patch touches some C++ headers in XPConnect.  JSContexts use the identifiers "malloc", "calloc", and "free" as field names, but since we're #define'ing these tokens, "jscnxt.h" can't be included after "mozalloc.h"  This seems trivial enough, but please advise if XPConnect review is necessary.

Comment 2

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

Attachment: v1, mobile-browser part

Need to link against libmozalloc when building libmozphone.  r? to bsmedberg b/c it's a build change.

NB: this needs to be committed "atomically" with the above patch or there's a window when builds might fail.  I think that this patch should go first, b/c I /think/ that make will expand the |MOZALLOC_LIB| to empty if it's undefined.

Comment 3

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

Comment on attachment 391812 [details] [diff] [review]
v1, mozilla-central part

Must have obsoleted this without thinking, oops.

Comment 4

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

Attachment: v1, mozilla-central part

In all the "excitement" of fixing linker errors, I forgot to change the implementation of NS_alloc/_realloc!  Included here.  (Note that on the advice of bsmedberg I did not attempt to modify nsCRT.)

Comment 5

[Benjamin Smedberg]

Comment on attachment 391904 [details] [diff] [review]
v1, mozilla-central part

If you add MOZALLOC_LIB to XPCOM_GLUE_LDOPTS/XPCOM_FROZEN_LDOPTS/XPCOM_LIBS you'll avoid a lot of the custom makefile patchery here (and make me happy).

>diff --git a/toolkit/mozapps/update/src/updater/Makefile.in b/toolkit/mozapps/update/src/updater/Makefile.in

> LIBS += \
> 	$(DEPTH)/modules/libmar/src/$(LIB_PREFIX)mar.$(LIB_SUFFIX) \
> 	$(BZ2_LIBS) \
>+	$(MOZALLOC_LIB) \
> 	$(NULL)

Updater must not use mozalloc (because it is responsible for updating mozalloc, and it can't have the file open while updating it).


>--- a/widget/src/xremoteclient/Makefile.in
>+++ b/widget/src/xremoteclient/Makefile.in
>@@ -72,16 +72,17 @@ endif
> PROGRAM         = mozilla-xremote-client$(BIN_SUFFIX)
> 
> PROGOBJS        = mozilla-xremote-client.$(OBJ_SUFFIX) \
> 		XRemoteClient_standalone.$(OBJ_SUFFIX) \
> 		$(NULL)
> 
> LIBS            = \
> 		$(NSPR_LIBS) \
>+		$(MOZALLOC_LIB) \
> 		$(XLDFLAGS) $(XLIBS)

I *think* mozilla-xremote-client is installed into /usr/bin in the linux distros which still build it, and we shouldn't make it depend on mozalloc.

>diff --git a/xpcom/sample/program/Makefile.in b/xpcom/sample/program/Makefile.in

> LIBS            = \
> 		$(XPCOM_STANDALONE_GLUE_LDOPTS) \
>+		$(MOZALLOC_LIB) \
> 		$(NULL)

Anything which uses the standalone glue must not use mozalloc (it can't dynamically link against the mozilla libraries at all, because it doesn't know where they are located yet). That also means that in nscore.h if XPCOM_GLUE is defined we shouldn't include mozalloc.h, and we may not be allowed to use the infallible functions in xpcom/glue.

>diff --git a/xulrunner/stub/Makefile.in b/xulrunner/stub/Makefile.in

More standalone glue here.

Comment 6

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

Attachment: v2

Comment 7

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

Comment on attachment 395211 [details] [diff] [review]
v2

Something broke badly in between v1 and v2.  Hunting down.

Comment 8

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

Comment on attachment 395211 [details] [diff] [review]
v2

The problem was elsewhere

Comment 9

[Benjamin Smedberg]

Comment on attachment 395211 [details] [diff] [review]
v2

>diff --git a/configure.in b/configure.in

>-DYNAMIC_XPCOM_LIBS='-L$(LIBXUL_DIST)/bin -lxpcom -lxpcom_core'
>+DYNAMIC_XPCOM_LIBS='-L$(LIBXUL_DIST)/bin -lxpcom -lxpcom_core $(MOZALLOC_LIB)'

I believe there are other locations in configure.in which will also need to be updated, such as the MSVC-specific section (because MSVC uses import libs and doesn't recognize the -Ldir -llib syntax).

>diff --git a/xpcom/base/nscore.h b/xpcom/base/nscore.h

>+/* Definitions of functions and operators that allocate memory. */
>+#if defined(MOZILLA_INTERNAL_API) && !defined(MOZ_NO_MOZALLOC)
>+#  include "mozilla/mozalloc.h"
>+#endif

I may have misled you at some point on IRC: I think it's ok for xpcom/glue to link against mozalloc and use the functions, just not xpcom/glue/standalone. So I think this should read:

#if !defined(XPCOM_GLUE) && !defined(MOZ_NO_MOZALLOC)

>--- a/xpcom/stub/Makefile.in

> ifeq ($(OS_TARGET),OS2)
> EXTRA_DSO_LIBS = xpcomcor
> DEPENDENT_LIBS_LIST += xpcomcor.dll
> else
> EXTRA_DSO_LIBS = xpcom_core
>-DEPENDENT_LIBS_LIST += $(LIB_PREFIX)xpcom_core$(DLL_SUFFIX)
>+DEPENDENT_LIBS_LIST += \
>+	$(LIB_PREFIX)xpcom_core$(DLL_SUFFIX) \
>+	$(LIB_PREFIX)mozalloc$(DLL_SUFFIX) \
>+	$(NULL)
> endif

mozalloc needs to go before xpcom_core here, and did you miss an OS2 block?

r=me with those changes

Comment 10

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

Bug 511568 landing makes this rather more complicated.  I'll r? vlad for the forthcoming wince hacks.

Comment 11

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

Not much point in doing this in two stages anymore.