634155 - NewCompartment should account memory using gcMallocBytes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

Otherwise we don't GC when making a lot of sandboxes (or compartments in general).

Comment 1

[Andreas Gal :gal]

This is trivial. I would take a reviewed patch but not blocking on it.

Comment 2

[Andreas Gal :gal]

Paul want to take a stab at this? You and nick were looking at the allocation patterns in the engine (js_new<> and such).

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch

Is this on the right track?

NewCompartment() currently has two "return NULL" paths in which the allocated compartment is not freed.  That looks wrong.

Comment 4

[Paul Biggar]

Sure, I'll take it over, if njn doesn't mind. Is njn's patch all you want, or is there more memory that isn't being accounted?

(I'll add the OOM checking too.)

Comment 5

[Nicholas Nethercote [inactive]]

It's all yours!

Comment 6

[Andreas Gal :gal]

Thanks to both of you. I think JSCompartment is the much smaller part. We also should account for the tracer and JM and the property tree.

Comment 7

[Andreas Gal :gal]

(basically all the js_new stuff is not accounted for right now)

Comment 8

[Paul Biggar]

(In reply to comment #7)
> (basically all the js_new stuff is not accounted for right now)

Ah, of course. So js_new etc calls js_malloc directly, instead of calling cx->malloc?

Comment 9

[Andreas Gal :gal]

Yeah

Comment 10

[Paul Biggar]

(In reply to comment #8)
> (In reply to comment #7)
> > (basically all the js_new stuff is not accounted for right now)
> 
> Ah, of course. So js_new etc calls js_malloc directly, instead of calling
> cx->malloc?

Here's roughly my plan. Thoughts njn?

-#define JS_NEW_BODY(t, parms)                                                 \
-    void *memory = js_malloc(sizeof(t));                                      \
+#define JS_NEW_BODY(t, parms, cx)                                             \
+    void *memory = cx ? cx->malloc(sizeof(t)) : js_malloc(sizeof(t));         \
     return memory ? new(memory) t parms : NULL;

 template <class T>
-JS_ALWAYS_INLINE T *js_new() {
+JS_ALWAYS_INLINE T *js_new(JSContext *cx = NULL) {
     JS_NEW_BODY(T, ())
 }

Comment 11

[Nicholas Nethercote [inactive]]

I guess so, though I'll need to see a real patch to really understand.  In what cases would you pass NULL to js_new()?

Comment 12

[Paul Biggar]

(In reply to comment #11)
> I guess so, though I'll need to see a real patch to really understand. In what
> cases would you pass NULL to js_new()?

If there wasn't a convenient JSContext (for example, when we're deep in Yarr).

Comment 13

[Paul Biggar]

Attachment: WIP 1

This doesn't work, but is most of the way there.

Basically, I'm replacing:

js_new -> cx->create
js_delete -> cx->destroy
js_malloc -> cx->malloc
js_free -> cx->free

etc.

If there is no cx, I use a JSRuntime.

Things are a bit weird when we're creating the first Compartment, so WIP. Also, I have to hunt down many more js_xs, and write some |make check| script to prevent regression.

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to comment #13)
> 
> js_new -> cx->create
> js_delete -> cx->destroy
> js_malloc -> cx->malloc
> js_free -> cx->free

I came to the conclusion overnight that the other way is better:

  cx->malloc(n)   --> js_malloc(cx, n)
  cx->free(p)     --> js_free(p)
  js_new<T>(...)  --> js_new<T>(cx, ...)
  js_delete<T>(p) --> (unchanged)

I've always found |cx->malloc| confusing.  It looks like it allocates in a per-context pool or something, but it doesn't.  It just uses |cx| as a normal argument, just like a zillion other functions in SM, so why not pass it as a normal parameter?  We even get to use the standard SM idiom where "cx as first param means the function is fallible".  And unlike cx->malloc, it also works in the few places where we don't have access to a |cx| -- you can pass in NULL.

We could also have js_malloc/js_new alternatives that take a runtime instead of a cx, if that's helpful.

The non-cx, non-runtime version of js_malloc() (ie. the base allocator) would still be present, but should only called from the cx and runtime versions of js_malloc/js_new.  It would be nice to auto-check that this is true.

Comment 15

[Paul Biggar]

(In reply to comment #14)
> (In reply to comment #13)
> > 
> > js_new -> cx->create
> > js_delete -> cx->destroy
> > js_malloc -> cx->malloc
> > js_free -> cx->free
> 
> I came to the conclusion overnight that the other way is better:
> 
>   cx->malloc(n)   --> js_malloc(cx, n)
>   cx->free(p)     --> js_free(p)
>   js_new<T>(...)  --> js_new<T>(cx, ...)
>   js_delete<T>(p) --> (unchanged)

I came to a similar conclusion, largely because sometimes you need to delete from a destructor, and there isn't a context in sight.

Comment 16

[Paul Biggar]

(In reply to comment #14)
> (In reply to comment #13)
> > 
> > js_new -> cx->create
> > js_delete -> cx->destroy
> > js_malloc -> cx->malloc
> > js_free -> cx->free
> 
> I came to the conclusion overnight that the other way is better:
> 
>   cx->malloc(n)   --> js_malloc(cx, n)
>   cx->free(p)     --> js_free(p)
>   js_new<T>(...)  --> js_new<T>(cx, ...)
>   js_delete<T>(p) --> (unchanged)

Well this is unpleasant. cx->create and cx->destroy have access to pricate constructors because the constructed class is a friend of JSContext. I can't figure out how to do that for global functions, and njn's comment from jsutil.h indicates it's more trouble than it's worth:

> * If you have a class with a private constructor or destructor, you can
> * make js_new/js_delete a friend.  This can be fiddly, and the interaction of
> * template functions, friend functions and namespaces can overwhelm even
> * modern compilers.  Manual inlining is probably easier.

Am currently trying a special class wrapper to js_new to explicitly be a friend. You would call AllocatorFriend::js_new<T> and |friend ::AllocatorFriend|.

Comment 17

[Paul Biggar]

(In reply to comment #16)
> >   js_new<T>(...)  --> js_new<T>(cx, ...)
> >   js_delete<T>(p) --> (unchanged)

And another new plan. The one above is really unpleasant because some functions already take a JSRuntime or JSContext as their first parameter, and I can't figure out any way to make it hard to do that. Another alternative is make js_new a macro:

#define js_new(alloc, type, ...) __js_new<type>(alloc, __VA_ARGS__)

which works, but is unclear and makes error messages very unpleasant.

jorendorff suggestd (and also suggested the macro thing above) something like:

OffTheBooks::js_new<type>()
cx->js_new<type>
rt->js_new<type>

which is currently our best option I think.

Comment 18

[Paul Biggar]

Are we allowed change the interface of js_malloc. It's lowercase, so it should be private, but the jscustomallocator.h stuff worries me.

Comment 19

[Paul Biggar]

Attachment: finished patch

This gives 3 standard ways of allocating memory:
 - using a context
 - using a runtime
 - using OffTheBooks

each of which has the same functions:

 - ->js_new
 - ->malloc
 - ->calloc
 - ->realloc
 - ->array_new

Freeing memory is normal:
 - ::js_delete
 - ::js_free
 - ::js_array_delete


I've replaced all calls to js_new or js_malloc with calls through these interfaces. Whenever there was a cx or rt available, that was used. When there was nothing, OffTheBooks was used. This makes it nice and simple to know when memory is being allocated without a runtime being told: grep for OffTheBooks.

This also got around the private constructors problem: it's easy and portable to friend a class (OffTheBooks), while it was not for template functions (js_new). So I took out all the inlined js_new() calls (except ones that used calloc rather than malloc).

I reordered the allocation of atomsCompartment so that the GC is inited first, which should solve the original problem. I also deleted the extra compartments on OOM.

The only thing this patch doesn't do that I'd like to is to remove ::js_malloc, ::js_calloc and ::js_realloc, but I think I can't because of allowing custom allocators.

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 514596 [details] [diff] [review]
finished patch

Nice patch, it looks like a pretty clean solution to a tricky problem.  Two
thumbs up for getting those private constructors/destructors to work nicely.
And I like the name "OffTheBooks" :)


>  - ->js_new
>  - ->malloc
>  - ->calloc
>  - ->realloc
>  - ->array_new
> 
> Freeing memory is normal:
>  - ::js_delete
>  - ::js_free
>  - ::js_array_delete

It's a shame |js_new| can't be called |new|.  I'll suggest adding |js_|
prefixes to malloc/calloc/realloc/array_new for consistency, but I'll leave
the final decision up to you.


>-    if (!(atomsCompartment = js_new<JSCompartment>(this)) ||
>+    if (!js_InitGC(this, maxbytes))
>+        return false;
>+
>+    if (!(atomsCompartment = this->js_new<JSCompartment>(this)) ||
>         !atomsCompartment->init() ||
>         !compartments.append(atomsCompartment)) {
>         return false;
>     }

If init() or append() fails, won't the compartment have leaked?

>-    if (!js_InitGC(this, maxbytes) || !js_InitAtomState(this))
>+    atomsCompartment->setGCLastBytes(8192);
>+
>+    if (!js_InitAtomState(this))
>         return false;

Same here?  And maybe more like this further down in this function?


>diff --git a/js/src/jsarena.cpp b/js/src/jsarena.cpp
>--- a/js/src/jsarena.cpp
>+++ b/js/src/jsarena.cpp
>@@ -155,22 +155,22 @@ JS_ArenaAllocate(JSArenaPool *pool, size
>-                b = (JSArena *) js_malloc(gross);
>+                b = (JSArena *) js::OffTheBooks::malloc(gross);
>                 if (!b)

Arenas can allocate a lot of memory.  It would be nice to get them on the
books.  Can you try adding a JSContext* to JSArenaPool to get it on the
books?  It may end up being a pain, but seems worthwhile to try.

Likewise the js_calloc() in nanojit::Allocator::allocChunk -- could add a
JSContext* to VMAllocator.  Likewise for Tracker.  In general, every
OffTheBooks allocation is undesirable, right?  Maybe you can file a
follow-up bug to reduce them further?  (If so, you could add a "FIXME: bug
<NNNNNN>" comment to the comment explaining OffTheBooks.)


>+    CHECK_MALLOC(dataReserve, char*, DataReserveSize);
>+    CHECK_MALLOC(traceReserve, char*, TraceReserveSize);
>+    CHECK_MALLOC(tempReserve, char*, TempReserveSize);

Those allocations are reasonably big and easy to get on the books -- can you
change?


>+/*
>+ * Note lack of ; in JSRunttime below. This is intentional so "calling" this
>+ * looks "normal".
>+ */

Nit: s/JSRunttime/JSRuntime/

Comment 21

[Paul Biggar]

Attachment: Add checking script

This adds a checking script to prevent regressions, and fixes the few mistakes it found. The checking script is run upon |make check| and it checks that someone doesn't add new calls to js_malloc, js_calloc, js_realloc or OffTheBooks.

More calls to OffTheBooks are expected, in which case |make check| will fail, and Makefile.in will need to be updated.

Comment 22

[Paul Biggar]

Comment on attachment 514651 [details] [diff] [review]
Add checking script

Didn't see your response there.

Comment 23

[Luke Wagner [:luke]]

Nice work.  I also mourn the verbosity of cx->js_new but I also don't see a
better alternative.  My one nit is: could you drop the js:: in
js::OffTheBooks::js_new for uses in .cpp's?  Those should be in scope of a
'using namespace js'.

Comment 24

[Andreas Gal :gal]

Please also see bug 636300 and the background thread delete I need there.

Comment 25

[Paul Biggar]

(In reply to comment #20)

> And I like the name "OffTheBooks" :)

Me too, but I can't claim credit - jorendorff came up with it.


> It's a shame |js_new| can't be called |new|.  I'll suggest adding |js_|
> prefixes to malloc/calloc/realloc/array_new for consistency, but I'll leave
> the final decision up to you.

I agree, I'd like to use |new|. I'm not particularly tied to leaving off the js_, but the script I added to the recent patch does take advantage of it.

Any thoughts on cx->NEW?


> >-    if (!js_InitGC(this, maxbytes) || !js_InitAtomState(this))
> >+    atomsCompartment->setGCLastBytes(8192);
> >+
> >+    if (!js_InitAtomState(this))
> >         return false;
> 
> Same here?  And maybe more like this further down in this function?

I don't think so. After append succeeds, atomCompartment's cleanup should be handled by |compartments|. I assume this is cleaned up somewhere, though I can't find it at first glance.
 

> Arenas can allocate a lot of memory.  It would be nice to get them on the
> books.  Can you try adding a JSContext* to JSArenaPool to get it on the
> books?  It may end up being a pain, but seems worthwhile to try.

The arena allocation site is part of the JSAPI. Is it worth changing? If so, it's not too hard to do.

Comment 26

[Paul Biggar]

(In reply to comment #24)
> Please also see bug 636300 and the background thread delete I need there.

I think you're saying that I've converted some cx->destry and cx->free calls into js_delete and js_free which aren't freed in the background?

Comment 27

[Paul Biggar]

(In reply to comment #23)
> Nice work.  I also mourn the verbosity of cx->js_new but I also don't see a
> better alternative.  My one nit is: could you drop the js:: in
> js::OffTheBooks::js_new for uses in .cpp's?  Those should be in scope of a
> 'using namespace js'.

How do you feel about cx->New? It seems legal...

Comment 28

[Nicholas Nethercote [inactive]]

> The arena allocation site is part of the JSAPI. Is it worth changing? If so,
> it's not too hard to do.

I'm not sure what you mean... Arenas aren't part of the JSAPI, AFAICT.

> How do you feel about cx->New? It seems legal...

I was just about to suggest it! :)

Comment 29

[Paul Biggar]

(In reply to comment #28)
> > The arena allocation site is part of the JSAPI. Is it worth changing? If so,
> > it's not too hard to do.
> 
> I'm not sure what you mean... Arenas aren't part of the JSAPI, AFAICT.

extern JS_PUBLIC_API(void)
JS_InitArenaPool(JSArenaPool *pool, const char *name, size_t size,
                 size_t align, size_t *quotap);


I thought the JSAPI was marked by JS_PUBLIC_API macros, but I could be wrong about this.

Comment 30

[Nicholas Nethercote [inactive]]

(In reply to comment #29)
> 
> extern JS_PUBLIC_API(void)
> JS_InitArenaPool(JSArenaPool *pool, const char *name, size_t size,
>                  size_t align, size_t *quotap);

Oh.  But jsarena.h doesn't seem to be included by jsapi.h.  I suggest you leave it for now and come back to it in the follow-up bug.

Comment 31

[Paul Biggar]

(In reply to comment #26)
> (In reply to comment #24)
> > Please also see bug 636300 and the background thread delete I need there.

I'm currently replacing js_delete with cx->Delete and Foreground::Delete.

I see rt->gcHelperThread. Should I add rt->free() and rt->Delete??

Comment 32

[Nicholas Nethercote [inactive]]

(In reply to comment #31)
> (In reply to comment #26)
> > (In reply to comment #24)
> > > Please also see bug 636300 and the background thread delete I need there.
> 
> I'm currently replacing js_delete with cx->Delete and Foreground::Delete.

This makes me a little nervous -- what happens if mismatches occur?  Presumably the GC accounting gets screwed up.  Is there some way to check for mismatches in debug mode?

Comment 33

[Paul Biggar]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #26)
> > > (In reply to comment #24)
> > > > Please also see bug 636300 and the background thread delete I need there.
> > 
> > I'm currently replacing js_delete with cx->Delete and Foreground::Delete.
> 
> This makes me a little nervous -- what happens if mismatches occur?

The helper thread keeps a vector of memory it has to free, and then frees it using js_free, but it doesn't touch accounting at all. A mismatch will work fine: the memory will be freed in the background of a different thread. However, I can think of a pathological case:

 - context A does lots of allocation
 - context B does no allocation, but frees a lot
 - context B free's thread A's allocations
 - context A gets collected a lot, but has nothing to free
 - context B never gets collected, and so holds on to its entire free list.


>  Is there some way to check for mismatches
> in debug mode?

I'm not sure. Mismatches are superficially OK, so we might get false positives. But I can always try.

Comment 34

[Nicholas Nethercote [inactive]]

(In reply to comment #33)
> > 
> > This makes me a little nervous -- what happens if mismatches occur?
> 
> The helper thread keeps a vector of memory it has to free, and then frees it
> using js_free, but it doesn't touch accounting at all.

Oh, right.  That's not so bad then.  If it's easy to detect mismatches, that would be nice, otherwise just a comment somewhere about this would be good.

Comment 35

[Brendan Eich [:brendan]]

Capitalizing method names goes against style we have in common with JSC and nanojit. It also looks funny for common Unix libc names such as malloc and free (is the latter prefixed with js_ because of mozalloc.h?).

I can't say js_ prefixes all around beats capitalization -- my reaction is "a pox on both those houses". I'd much rather we use trialing _ only for C++ keywords and plain names for the rest. Reactions?

/be

Comment 36

[Paul Biggar]

(In reply to comment #35)
> Capitalizing method names goes against style we have in common with JSC and
> nanojit. It also looks funny for common Unix libc names such as malloc and free
> (is the latter prefixed with js_ because of mozalloc.h?).

It's not prefixed in method calls. It's prefixed in jsutil.h for lots of reasons, mozalloc is one of them, LD_PRELOAD is another, allowing embedding is another.


> I can't say js_ prefixes all around beats capitalization -- my reaction is "a
> pox on both those houses". I'd much rather we use trialing _ only for C++
> keywords and plain names for the rest. Reactions?

Functionally, they only difference is that the capital versions are easier to code-complete; not a big deal though. Aesthetically, I prefer capitalization, but I think it's a pox all round.

I do prefer any of these options to cx->create and cx->destroy which is what's currently there - I would never have guessed they were purely allocation functions.

Comment 37

[Luke Wagner [:luke]]

(In reply to comment #36)
> I do prefer any of these options to cx->create and cx->destroy which is what's
> currently there - I would never have guessed they were purely allocation
> functions.

At the time, SM hackers polled on #jsapi thought "create"/"destroy" were the most logical names given that 'new' wasn't an option and, as we have now seen thrice, the various manglings of 'new' strike people as unattractive.   Also, given that the first sentence of the C++ standard section 5.3.4 "New" begins:
  "The new-expression attempts to create an object of the type-id ..."
the names don't seem too far off the mark.

Comment 38

[Nicholas Nethercote [inactive]]

(In reply to comment #37)
>  Also,
> given that the first sentence of the C++ standard section 5.3.4 "New" begins:
>   "The new-expression attempts to create an object of the type-id ..."
> the names don't seem too far off the mark.

Not all of us have sections of the C++ standard memorized! :P

I prefer 'New', '_new' and 'js_new' to 'create'.

Comment 39

[Brendan Eich [:brendan]]

njn: how about new_?

All: the create/destroy pair is less good if higher-level than memory allocation and release, since create implies new + init and destroy is finish + delete/free. If we're adding thing veneer to C stdlib and C++ operators, we should stick as close to the canonical names as we can.

In my view, that means trailing _ beats capitalization. But it's a close call.

/be

Comment 40

[Nicholas Nethercote [inactive]]

'new_' is fine by me.

Comment 41

[Paul Biggar]

New is used in some places already, each of which do more than simple memory allocation: MonoIC, JSWrapper, CTypes. So I guess new_ is best, even if it is unsightly.

Comment 42

[Brendan Eich [:brendan]]

The trailing underscore can be squinted at hard, to make it look like a space (but the mandatory parens around the arglist remain).

Same for delete_ then?

/be

Comment 43

[Brendan Eich [:brendan]]

s/thing veneer/thin veneer/ in comment 39.

/be

Comment 44

[Paul Biggar]

Attachment: Near final patch

This is the final version I think.

However, I'm slightly worried by bug 636561 comment 3 and bug 636558 comment 5, since this does a small portion of what they're talking about, so I'm getting one more review?

Gal, do we want this for FF4?

Comment 45

[Andreas Gal :gal]

Nice patch. I think we can survive without this in FF4. We have 3 bugs targeting the same issue from different angles and one of them is already in. So lets land this right after FF4 branches.

Comment 46

[Paul Biggar]

Attachment: Final patch

This is the final patch I'm planning to push on Monday, unless there is objection. It differs from the old patch in that I divided Foreground into two classes: Foreground and UnwantedForeground.

With the last patch, there was a |make check| rule indicating that we wanted to get rid of Foreground deallocations. However, that wasn't really true (see bug 636561 comment 3 and bug 636561 comment 5). We only want to get rid of Foreground deallocations on the fast path, hence UnwantedForeground.

So now deallocations in the foreground which we don't want use UnwantedForeground, and deallocations in the foreground which we do want use Foreground.

Comment 47

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 517216 [details] [diff] [review]
Final patch

>diff --git a/js/src/bench.sh b/js/src/bench.sh
>old mode 100755
>new mode 100644

?

>diff --git a/js/src/config/find_vanilla_new_calls b/js/src/config/find_vanilla_new_calls
>--- a/js/src/config/find_vanilla_new_calls
>+++ b/js/src/config/find_vanilla_new_calls
>@@ -14,17 +14,17 @@
> #   alloc/free checking.
> # - Doesn't detect the 'nothrow' variants, which are ok but probably still
> #   best avoided.
> # - Is designed to only run on Linux (though it may also work on Mac);  one
> #   platform will be enough to catch any violations.
> #
> # If this script fails:
> # - You need to find the uses of vanilla new/delete and replace them with
>-#   js_new()/js_delete().
>+#   {js::Foreground,JSContext,JSRuntime}::{New,/array_new}.

'New' or 'new_'?

>diff --git a/js/src/jsutil.h b/js/src/jsutil.h
>--- a/js/src/jsutil.h
>+++ b/js/src/jsutil.h
>+ *   - Classes which have private constructors or destructors should have
>+ *     JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR added to their
>+ *     declaration (with no ';').

>+ *   JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR makes the allocation
>+ *   classes friends with your class, giving them access to private
>+ *   constructors and destructors. Adding a superfluous ';' after the
>+ *   declaration will cause the Mozilla tinderbox to go red, so avoid it.

>+/*
>+ * Note lack of ; in JSRuntime below. This is intentional so "calling" this
>+ * looks "normal".
>+ */
>+#define JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR \
>+    friend class js::OffTheBooks;\
>+    friend class js::Foreground;\
>+    friend class js::UnwantedForeground;\
>+    friend struct ::JSContext;\
>+    friend struct ::JSRuntime

>diff --git a/js/src/yarr/yarr/RegexPattern.h b/js/src/yarr/yarr/RegexPattern.h
>--- a/js/src/yarr/yarr/RegexPattern.h
>+++ b/js/src/yarr/yarr/RegexPattern.h
> struct CharacterClassTable {
>+
>+    JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR;
>+

So, what's up here?

Comment 48

[Paul Biggar]

Attachment: near final patch

(In reply to comment #47)
> >diff --git a/js/src/bench.sh b/js/src/bench.sh
> >old mode 100755
> >new mode 100644
> ?

Indeed. Dunno how that snuck in.



> >-#   js_new()/js_delete().
> >+#   {js::Foreground,JSContext,JSRuntime}::{New,/array_new}.
> 
> 'New' or 'new_'?

new_. Fixed.

 
> >+ *     JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR added to their
> >+ *     declaration (with no ';').
> >+ * Note lack of ; in JSRuntime below. This is intentional so "calling" this
> >+ * looks "normal".
> >+ */
> >+#define JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR \
> >+    friend struct ::JSRuntime
> > struct CharacterClassTable {
> >+
> >+    JS_DECLARE_ALLOCATION_FRIENDS_FOR_PRIVATE_CONSTRUCTOR;
> >+
> 
> So, what's up here?

Error in the comment. The ';' isn't consumer facing, so I've removed it.


Great catches, thanks.

Comment 49

[Paul Biggar]

cjones, I can't figure this out. This patch causes the build to break at various points related to moz_free. For example:

g++ -o HttpBaseChannel.o -c  -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Darwin10.6.0\" -DOSARCH=Darwin -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD  -DOS_MACOSX=1 -DOS_POSIX=1  -DIMPL_NS_NET -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/../../base/src -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/xpcom/ds -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/content/base/src -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/content/events/src  -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/ipc/chromium/src -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/ipc/glue -I../../../ipc/ipdl/_ipdlheaders  -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/obj-x86_64-apple-darwin10.6.0/dist/include/nspr -I/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/obj-x86_64-apple-darwin10.6.0/dist/include/nss       -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -gstabs -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fomit-frame-pointer   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/HttpBaseChannel.pp /Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/HttpBaseChannel.cpp
In file included from ../../../ipc/ipdl/_ipdlheaders/mozilla/net/PHttpChannel.h:21,
                 from ../../../ipc/ipdl/_ipdlheaders/mozilla/net/PHttpChannelChild.h:10,
                 from ../../../dist/include/mozilla/net/HttpChannelChild.h:48,
                 from /Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp:108:
../../../dist/include/mozilla/net/NeckoMessageUtils.h: In static member function ‘static void IPC::ParamTraits<IPC::InputStream>::Write(IPC::Message*, const IPC::InputStream&)’:
../../../dist/include/mozilla/net/NeckoMessageUtils.h:218: warning: unused variable ‘rv’
../../../dist/include/mozilla/net/NeckoMessageUtils.h:229: warning: unused variable ‘rv’
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp: In function ‘nsresult PrepareAcceptLanguages(const char*, nsACString_internal&)’:
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp:1191: error: ‘moz_free’ is not a member of ‘nsCRT’
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp:1222: error: ‘moz_free’ is not a member of ‘nsCRT’
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp: In function ‘nsresult PrepareAcceptCharsets(const char*, nsACString_internal&)’:
/Users/pbiggar/work/mozilla/rename_jsmalloc_etc/netwerk/protocol/http/nsHttpHandler.cpp:1349: error: ‘moz_free’ is not a member of ‘nsCRT’

I had other problems with moz_strdup when I sprinkled the headers more.

I note that xpcom/base/nscore.h opens macro_wrappers but doesn't close them, is that right? Likewise in ipc/chromium/src/base/message_pump_qt.h, but I doubt that's our problem here.

What are the rules for using macro wrappers? Can you enumerate them, and I'll add them as a comment somewhere appropriate?

Comment 50

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

The goal for the wrappers is to #define malloc/free/et al. for gecko; that's why nscore.h includes the header.  There aren't any hard-and-fast rules for including them, just whatever works (it's a big gross hack).  It would help understand what's failing if you made a preprocessed version of nsHttpHandler, |make -C $objdir/netwerk/protocol/http nsHttpHandler.i|.  If the macros are conflicting with JS headers, moving the JS includes before Gecko stuff can work around that.

Comment 51

[Paul Biggar]

http://hg.mozilla.org/tracemonkey/rev/f949c8533aa0

Comment 52

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/f949c8533aa0