Hook up allocator shim on mac.

Hook up allocator shim on mac.

This CL should have no behavioral effect, since the relevant code is not enabled
yet.

allocator_shim_override_mac_symbols.h intercepts heap allocations using the
newly introduced APIs in allocator_interception_mac.h and redirects them to the
allocator shim. This eventually forwards to AllocatorDispatch::default_dispatch,
which in turns calls the original malloc zone functions..

BUG=665567
Review-Url: https://codereview.chromium.org/2658723007
Cr-Commit-Position: refs/heads/master@{#447705}
Committed: https://chromium.googlesource.com/chromium/src/+/0d0395aeb25b9ab1422ab547f976d049a14e192f

[erikchen, 2017-01-27 02:05:42]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 02:06:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 02:18:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 02:18:48]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[erikchen, 2017-01-27 22:23:59]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 22:25:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 22:28:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:28:32]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2017-01-27 22:39:28]

Description was changed from

==========
Hook up allocator shim on mac.

BUG=
==========

to

==========
Hook up allocator shim on mac.

This CL should have no behavioral effect, since the relevant code is not enabled
yet.

allocator_shim_override_mac_symbols.h intercepts heap allocations using the
newly introduced APIs in allocator_interception_mac.h and redirects them to the
allocator shim. This eventually forwards to AllocatorDispatch::default_dispatch,
which in turns calls the original malloc zone functions..

BUG=665567
==========

[erikchen, 2017-01-27 22:42:15]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 22:42:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-01-27 22:42:52]

erikchen@chromium.org changed reviewers:
+ primiano@chromium.org

[erikchen, 2017-01-27 22:42:53]

primiano: Please review.

[DmitrySkiba, 2017-01-27 23:00:53]

dskiba@chromium.org changed reviewers:
+ dskiba@chromium.org

[DmitrySkiba, 2017-01-27 23:00:53]

Continuing from 2601573002:

> I hooked up free_definite_size.

Yes, it's hooked, but what I meant was that you also need to add an entry to
AllocatorDispatch, just as you did for batch_* functions. Because right now
InitializeAllocatorShim() replaces default zone's free_definite_size() with a
version that calls ShimFree(), discarding size information. It should call
ShimFreeDefiniteSize() instead, just as batch_ functions do.

> #ifdef is not a good way of marking this as mac-only, since that pollutes
#ifdefs in all the files. 

This is exactly my intention. I don't see it as a pollution though, I see at as
a clear communication of the fact that some functionality in AllocationDispatch
is macOS-only. I.e. if I were to write something that doesn't build on macOS, I
would not care about those functions.

[erikchen, 2017-01-27 23:04:04]

> > #ifdef is not a good way of marking this as mac-only, since that pollutes
> #ifdefs in all the files. 
> 
> This is exactly my intention. I don't see it as a pollution though, I see at
as
> a clear communication of the fact that some functionality in
AllocationDispatch
> is macOS-only. I.e. if I were to write something that doesn't build on macOS,
I
> would not care about those functions.

This sounds like we disagree on aesthetics. I'm happy to let primiano@ decide -
any objections?

[commit-bot: I haz the power, 2017-01-28 00:12:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 00:12:04]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-01-28 01:46:24]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 01:47:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 01:55:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 01:55:41]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Primiano Tucci (use gerrit), 2017-01-28 05:10:03]

thanks a lot for the progress here.
It looks in a good state. The only thing that I find confusing is the
initialization logic. I had to bunny hop between 4 files to figure out what is
going on. I think with some very minor changes it can become all more readable
(See below).

Also, while reviewing thisI realized that the names became very confusing as now
we have 4 files that all claim to to "replacements" or "interceptions". However,
there is already enough stuff in this CL, so I'd postpone a general renaming
pass to a follow up CL, so we can bikeshed on naming separately with a fresh
mind. This CL is already complicated enough :)

So, back to this one.

