primiano: We will also need to add some data to AllocatorDispatch so that the default dispatch will fire correctly. I would shove an entire MallocZoneFunctions into the AllocatorDispatch, since we're not concerned with the size of AllocatorDispatch, and this minimizes pointer dereferences.

> We're trying to not use interposition as much as possible, so I propose the following pull model
+1 every single time I did any symbol inerposition the pattern was the same: it looked a great in the beginning and than I ended up crying under my desk for a week.

Re polling: 
Let's think, what is the thing we really want to achieve here? Is it about memory reporting right? can we poll only when we actually create memory dumps? I mean in that case we are already in the state of mind of creating some overhead.
The only problem here is the model where we create exactly one memory dump (e.g. TraceOnTap) and discovering a new zone at the time of the dump is just too late. We will have lost all the history of allocation for the heap profiler.

Another question I have is: do we want to support the heap profiler on the extra zones or we are fine only with reporting them in standard (lightweight) dumps. If the latter, the point above is a non-problem. But since you are talking about shimming I guess you mean the former here.


> Actually hooking the zones with the allocator shim:
> I proposed a simple model, which is to modify all Shim and AllocatorDispatch methods to take an extra void* argument, which we can cast to malloc_zone_t* on macOS.

So my fear is that:
- adding a zone argument to *all* platforms might cause a regression at least on one of them, and cause back and forth revert cycles with per sheriffs. although, might worth try this a bit, after all is just an extra "push" operation or register assignment.
- adding a zone under #ifdef might make the code a bit too clunky (e.g, now you have to add #ifdefs to siggi's stuff also)

If I have to chose between the two I'd go for the former (i.e. your proposal) and cross fingers that won't cause measurable slowdowns. Maybe there is some annotation to tell the compiler "this argument is actually something you can ignore".

> Note: This intentionally does not distinguish between malloc zones. We can add that if we deem it necessary, but the information from native/pseudo heap profiling should already provide a lot more information.
I got a bit lost here. One line above you were proposing to pass the zone as argument. Now you are saying "his intentionally does not distinguish between malloc zones" ??


> Note: This also relies on the assumption that when a consumer calls malloc(zone31, 100), that we actually need to forward the malloc to the appropriate zone. This seems like a fairly important assumption not to break, since we don't know how/what these consumers are doing with their custom malloc zones.
Yes we definitely need to preserve that. I don't even want to think what will happen if you criss-cross the zones.

> I got a bit lost here. One line above you were proposing to pass the zone as argument. Now you are saying "his intentionally does not distinguish between malloc zones" ??

Passing the zone as an argument is required to dispatch the call to the correct malloc zone, once we're done with it.

We could add additional instrumentation to our various profilers to distinguish between allocations in different malloc zones - I'm suggesting we don't do that unless we find a good reason to do so.

Re #3: ahh, yes fully agree. We can leverage the type-info field if we really need to surface that detail but wouldn't go there until we know that we need it.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eff0ecbf12a6757ebb46438100fef60dff531e43

commit eff0ecbf12a6757ebb46438100fef60dff531e43
Author: erikchen <erikchen@chromium.org>
Date: Mon Feb 20 13:04:50 2017

base: Add support for malloc zones to the allocator shim

This CL has no functional change, but could cause performance
regressions. It adds an additional void* context parameter to all
allocator-shim related functions, as it will be necessary to support
multiple malloc zones.

BUG=693237
TBR=thakis@chromium.org

