818922 - Add a way to do bidirectional function calls between libxul and a replace-malloc lib

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Mike Hommey [:glandium]]

There can be needs to call libxul functions from a replace-malloc library, and vice-versa.

A reasonable generic way to address this need is to add a function that takes a magic number and a function table and returns a function table. The magic number would be used for both ends to agree on compatibility, and the function tables would contain functions pointers from both ends.

Comment 1

[Mike Hommey [:glandium]]

For convenience, the function tables could be replaced by pure virtual class instances in C++. Really, any type libxul and the replace malloc lib can agree on, so API-wise, we'd give and return void *.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Add bidirectional method calls with replace-malloc library

See patch from bug 1097507 for how replace_malloc_end_point.h ends up being filled.

I'm not very attached to the "end point" terminology. I just had no better idea. Please feel free to bikeshed.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8521192 [details] [diff] [review]
Add bidirectional method calls with replace-malloc library

Review of attachment 8521192 [details] [diff] [review]:
-----------------------------------------------------------------

(Still working through this. Here are some comments.)

As discussed on IRC, "bridge" might be a better name than "end point".

Is the ReplaceMallocEndPointImpl/ReplaceMallocEndPoint split necessary? (Likewise with DMDImpl/DMD.)

::: memory/build/replace_malloc.c
@@ +15,5 @@
>  #endif
>  
>  #include "mozmemory_wrap.h"
>  
> +#define ReplaceMallocEndPointImpl ReplaceMallocEndPoint

XXX: Why is this necessary?

::: memory/build/replace_malloc_end_point.h
@@ +20,5 @@
> + * can decide to implement those methods or not.
> + *
> + * Replace-malloc libraries can provide such an end point by implementing
> + * a ReplaceMallocEndPointImpl-derived class, and a replace_get_end_point
> + * function returning an instance of that class. The defaults methods in

s/defaults/default/

@@ +26,5 @@
> + * would be understand as "end point doesn't implement this method", so that
> + * a replace-malloc library doesn't have to implement all methods.
> + *
> + * All methods of ReplaceMallocEndPointImpl must be virtual. Similarly,
> + * anything passed as argument to those methods must be plain data, or

"as an argument"

@@ +27,5 @@
> + * a replace-malloc library doesn't have to implement all methods.
> + *
> + * All methods of ReplaceMallocEndPointImpl must be virtual. Similarly,
> + * anything passed as argument to those methods must be plain data, or
> + * instances of classes with only virtual methods.

"an instance of a class"

@@ +31,5 @@
> + * instances of classes with only virtual methods.
> + *
> + * Binary compatibility is expected to be maintained, such that a newer
> + * Firefox can be used with an old replace-malloc library, or an old
> + * Firefox can be used with a newere replace-malloc library. As such, only

s/newere/newer/

@@ +57,5 @@
> + * Helper around ReplaceMallocEndPointImpl for use by the program.
> + * Each virtual method in ReplaceMallocEndPointImpl should have a
> + * corresponding static method doing version checking.
> + */
> +struct ReplaceMallocEndPoint: public ReplaceMallocEndPointImpl

Nit: space before ':'.

Comment 4

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Comment on attachment 8521192 [details] [diff] [review]
> Add bidirectional method calls with replace-malloc library
> 
> Review of attachment 8521192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (Still working through this. Here are some comments.)
> 
> As discussed on IRC, "bridge" might be a better name than "end point".
> 
> Is the ReplaceMallocEndPointImpl/ReplaceMallocEndPoint split necessary?
> (Likewise with DMDImpl/DMD.)

I wanted to clearly separate methods for use from the program side (gecko).

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8521192 [details] [diff] [review]
Add bidirectional method calls with replace-malloc library

Review of attachment 8521192 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see new patches here and in bug 1097507 using "bridge" naming before I give r+.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Add bidirectional method calls with replace-malloc library

This addresses your first comments, and changes some more, hopefully helping clarify things.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8522555 [details] [diff] [review]
Add bidirectional method calls with replace-malloc library

Review of attachment 8522555 [details] [diff] [review]:
-----------------------------------------------------------------

::: memory/build/malloc_decls.h
@@ +25,5 @@
>  #  define MALLOC_FUNCS_JEMALLOC 2
>  #  define MALLOC_FUNCS_INIT 4
> +#  define MALLOC_FUNCS_END_POINT 8
> +#  define MALLOC_FUNCS_ALL (MALLOC_FUNCS_INIT | MALLOC_FUNCS_END_POINT | \
> +                            MALLOC_FUNCS_MALLOC | MALLOC_FUNCS_JEMALLOC)

Change END_POINT to BRIDGE, please :)

::: memory/build/replace_malloc_bridge.h
@@ +64,5 @@
> +
> +#ifndef REPLACE_MALLOC_IMPL
> +  /* Returns the replace-malloc bridge if its version is at least the
> +   * requested one. */
> +  static ReplaceMallocBridge* get(int aWantedVersion) {

Nit: to match Mozilla style, this should be Get().

Would aMinimumVersion be a better name than aWantedVersion?

Comment 8

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd1cbced0ccb

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd1cbced0ccb