I understand you need a InitializeAllocatorShim(). In the other platforms I
always assumed both the interception and dispatch can be done at compile time.
this is not true anymore on Mac, and you need to do that dynamically. I get
this. 
Conceptually you need to do two things here:
 1. replace the default malloc zone, so that malloc calls are routed into the
shim (ShimMalloc & co)
 2. prepare the default_dispatch, so that, after traversing the shim, you end up
 in the default malloc zone.
(I know, strictly code-wise they need to happen in reverse order, 2 first and
then 1)
 
I would do the following:
1. define InitializeAllocatorShim in allocator_shim.cc. That will reduce the
   surprise factor. InitializeAllocatorShim is declared in allocator_shim.h
   so the definition will happen where one would expect, i.e. in the
   corresponding .cc file)

In allocator_shim.cc, just do the following:
#if defined(OS_MACOSX)
void InitializeAllocatorShim() {
  // Prepares the default dispatch. After the intercepted malloc calls have
  // traversed the shim this will route them to the default malloc zone. 
  InitializeDefaultDispatchToMacAllocator();  // name this accordingly to the
default_dispatch_to.... file

  // This replaces the default malloc zone, causing calls to malloc & friends
  // from the codebase to be routed to ShimMalloc() above.
  OverrideMacSymbols();  // name this the accordingly to the
allocator_shim_override_.... file. 
}
#endif

2. in allocator_shim_default_dispatch_to_zoned_malloc.cc
   rename that SetDefaultDispatchMallocZoneFunctions to
   InitializeDefaultDispatchToMacAllocator()
   I think there you just need to do:

  void InitializeDefaultDispatchToMacAllocator() {
     StoreDefaultZoneFunctions(&g_default_zone);
  }

3. in allocator_shim_override_mac_symbols.h:
   rename InitializeAllocatorShim() to OverrideMacSymbols()
   and remove the initial 3 lines
 

Other minor comments inline

https://codereview.chromium.org/2658723007/diff/60001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2658723007/diff/60001/base/BUILD.gn#newcode1149
base/BUILD.gn:1149:
"allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc",
since this is mac only, can those two files have "mac" in their name? will make
it easier to make a sense out of the base/Allocator directory. proposals:

allocator_shim_default_dispatch_to_mac_zoned_malloc.{cc,h}
allocator_shim_default_dispatch_to_mac_malloc.{cc,h}
allocator_shim_default_dispatch_to_mac_system_allocator.{cc,h}
or anything else with "mac" in the name

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.h (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_interception_mac.h:69: void InterceptAllocationsMac();
in a follow up CL we should sort out a bit these names. Now we have two parallel
systems doing interceptions, so this name might be confusing. Anyhow, let's deal
later with this, there is already enough stuff here.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.mm (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:364: void
StoreDefaultZoneFunctions(MallocZoneFunctions* functions) {
purely a naming thing, but this tripped my brain for a while before I really
understood what was going on.
What you are trying to convey here is: "Store the functions for the default
zone" 
Instead, confusingly, I got "Store the default functions for the zone" 

In order to avoid the ambiguity can this be called
just StoreMallocZoneFunctions
or
StoreFunctionsForDefaultZone
or anything else that solves the ambiguity

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim.h (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim.h:110: #if defined(OS_MACOSX)
Was this the argument of the discussion with dskiba?
Honestly I like the #ifdef here, as it will make explicit in content_main (or
wherever this will be called) that this happens only on OSX.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc:23: auto
CallUnshimmed(F ChromeMallocZone::*m, Args... args)
This is the part I am really missing here.
I am not talking about the template blackmagic. I just could not figure out why
do you need this fallback CallUnshimmed logic.
You should never end up in a situation where the MallocImpl function is called
but the g_default_zone is null. What am I missing?

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc:94: void
SetDefaultDispatchMallocZoneFunctions(MallocZoneFunctions* functions) {
this one should be just:
void InitializeDefaultDispatchToMacAllocator() {
   StoreDefaultZoneFunctions(&g_default_zone);
}

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_override_cpp_symbols.h (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_override_cpp_symbols.h:17: #if defined(OS_MACOSX)
just checking: do we actually need these?
IIRC the reason why I had to introduce them was because the stdlibc++ on linux
was directly going to __libc_malloc, so was bypassing the malloc-shim.
For instance, on Windows we don't need to override the C++ symbols because they
under the hood end up through malloc.
I have no idea about the current situation on Mac OS. My only question is: what
do we do today for the suicide-on-OOM feature? IIRC on Mac today we don't
override the c++ symbols.
Essentially what I am saying here is:
- either the current suicide-on-oom logic is bugged, and we don't suicide on
"new" failure
OR
- this is unnecessary and you can just piggy back on the #if !defined(OS_WIN) of
allocator_shim.cc:246

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_override_mac_symbols.h (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_override_mac_symbols.h:17: void
InitializeAllocatorShim() {
I'd rename this to OverrideMacSymbols, which will make it match the file name,
and hence less surprising and easier to find in codeserach.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_override_mac_symbols.h:18: MallocZoneFunctions
functions;
As said above, these 3 lines are really unnecessary here and can just be done in

allocator_shim_default_dispatch_to_zoned_malloc.cc :
InitializeDefaultDispatchToMacAllocator()

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_unittest.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_unittest.cc:93: // Hitting it for the first time
will always cause a failure, causing the
I'd remove the  "always", as it is confusing. The failure happens only on the
1st hit. it doesn't makes too much sense to me saying "always" for something
that happens only once (or maybe I am mis-reading all this?)

https://codereview.chromium.org/2658723007/diff/60001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:103: if (to_be_freed[i] != nullptr) {
do you know if this will actually happen frequenty or not?
The code of MallocDumpProvider can already tolerate RemoveAllocation(nullptr).
it is not super-smart about it (it will still take the lock and attempt a
hashtable lookup) but won't crash.
So if this is just for paranoid cases, you can avoid this extra check here.

https://codereview.chromium.org/2658723007/diff/60001/chrome/app/framework.order
File chrome/app/framework.order (right):

https://codereview.chromium.org/2658723007/diff/60001/chrome/app/framework.or...
chrome/app/framework.order:18: # Overrides for C++ new/delete operators used by
base/allocator.
see my comment above about overriding c++ symbols

https://codereview.chromium.org/2658723007/diff/60001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/content/app/content_mai...
content/app/content_main_runner.cc:477: #if defined(OS_MACOSX) &&
BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM)
I would probably leave this for later, once we have cleaned up naming and
everything. It might be easier for content owners to get all this when the rest
of the shim code is in codeserach.
Also I need to reason a bit more to how the suicide-on-OOM is going to work now
end to end, but I have to honestly admit that I exhausted my brain power to do
so right now.

[erikchen, 2017-01-30 23:01:12]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 23:02:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-01-31 00:21:10]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 00:21:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 01:47:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 01:47:29]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-01-31 02:21:22]

primiano: Please review.

https://codereview.chromium.org/2658723007/diff/60001/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2658723007/diff/60001/base/BUILD.gn#newcode1149
base/BUILD.gn:1149:
"allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc",
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> since this is mac only, can those two files have "mac" in their name? will
make
> it easier to make a sense out of the base/Allocator directory. proposals:
> 
> allocator_shim_default_dispatch_to_mac_zoned_malloc.{cc,h}
> allocator_shim_default_dispatch_to_mac_malloc.{cc,h}
> allocator_shim_default_dispatch_to_mac_system_allocator.{cc,h}
> or anything else with "mac" in the name 

Done.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.h (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_interception_mac.h:69: void InterceptAllocationsMac();
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> in a follow up CL we should sort out a bit these names. Now we have two
parallel
> systems doing interceptions, so this name might be confusing. Anyhow, let's
deal
> later with this, there is already enough stuff here.

Noted.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_interception_mac.mm (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_interception_mac.mm:364: void
StoreDefaultZoneFunctions(MallocZoneFunctions* functions) {
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> purely a naming thing, but this tripped my brain for a while before I really
> understood what was going on.
> What you are trying to convey here is: "Store the functions for the default
> zone" 
> Instead, confusingly, I got "Store the default functions for the zone" 
> 
> In order to avoid the ambiguity can this be called
> just StoreMallocZoneFunctions
> or
> StoreFunctionsForDefaultZone
> or anything else that solves the ambiguity
 
changed name to StoreFunctionsForDefaultZone

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim.h (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim.h:110: #if defined(OS_MACOSX)
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> Was this the argument of the discussion with dskiba?
> Honestly I like the #ifdef here, as it will make explicit in content_main (or
> wherever this will be called) that this happens only on OSX.

No, the discussion was about whether the members batch_malloc_function,
batch_free_function, and free_definite_size_function should be if-defed.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc:23: auto
CallUnshimmed(F ChromeMallocZone::*m, Args... args)
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> This is the part I am really missing here.
> I am not talking about the template blackmagic. I just could not figure out
why
> do you need this fallback CallUnshimmed logic.
> You should never end up in a situation where the MallocImpl function is called
> but the g_default_zone is null. What am I missing?

This is needed because ThreadHeapUsageTracker::Start is called before
g_default_zone is set.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc:94: void
SetDefaultDispatchMallocZoneFunctions(MallocZoneFunctions* functions) {
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> this one should be just:
> void InitializeDefaultDispatchToMacAllocator() {
>    StoreDefaultZoneFunctions(&g_default_zone);
> }

Done.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_override_cpp_symbols.h (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_override_cpp_symbols.h:17: #if defined(OS_MACOSX)
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> just checking: do we actually need these?
> IIRC the reason why I had to introduce them was because the stdlibc++ on linux
> was directly going to __libc_malloc, so was bypassing the malloc-shim.
> For instance, on Windows we don't need to override the C++ symbols because
they
> under the hood end up through malloc.
> I have no idea about the current situation on Mac OS. My only question is:
what
> do we do today for the suicide-on-OOM feature? IIRC on Mac today we don't
> override the c++ symbols.
> Essentially what I am saying here is:
> - either the current suicide-on-oom logic is bugged, and we don't suicide on
> "new" failure
> OR
> - this is unnecessary and you can just piggy back on the #if !defined(OS_WIN)
of
> allocator_shim.cc:246

We don't, the implementation of libc++.dylib forwards to default malloc zone.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_override_mac_symbols.h (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_override_mac_symbols.h:17: void
InitializeAllocatorShim() {
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> I'd rename this to OverrideMacSymbols, which will make it match the file name,
> and hence less surprising and easier to find in codeserach.

Done.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_override_mac_symbols.h:18: MallocZoneFunctions
functions;
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> As said above, these 3 lines are really unnecessary here and can just be done
in
> 
> allocator_shim_default_dispatch_to_zoned_malloc.cc :
> InitializeDefaultDispatchToMacAllocator()

Done.

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_unittest.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_unittest.cc:93: // Hitting it for the first time
will always cause a failure, causing the
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> I'd remove the  "always", as it is confusing. The failure happens only on the
> 1st hit. it doesn't makes too much sense to me saying "always" for something
> that happens only once (or maybe I am mis-reading all this?) 

Done.

https://codereview.chromium.org/2658723007/diff/60001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:103: if (to_be_freed[i] != nullptr) {
On 2017/01/28 05:10:02, Primiano Tucci wrote:
> do you know if this will actually happen frequenty or not?
> The code of MallocDumpProvider can already tolerate RemoveAllocation(nullptr).
> it is not super-smart about it (it will still take the lock and attempt a
> hashtable lookup) but won't crash.
> So if this is just for paranoid cases, you can avoid this extra check here.

*shrug*, okay. I was just mirroring the format of HookFree.

https://codereview.chromium.org/2658723007/diff/60001/chrome/app/framework.order
File chrome/app/framework.order (right):

https://codereview.chromium.org/2658723007/diff/60001/chrome/app/framework.or...
chrome/app/framework.order:18: # Overrides for C++ new/delete operators used by
base/allocator.
On 2017/01/28 05:10:03, Primiano Tucci wrote:
> see my comment above about overriding c++ symbols

Done.

https://codereview.chromium.org/2658723007/diff/60001/content/app/content_mai...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/content/app/content_mai...
content/app/content_main_runner.cc:477: #if defined(OS_MACOSX) &&
BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM)
On 2017/01/28 05:10:03, Primiano Tucci wrote:
> I would probably leave this for later, once we have cleaned up naming and
> everything. It might be easier for content owners to get all this when the
rest
> of the shim code is in codeserach.
> Also I need to reason a bit more to how the suicide-on-OOM is going to work
now
> end to end, but I have to honestly admit that I exhausted my brain power to do
> so right now.

shrug, okay, done.

[DmitrySkiba, 2017-01-31 02:33:37]

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc:23: auto
CallUnshimmed(F ChromeMallocZone::*m, Args... args)
On 2017/01/31 02:21:22, erikchen wrote:
> On 2017/01/28 05:10:02, Primiano Tucci wrote:
> > This is the part I am really missing here.
> > I am not talking about the template blackmagic. I just could not figure out
> why
> > do you need this fallback CallUnshimmed logic.
> > You should never end up in a situation where the MallocImpl function is
called
> > but the g_default_zone is null. What am I missing?
> 
> This is needed because ThreadHeapUsageTracker::Start is called before
> g_default_zone is set.

Interesting. Originally I added this because in tests some global objects were
allocating memory before InitializeShim() (which was __attribute__(constructor)
at that time).

[erikchen, 2017-01-31 04:10:18]

> Interesting. Originally I added this because in tests some global objects were
> allocating memory before InitializeShim() (which was
__attribute__(constructor)
> at that time).

dskiba, note that your original CL also shimmed the cpp new operator, which
forwarded directly into the allocator shim. So any code that direction or
indirectly causes a C++ allocation [e.g. logging] would also require this logic.

[Primiano Tucci (use gerrit), 2017-01-31 17:40:05]

Everything % 1 thing looks great to me. Thanks for the patience and also thanks
for updating the tests. I guarantee that on average I am less of a pain as a
reviewer :)

So the only open question is still those CallUnshimmed.
I am not saying that they are wrong. I am just saying that is still not clear to
me what is the scenario that ends up requiring them. If there is any allocation
happening early, that should not enter the shim in the first place.
The only problem I can see right now is if something does a call to
InsertAllocatorDispatcher before InitializeAllocatorShim is called. In which
case nothing still should crash, just the InsertAllocatorDispatcher would have
no effect (unrelated: should we a CHECK(g_is_initialized) to make this case more
explicit?)
But this case still wouldn't require CallUnshimmed, as that code should not be
hit.
So I guess I am still missing something?

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
File base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc (right):

https://codereview.chromium.org/2658723007/diff/60001/base/allocator/allocato...
base/allocator/allocator_shim_default_dispatch_to_zoned_malloc.cc:23: auto
CallUnshimmed(F ChromeMallocZone::*m, Args... args)
On 2017/01/31 02:21:22, erikchen wrote:
> On 2017/01/28 05:10:02, Primiano Tucci wrote:
> > This is the part I am really missing here.
> > I am not talking about the template blackmagic. I just could not figure out
> why
> > do you need this fallback CallUnshimmed logic.
> > You should never end up in a situation where the MallocImpl function is
called
> > but the g_default_zone is null. What am I missing?
> 
> This is needed because ThreadHeapUsageTracker::Start is called before
> g_default_zone is set.

Hmm. I can see how that would cause the test to fail. I cannot see how you would
end up inside here in a state where the default zone is replaced (so the
override part is initialized) but this dispatch part is not ready.
ThreadHeapUsageTracker::Start  calls InsertAllocatorDispatch() inserting some
harness.
At this point you should be in a state where there is an element (the test's
dispatcher) in the shim chain, but nothing can't possibly entering the shim
chain, because nothing called InitializeAllocatorShim().  So if the code path
(g_default_zone.malloc == nullptr) is hit, I am still missing something.

If, otherwise, the problem is that the expectations of those test are failing,
the solution should be adding a call to InitializeAllocatorShim() in the test
initialization.

https://codereview.chromium.org/2658723007/diff/120001/base/allocator/allocat...
File base/allocator/allocator_shim_unittest.cc (right):

https://codereview.chromium.org/2658723007/diff/120001/base/allocator/allocat...
base/allocator/allocator_shim_unittest.cc:355: #endif  // defined(OS_MACOSX)
you could just fall through here and just use one #if-#endif block for both
these functions

https://codereview.chromium.org/2658723007/diff/120001/base/allocator/allocat...
base/allocator/allocator_shim_unittest.cc:411: // then does a realloc(<new
memory>, 0xDEAD).
wasn't this 0xFEED?

[erikchen, 2017-01-31 18:25:12]

On 2017/01/31 17:40:05, Primiano Tucci wrote:
> Everything % 1 thing looks great to me. Thanks for the patience and also
thanks
> for updating the tests. I guarantee that on average I am less of a pain as a
> reviewer :)
> 
> So the only open question is still those CallUnshimmed.
> I am not saying that they are wrong. I am just saying that is still not clear
to
> me what is the scenario that ends up requiring them. If there is any
allocation
> happening early, that should not enter the shim in the first place.
> The only problem I can see right now is if something does a call to
> InsertAllocatorDispatcher before InitializeAllocatorShim is called. In which
> case nothing still should crash, just the InsertAllocatorDispatcher would have
> no effect (unrelated: should we a CHECK(g_is_initialized) to make this case
more
> explicit?)
> But this case still wouldn't require CallUnshimmed, as that code should not be
> hit.
> So I guess I am still missing something?

GetOrCreateThreadUsage() calls alloc_function
https://cs.chromium.org/chromium/src/base/debug/thread_heap_usage_tracker.cc?...

The relevant flag will almost always be on when the allocator shim is enabled:
https://cs.chromium.org/chromium/src/base/BUILD.gn?type=cs&q=ENABLE_MEMORY_TA...
https://cs.chromium.org/chromium/src/base/tracked_objects.cc?type=cs&l=969

[erikchen, 2017-01-31 18:27:53]

https://codereview.chromium.org/2658723007/diff/120001/base/allocator/allocat...
File base/allocator/allocator_shim_unittest.cc (right):

https://codereview.chromium.org/2658723007/diff/120001/base/allocator/allocat...
base/allocator/allocator_shim_unittest.cc:355: #endif  // defined(OS_MACOSX)
On 2017/01/31 17:40:05, Primiano Tucci wrote:
> you could just fall through here and just use one #if-#endif block for both
> these functions

Done.

https://codereview.chromium.org/2658723007/diff/120001/base/allocator/allocat...
base/allocator/allocator_shim_unittest.cc:411: // then does a realloc(<new
memory>, 0xDEAD).
On 2017/01/31 17:40:05, Primiano Tucci wrote:
> wasn't this 0xFEED?

Done.

[erikchen, 2017-01-31 18:27:59]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 18:28:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 19:43:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:43:37]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-01-31 20:03:16]

On 2017/01/31 18:27:53, erikchen wrote:
>
https://codereview.chromium.org/2658723007/diff/120001/base/allocator/allocat...
> File base/allocator/allocator_shim_unittest.cc (right):
> 
>
https://codereview.chromium.org/2658723007/diff/120001/base/allocator/allocat...
> base/allocator/allocator_shim_unittest.cc:355: #endif  // defined(OS_MACOSX)
> On 2017/01/31 17:40:05, Primiano Tucci wrote:
> > you could just fall through here and just use one #if-#endif block for both
> > these functions
> 
> Done.
> 
>
https://codereview.chromium.org/2658723007/diff/120001/base/allocator/allocat...
> base/allocator/allocator_shim_unittest.cc:411: // then does a realloc(<new
> memory>, 0xDEAD).
> On 2017/01/31 17:40:05, Primiano Tucci wrote:
> > wasn't this 0xFEED?
> 
> Done.

Talked with primiano@ offline. He said he's going to try to fix
ThreadHeapUsageTracker to not call alloc_function.

[erikchen, 2017-01-31 20:06:09]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 20:06:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[erikchen, 2017-01-31 20:06:36]

erikchen@chromium.org changed reviewers:
+ mark@chromium.org

[erikchen, 2017-01-31 20:06:36]

mark: Please review.

[Mark Mentovai, 2017-01-31 20:27:05]

LGTM

[commit-bot: I haz the power, 2017-01-31 21:53:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 21:53:32]

Dry run: This issue passed the CQ dry run.

[erikchen, 2017-01-31 22:00:34]

On 2017/01/31 20:27:05, Mark Mentovai wrote:
> LGTM

Landing this, since I've also addressed all points from primiano.

[erikchen, 2017-01-31 22:00:41]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2017-01-31 22:01:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 22:15:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 22:15:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Primiano Tucci (use gerrit), 2017-01-31 22:17:02]