Review-Url: https://codereview.chromium.org/2697123007
Cr-Commit-Position: refs/heads/master@{#451612}

[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim.cc
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim.h
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim_default_dispatch_to_glibc.cc
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim_default_dispatch_to_linker_wrapped_symbols.cc
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim_default_dispatch_to_mac_zoned_malloc.cc
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim_default_dispatch_to_tcmalloc.cc
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim_default_dispatch_to_winheap.cc
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim_override_glibc_weak_symbols.h
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim_override_mac_symbols.h
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim_override_ucrt_symbols_win.h
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/allocator/allocator_shim_unittest.cc
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/debug/thread_heap_usage_tracker.cc
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/debug/thread_heap_usage_tracker_unittest.cc
[modify] https://crrev.com/eff0ecbf12a6757ebb46438100fef60dff531e43/base/trace_event/malloc_dump_provider.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/475483d5281e57336eab47e4527424e1140cabf4

commit 475483d5281e57336eab47e4527424e1140cabf4
Author: erikchen <erikchen@chromium.org>
Date: Wed Feb 22 12:39:12 2017

Add a comment to the allocator shim.

BUG=693237

Review-Url: https://codereview.chromium.org/2712503002
Cr-Commit-Position: refs/heads/master@{#452012}

[modify] https://crrev.com/475483d5281e57336eab47e4527424e1140cabf4/base/allocator/allocator_shim.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7a54483e22a7e4578870d144b7d4a71380e63a10

commit 7a54483e22a7e4578870d144b7d4a71380e63a10
Author: erikchen <erikchen@chromium.org>
Date: Fri Feb 24 10:04:43 2017

macOS: Shim all malloc zones.

All information about malloc zones [and their original functions] is stored in
MallocZoneAggregator. When the allocator shim is ready to dispatch back to the
original system implementations, it calls the MallocZoneAggregator with
|context|, which provides enough information to dispatch to the correct malloc
zone.

BUG=693237

Review-Url: https://codereview.chromium.org/2703803004
Cr-Commit-Position: refs/heads/master@{#452784}

[modify] https://crrev.com/7a54483e22a7e4578870d144b7d4a71380e63a10/base/BUILD.gn
[modify] https://crrev.com/7a54483e22a7e4578870d144b7d4a71380e63a10/base/allocator/allocator_interception_mac.h
[modify] https://crrev.com/7a54483e22a7e4578870d144b7d4a71380e63a10/base/allocator/allocator_interception_mac.mm
[modify] https://crrev.com/7a54483e22a7e4578870d144b7d4a71380e63a10/base/allocator/allocator_shim_default_dispatch_to_mac_zoned_malloc.cc
[modify] https://crrev.com/7a54483e22a7e4578870d144b7d4a71380e63a10/base/allocator/allocator_shim_default_dispatch_to_mac_zoned_malloc.h
[modify] https://crrev.com/7a54483e22a7e4578870d144b7d4a71380e63a10/base/allocator/allocator_shim_override_mac_symbols.h
[add] https://crrev.com/7a54483e22a7e4578870d144b7d4a71380e63a10/base/allocator/malloc_zone_functions_mac.cc
[add] https://crrev.com/7a54483e22a7e4578870d144b7d4a71380e63a10/base/allocator/malloc_zone_functions_mac.h
[add] https://crrev.com/7a54483e22a7e4578870d144b7d4a71380e63a10/base/allocator/malloc_zone_functions_mac_unittest.cc

There are currently two problems:
1) malloc_get_all_zones is not thread-safe, and causes undefined behavior is a zone is added or removed during iteration. the memory_reader_t argument is pointless since the function call itself doesn't call MALLOC_LOCK. 

Three potential approaches:
1a) Interposing malloc_register_zone allows us to avoid calling malloc_get_all_zones. It also avoids the need to poll to find new zones. 
1b) We could theoretically use _malloc_fork_prepare and _malloc_fork_parent to lock/unlock. This seems like a bad idea.
1c) Only call malloc_get_all_zones very early in the life cycle of the process, when we think this race is unlikely to occur.

2) There's is no way to hook into a malloc zone as soon as it's added. 
2a) Poll intermittently to collect new malloc zones.
2b) Try to "force" malloc zones to get allocated by calling the relevant initializers before shimming all malloc zones.

It looks like we currently have one use case for interposition [cf_bundle_blocker]. I'd like to add another, given that it cleanly solves both (1) and (2).

We've been trying to avoid new interposing (and we did try and remove cf_bundle_blocker now that we can do the same thing with code signing, but that caused an inexplicable perf regression). But it does seem like the best option.

Also, it seems like we should file a bug for malloc_get_all_zones not locking.

Filed radar 30771172.

I recently noticed that malloc zones can reference each other. This means that the allocator-shim can be re-entrant. I've observed the following behavior when shimming all malloc zones:

default malloc zone = Zone A [with shim A]
other malloc zone = Zone B [with shim B]

batch_malloc -> shim A
                ThreadHeapUsageTracker records statistics
                MallocDumpProvider records statistics
             -> original zone A
             -> shim B
                ThreadHeapUsageTracker records statistics
                MallocDumpProvider records statistics
             -> original zone B.
This results in double statistics recorded.

There are three possible solutions:

Solution 1:
When allocator shim detects re-entrance, skip to last AllocatorDispatch. This is cleanest, but has high performance overhead [requires TLS, even when allocator shim turned off].

