1412234 - Make all allocator API entry points handle initialization properly

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1412234 - Make malloc_init return whether the allocator was successfully initialized.

Review commit: https://reviewboard.mozilla.org/r/193854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193854/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1412234 - Make all allocator API entry points handle initialization properly.

Some need initialization to happen, some can be skipped when the
allocator was not initialized, and others should crash.

Review commit: https://reviewboard.mozilla.org/r/193856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193856/

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8922702 [details]
Bug 1412234 - Make all allocator API entry points handle initialization properly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193856/diff/1-2/

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8922701 [details]
Bug 1412234 - Make malloc_init return whether the allocator was successfully initialized.

https://reviewboard.mozilla.org/r/193854/#review199304

::: memory/build/mozjemalloc.cpp:4106
(Diff revision 1)
>  #if defined(XP_WIN)
> -#define malloc_init() false
> +#define malloc_init() true
>  #else
> +// Returns whether the allocator was successfully initialized.
>  static inline bool
>  malloc_init(void)

The "void" could be removed.

::: memory/build/mozjemalloc.cpp:4139
(Diff revision 1)
> +// Returns whether the allocator was successfully initialized.
>  #if !defined(XP_WIN)
>  static
>  #endif
>    bool
>    malloc_init_hard(void)

ditto

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8922702 [details]
Bug 1412234 - Make all allocator API entry points handle initialization properly.

https://reviewboard.mozilla.org/r/193856/#review199306

This all seems reasonable. I'm wondering what prompted you to write the patch, and how you found all the places to change. Via some testing, or just code inspection?

::: memory/build/mozjemalloc.cpp:3378
(Diff revision 2)
>    arena_chunk_t* chunk = GetChunkForPtr(aPtr);
>  
>    // Is the pointer null, or within one chunk's size of null?
> -  if (!chunk) {
> +  // Alternatively, if the allocator is not initialized yet, the pointer
> +  // can't be known.
> +  if (!chunk || !malloc_initialized) {

This is pre-existing but: what is the synchronization story for `malloc_initialized`? It looks like it's totally racy, being accessed on arbitrary threads without any protection...

::: memory/build/mozjemalloc.cpp:4482
(Diff revision 2)
>    offset = GetChunkOffsetForPtr(aPtr);
>    if (offset != 0) {
> +    MOZ_RELEASE_ASSERT(malloc_initialized);
>      arena_dalloc(aPtr, offset);
>    } else if (aPtr) {
> +    MOZ_RELEASE_ASSERT(malloc_initialized);

Move the duplicated assertions before the `if`.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Comment on attachment 8922702 [details]
> Bug 1412234 - Make all allocator API entry points handle initialization
> properly.
> 
> https://reviewboard.mozilla.org/r/193856/#review199306
> 
> This all seems reasonable. I'm wondering what prompted you to write the
> patch,

Julian reporting to me that calling moz_alloc_* functions directly from a test case caused crashes.

> and how you found all the places to change. Via some testing, or just
> code inspection?

I just went through the public API.

> ::: memory/build/mozjemalloc.cpp:3378
> (Diff revision 2)
> >    arena_chunk_t* chunk = GetChunkForPtr(aPtr);
> >  
> >    // Is the pointer null, or within one chunk's size of null?
> > -  if (!chunk) {
> > +  // Alternatively, if the allocator is not initialized yet, the pointer
> > +  // can't be known.
> > +  if (!chunk || !malloc_initialized) {
> 
> This is pre-existing but: what is the synchronization story for
> `malloc_initialized`? It looks like it's totally racy, being accessed on
> arbitrary threads without any protection...

There is a race but in practice, we're always being initialized when there's only one thread, and the value only transitions once from false to true.

Anyways, I'll file a bug to turn it into an Atomic<>.

> ::: memory/build/mozjemalloc.cpp:4482
> (Diff revision 2)
> >    offset = GetChunkOffsetForPtr(aPtr);
> >    if (offset != 0) {
> > +    MOZ_RELEASE_ASSERT(malloc_initialized);
> >      arena_dalloc(aPtr, offset);
> >    } else if (aPtr) {
> > +    MOZ_RELEASE_ASSERT(malloc_initialized);
> 
> Move the duplicated assertions before the `if`.

It's duplicated because the implicit else case that does nothing (free(nullptr)) doesn't need the check.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8922701 [details]
Bug 1412234 - Make malloc_init return whether the allocator was successfully initialized.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193854/diff/1-2/

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8922702 [details]
Bug 1412234 - Make all allocator API entry points handle initialization properly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193856/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ac86eb005d4
Make malloc_init return whether the allocator was successfully initialized. r=njn
https://hg.mozilla.org/integration/autoland/rev/3d655d6abecf
Make all allocator API entry points handle initialization properly. r=njn

Comment 10

[Nicholas Nethercote [inactive]]

> There is a race but in practice, we're always being initialized when there's
> only one thread, and the value only transitions once from false to true.

Fair enough.
 
> Anyways, I'll file a bug to turn it into an Atomic<>.

I'm not fussed by this. We have other code that does much the same thing without using Atomic<>.

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5ac86eb005d4
https://hg.mozilla.org/mozilla-central/rev/3d655d6abecf