Ok I spent some time debugging ThreadHeapUsageTracker, here's what's going on.
I didn't realize that ThreadHeapUsageTracker is enabled by default on
non-official builds (But yup, I was on the review, so not blaming anybody here).


Here's what happening:
ThreadHeapUsageTracker effectively inserts an allocator dispatcher when a thread
is started. Specifically by this path:
    base::debug::ThreadHeapUsageTracker::EnableHeapTracking()               
thread_heap_usage_tracker.cc
    tracked_objects::ThreadData::EnsureTlsInitialization()                  
tracked_objects.cc          
    tracked_objects::ThreadData::InitializeThreadContext(std::string const&)
tracked_objects.cc          
    base::PlatformThread::SetName(std::string const&)                       
platform_thread_mac.mm      
    base::Thread::ThreadMain()                                              
thread.cc                   
  
now the problem here is that the code of the dispatcher inserted by
EnableHeapTracking() directly calls in the shim (Specifically here
https://cs.chromium.org/chromium/src/base/debug/thread_heap_usage_tracker.cc?...).

This was fine until now, because there was no initialization. but now  the shim
has an initialization, cause this is the way stuff works on OSX. and now, by the
time we hit that path, the shim is not initialized.

The possible solutions to this are:
1. Making thread_heap_usage_tracker stop directly entering the shim (if not via
malloc)
2. Initialize the shim at the same time the heap tracker is initialized. that
place would be ThreadData::EnsureTlsInitialization() right before
EnableHeapTracking() is called.

I find 1 cleaner and I'll try to fix it myself. I also have to re-think about
the all thread heap usage tracker thingy now that we need a Shim::Initialization
(e.g., should it be allowed to InsertAllocatorDispatch before the shim is
initialized?).
However all this is outside of the scope of this CL, and more in general not
related with what you are doing. you just happened to collide with that..
I'm going to deal with that very soon. if I fail to do that, I just verified
that Option 2 works. so, in worst case scenario, you will be unblocked with a 2
line change.

And, having said that, LGTM. Sorry for being a pain here. Just trying to keep
all the planets aligned :P

[Primiano Tucci (use gerrit), 2017-01-31 22:26:17]

primiano@chromium.org changed reviewers:
+ brettw@chromium.org

[Primiano Tucci (use gerrit), 2017-01-31 22:26:18]

+brettw for adding is_mac condition to build/config/allocator.gni

[Sigurður Ásgeirsson, 2017-01-31 23:14:34]

On Tue, Jan 31, 2017 at 2:17 PM <primiano@chromium.org> wrote:

> The possible solutions to this are:
> 1. Making thread_heap_usage_tracker stop directly entering the shim (if
> not via
> malloc)
>
> Freeing the per-thread structure has to call down to next lower shim, as
re-entering the heap shim chain at the top will re-initialize the TLS slot
being freed:
See <
https://cs.chromium.org/chromium/src/base/debug/thread_heap_usage_tracker.cc?...
>.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Primiano Tucci (use gerrit), 2017-02-01 22:07:10]