Solution 2:
Do nothing. ThreadHeapUsageTracker will break on macOS for this use case. MallocDumpProvider already does the right thing [if address already present, overwrite with most recent stack trace, which will record a stack trace for the outer loop.

Solution 3:
Both ThreadHeapUsageTracker and MallocDumpProvider already require some type of synchronization primitive to deal with multi-threading. Expose some type of thread-affine sentinel in their implementations of MallocShim that detect re-entrance and skip recording stats if so.

Honestly, I think that [Solution 2] seems best. I would probably just turn off ThreadHeapUsageTracker on macOS, since it's stats would no longer be valid.
  

Sorry for being late on this. Bunch of thoughts.

Re: interposition: I don't know anything about mac, but from experience on Linux/Android I can tell you that every single time we touched anything about interposition there we ended up with all sort of surprises. I would probably not go there.

> 1b) We could theoretically use _malloc_fork_prepare and _malloc_fork_parent to lock/unlock. This seems like a bad idea.
I was looking at the darwin sources and was about to suggest this. But then while googling, the 3rd result I found was this bug XD. Why do you think is bad (other than, yeah, hacky)? In theory fork_prepare shouldn't do anything that changes the state of the parent process as fork can happen all the times.
It really sucks that malloc_get_all_zones is not locked, but honestly, so far this seems that so far this seems the best option on the table.

The alternative is poll and take the risk of crashing. Which honestly is okay for the case of user-initiated heap profiling (i.e. passing --enable-heap-profiling) but could become a bummer for the heap-profiling from the field.

Re: #12, multi-zone re-entrancy.
>Solution 1: requires TLS, even when allocator shim turned off
I think you meant "when the heap profiler is turned off". but yeah agree that this might be a perf bummer so I wouldn't go there. Let's just add a comment to explain this to the shim files in one of your next CLs.

> Solution 2: Do nothing.
Yeah. Essentially here you are saying: the hooks must be reentrant safe, deal with that, good luck.
FYI as per very recently the ThreadHeapUseageTracker is behind a commandline flag (https://codereview.chromium.org/2695013005/) so won't be turned on if you explicitly ask for that. Still we should re-audit the existing malloc_dump_provider to check is reentrant safe for real, and not only by luck :) 

> Solution 3
Yeah this might be the answer to "how to make them reentrant safe". Both of them (malloc_dump_provider and THUT) already a sentinel mechanism to deal with the case of reentrancy while initializing the TLS object the first time, so it's more a matter of extending it. 

Out of curiosity, can the nesting be between two arbitrary zones, or it's always between one non-default zone and the default zone?
In the latter case might be able to solve the problem splitting the functions for the default and non-default zone.

Apple responded on the radar indicating that malloc_get_all_zones is not thread-safe.

"""
this interface is meant to be used with the target process suspended, hence doesn’t need a lock.
"""

Not exactly a great solution for us.

is this bug mac only?

mark, response requested:

There are two problems we need to solve. 
1) Iterating through malloc_zones isn't thread-safe unless we grab a lock internal to libmalloc. We are going to need to do this for memory-infra UMA metrics, since malloc_zone_statistics internally iterates through all malloc_zones without taking a lock. Three possible solutions:
  1a) Do nothing. Hope that nothing bad happens.
  1b) Call _malloc_fork_prepare() and _malloc_fork_parent to lock/unlock. This relies on internal implementation details of libmalloc.
  1c) Interpose register_zones and unregister_zones. 

primiano prefers (1b). This solution will need to be run on all users. I'm okay with (1b).

2) There are no notifications when new malloc zones are created. This means that if we want to intercept all malloc zones, we need to continuously poll to make sure we don't miss any, and we'll still miss allocations that happen before we finish shimming the relevant malloc zones. Two possible solutions:
  2a) Poll.
  2b) Interpose register_zones and unregister_zones.

I prefer (2b), but would be willing to consider (2a). Note that this only affects users that have heap profiling enabled. 

Bonus problem:
3) I suspect that in the not-too-distant future, I will want to hook mach_vm_allocate. 

Note that for (2) and (3), we could also do shenanigans involving library load order [DYLD_INSERT_LIBRARIES]. Or, as an even more drastic change, we could ship an external tool that requires root privileges [dtrace, etc.]. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f9bf057e1227bcee1d24e4b619894233946d26c

commit 2f9bf057e1227bcee1d24e4b619894233946d26c
Author: erikchen <erikchen@chromium.org>
Date: Tue Mar 28 01:11:26 2017