https://codereview.chromium.org/2658723007/diff/160001/base/allocator/allocat...
File base/allocator/allocator_shim_unittest.cc (right):

https://codereview.chromium.org/2658723007/diff/160001/base/allocator/allocat...
base/allocator/allocator_shim_unittest.cc:250: TEST_F(AllocatorShimTest, asdf) {
I might be a naming freak, but this seems quite easy to object :P
(very likely you just forgot to remove this)

[erikchen, 2017-02-01 23:10:03]

https://codereview.chromium.org/2658723007/diff/160001/base/allocator/allocat...
File base/allocator/allocator_shim_unittest.cc (right):

https://codereview.chromium.org/2658723007/diff/160001/base/allocator/allocat...
base/allocator/allocator_shim_unittest.cc:250: TEST_F(AllocatorShimTest, asdf) {
On 2017/02/01 22:07:10, Primiano Tucci wrote:
> I might be a naming freak, but this seems quite easy to object :P
> (very likely you just forgot to remove this)

Done.

[erikchen, 2017-02-01 23:10:50]

erikchen@chromium.org changed reviewers:
+ dpranke@chromium.org

[erikchen, 2017-02-01 23:10:51]

dpranke: Please review build/config/allocator.gni.

[Dirk Pranke, 2017-02-02 01:33:36]

lgtm

[erikchen, 2017-02-02 02:04:33]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-02-02 02:04:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2658723007/#ps180001
(title: "remove a debugging test.")

[commit-bot: I haz the power, 2017-02-02 02:05:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 06:16:33]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1486001073791850,
"parent_rev": "26df0a66691fd1d8d451a1dc701ae4682569a676", "commit_rev":
"0d0395aeb25b9ab1422ab547f976d049a14e192f"}

[commit-bot: I haz the power, 2017-02-02 06:18:00]

Description was changed from

==========
Hook up allocator shim on mac.

This CL should have no behavioral effect, since the relevant code is not enabled
yet.

allocator_shim_override_mac_symbols.h intercepts heap allocations using the
newly introduced APIs in allocator_interception_mac.h and redirects them to the
allocator shim. This eventually forwards to AllocatorDispatch::default_dispatch,
which in turns calls the original malloc zone functions..

BUG=665567
==========

to

==========
Hook up allocator shim on mac.

This CL should have no behavioral effect, since the relevant code is not enabled
yet.

allocator_shim_override_mac_symbols.h intercepts heap allocations using the
newly introduced APIs in allocator_interception_mac.h and redirects them to the
allocator shim. This eventually forwards to AllocatorDispatch::default_dispatch,
which in turns calls the original malloc zone functions..

BUG=665567

Review-Url: https://codereview.chromium.org/2658723007
Cr-Commit-Position: refs/heads/master@{#447705}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d0395aeb25b9ab1422ab547f976...
==========

[commit-bot: I haz the power, 2017-02-02 06:18:01]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/0d0395aeb25b9ab1422ab547f976...