mac: Several minor fixes to allocator shim.

This CL:
  * Ensures that no allocator shim state is carried over between tests.
  * Updates malloc zone interception logic to reflect the fact that the default
  malloc zone is not included in malloc_get_all_zones.
  * Turn MallocZoneFunctions into POD. Update g_malloc_zones to be an array
  rather than a pointer.

BUG=693237

Review-Url: https://codereview.chromium.org/2727463002
Cr-Commit-Position: refs/heads/master@{#459964}

[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/allocator/allocator_interception_mac.h
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/allocator/allocator_interception_mac.mm
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/allocator/allocator_shim.cc
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/allocator/allocator_shim_override_mac_symbols.h
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/allocator/allocator_shim_unittest.cc
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/allocator/malloc_zone_functions_mac.cc
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/allocator/malloc_zone_functions_mac.h
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/allocator/malloc_zone_functions_mac_unittest.cc
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/debug/thread_heap_usage_tracker_unittest.cc
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/base/process/memory_unittest.cc
[modify] https://crrev.com/2f9bf057e1227bcee1d24e4b619894233946d26c/chrome/installer/setup/memory_unittest.cc

1. 1b.

2. For 2b, you mean malloc_zone_register() and malloc_zone_unregister(), right? Thinking aloud: instead of a straight poll, set the protection on the page containing malloc_num_zones to just PROT_READ, and have a user-space fault handler that gets tickled act as a hint that you’ve got to unprotect (from the handler), then wait-reprotect-check?

3. Run a mach_msg_server for subsystems that would receive task port messages. mach_task_self_ = new_port. Intercept the calls that you’re interested in, and forward them along to the real task port.

I read all of this back and think that I may be slowly losing my mind. Think about it, let it stew, and I’ll do the same. Then we can discuss.

> Note that for (2) and (3), we could also do shenanigans involving library load
> order [DYLD_INSERT_LIBRARIES].

Negative. By virtue of setting the “restrict” bit in our code signature, dyld will pruneEnvironmentVariables() which unsets all of DYLD_*. Yes, we do want this “pruning.”

Thanks mark. I stewed. 

> 1. 1b.
Acknowledged.

> 2. For 2b, you mean malloc_zone_register() and malloc_zone_unregister(), right? Thinking aloud: instead of a straight poll, set the protection on the page containing malloc_num_zones to just PROT_READ, and have a user-space fault handler that gets tickled act as a hint that you’ve got to unprotect (from the handler), then wait-reprotect-check?
I've decided that the marginal benefit of hooking all malloc zones properly is small. Basically, it only helps heap profiling, and only if one of these unusual malloc zones is doing something interesting. If this is happening, we should be able to figure it out by looking at the difference in size between all malloc zones allocated objects, and the malloc zones we've hooked. The best way forward is to do a brief, periodic shim at startup:
https://codereview.chromium.org/2713553002/

Which everyone already lgtm-ed as a reasonable thing to do. This captures the most imoprtant malloc zones [such as nanomalloc]. 

3. Run a mach_msg_server for subsystems that would receive task port messages. mach_task_self_ = new_port. Intercept the calls that you’re interested in, and forward them along to the real task port.
I no longer wish to prematurely tackle this issue. We can work on this if we deem that it's important to hook vm_allocate.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/245f17559f973bb5a207c063d982c0bab0ec1c0b

commit 245f17559f973bb5a207c063d982c0bab0ec1c0b
Author: erikchen <erikchen@chromium.org>
Date: Tue Apr 04 21:07:18 2017

mac: Periodically shim new malloc zones immediately after startup.

On macOS, libraries lazily make malloc zones. There are no notifications when
this happens. This CL adds polling with an exponential backoff to check for the
existence of new malloc zones, stopping after 1 minute.

This CL only has an affect when heap profiling is enabled.

BUG=693237

Review-Url: https://codereview.chromium.org/2713553002
Cr-Commit-Position: refs/heads/master@{#461836}

[modify] https://crrev.com/245f17559f973bb5a207c063d982c0bab0ec1c0b/base/BUILD.gn
[modify] https://crrev.com/245f17559f973bb5a207c063d982c0bab0ec1c0b/base/allocator/allocator_interception_mac.h
[modify] https://crrev.com/245f17559f973bb5a207c063d982c0bab0ec1c0b/base/allocator/allocator_interception_mac.mm
[add] https://crrev.com/245f17559f973bb5a207c063d982c0bab0ec1c0b/base/allocator/allocator_interception_mac_unittest.mm
[modify] https://crrev.com/245f17559f973bb5a207c063d982c0bab0ec1c0b/content/browser/browser_main_loop.cc
[modify] https://crrev.com/245f17559f973bb5a207c063d982c0bab0ec1c0b/content/child/child_thread_impl.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ecc1d96e6789d172b45dd155ccdb26b631f1ecb9

commit ecc1d96e6789d172b45dd155ccdb26b631f1ecb9
Author: findit-for-me <findit-for-me@appspot.gserviceaccount.com>
Date: Tue Apr 04 22:52:36 2017

Revert of mac: Periodically shim new malloc zones immediately after startup. (patchset #9 id:160001 of https://codereview.chromium.org/2713553002/ )

Reason for revert:

Findit identified CL at revision 461836 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzI0NWYxNzU1OWY5NzNiYjVhMjA3YzA2M2Q5ODJjMGJhYjBlYzFjMGIM

Original issue's description:
> mac: Periodically shim new malloc zones immediately after startup.
>
> On macOS, libraries lazily make malloc zones. There are no notifications when
> this happens. This CL adds polling with an exponential backoff to check for the
> existence of new malloc zones, stopping after 1 minute.
>
> This CL only has an affect when heap profiling is enabled.
>
> BUG=693237
>
> Review-Url: https://codereview.chromium.org/2713553002
> Cr-Commit-Position: refs/heads/master@{#461836}
> Committed: https://chromium.googlesource.com/chromium/src/+/245f17559f973bb5a207c063d982c0bab0ec1c0b

TBR=primiano@chromium.org,mark@chromium.org,avi@chromium.org,erikchen@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=693237

Review-Url: https://codereview.chromium.org/2802493002
Cr-Commit-Position: refs/heads/master@{#461877}

[modify] https://crrev.com/ecc1d96e6789d172b45dd155ccdb26b631f1ecb9/base/BUILD.gn
[modify] https://crrev.com/ecc1d96e6789d172b45dd155ccdb26b631f1ecb9/base/allocator/allocator_interception_mac.h
[modify] https://crrev.com/ecc1d96e6789d172b45dd155ccdb26b631f1ecb9/base/allocator/allocator_interception_mac.mm
[delete] https://crrev.com/80b39f01869d523307a6b494167fdf4f3e533b53/base/allocator/allocator_interception_mac_unittest.mm
[modify] https://crrev.com/ecc1d96e6789d172b45dd155ccdb26b631f1ecb9/content/browser/browser_main_loop.cc
[modify] https://crrev.com/ecc1d96e6789d172b45dd155ccdb26b631f1ecb9/content/child/child_thread_impl.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/486716780e8483c70c50c1fc50ecca4112efb5aa

commit 486716780e8483c70c50c1fc50ecca4112efb5aa
Author: erikchen <erikchen@chromium.org>
Date: Wed Apr 05 00:56:16 2017

mac: Periodically shim new malloc zones immediately after startup.

On macOS, libraries lazily make malloc zones. There are no notifications when
this happens. This CL adds polling with an exponential backoff to check for the
existence of new malloc zones, stopping after 1 minute.

This CL only has an affect when heap profiling is enabled.

BUG=693237

Review-Url: https://codereview.chromium.org/2713553002
Cr-Original-Commit-Position: refs/heads/master@{#461836}
Committed: https://chromium.googlesource.com/chromium/src/+/245f17559f973bb5a207c063d982c0bab0ec1c0b
Review-Url: https://codereview.chromium.org/2713553002
Cr-Commit-Position: refs/heads/master@{#461924}

[modify] https://crrev.com/486716780e8483c70c50c1fc50ecca4112efb5aa/base/BUILD.gn
[modify] https://crrev.com/486716780e8483c70c50c1fc50ecca4112efb5aa/base/allocator/allocator_interception_mac.h
[modify] https://crrev.com/486716780e8483c70c50c1fc50ecca4112efb5aa/base/allocator/allocator_interception_mac.mm
[add] https://crrev.com/486716780e8483c70c50c1fc50ecca4112efb5aa/base/allocator/allocator_interception_mac_unittest.mm
[modify] https://crrev.com/486716780e8483c70c50c1fc50ecca4112efb5aa/content/browser/browser_main_loop.cc
[modify] https://crrev.com/486716780e8483c70c50c1fc50ecca4112efb5aa/content/child/child_thread_impl.cc