allocator/Windows: delete the old non-unified allocator-shim

allocator/Windows: delete the old non-unified allocator-shim

There are two "allocator shims" on windows.
1) The deprecated one in allocator_shim_win.cc
2) The current one ("unified shim") in
   allocator_shim_override_ucrt_symbols_win.h
They work in the same exact way, that is, redefining ucrt
symbols like malloc/realloc and routing them to
winheap_stubs_win.cc.

The only reason of the dual shim is purely historical:
wfh@ was developing the VS2015 interposition-based shim while
primiano@ was developing the unified shim on other platforms.
Back then we consciously agreed to proceed in parallel as both
changes were risky.
Later, in crrev.com/2164653002 (54.0.2831.0) siggi@ made the work
to port the unified shim to Windows and enabled the flag that made
it default. Back then we decided to keep the old code around in
case of reverts.

The unified shim has been now been enabled by default for 9 months,
it is time to cleanup 1. This CL, hence, has no functional changes
(other than fixing #ifdefs in tests) and is just removing old
code that has not been used in the last 9 months.

BUG=550886
TBR=thakis

Review-Url: https://codereview.chromium.org/2907563002
Cr-Commit-Position: refs/heads/master@{#475012}
Committed: https://chromium.googlesource.com/chromium/src/+/3038b2f84f7244cda0fce882a108cb315af4b478

[Primiano Tucci (use gerrit), 2017-05-25 11:46:14]

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

[commit-bot: I haz the power, 2017-05-25 11:46:22]

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-05-25 11:49:32]

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

[commit-bot: I haz the power, 2017-05-25 11:49:33]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Primiano Tucci (use gerrit), 2017-05-25 11:50:36]

primiano@chromium.org changed reviewers:
+ siggi@chromium.org, wfh@chromium.org

[Primiano Tucci (use gerrit), 2017-05-25 11:50:36]

Spring cleanup. If I did everything correctly this has no functional changes.

As noted in the .cc file I am removing
-----
// TODO(wfh): Remove this once gyp is dead.	
// TODO(siggi): Remove this file once the generic shim sticks.
-----

Both conditions are now satisfied. Gyp is no more and the generic shim stuck. :)

https://codereview.chromium.org/2907563002/diff/1/base/allocator/BUILD.gn
File base/allocator/BUILD.gn (left):

https://codereview.chromium.org/2907563002/diff/1/base/allocator/BUILD.gn#old...
base/allocator/BUILD.gn:96: "winheap_stubs_win.cc",
this file is staying, but is already also included in base/allocator/BUILD.gn

[Primiano Tucci (use gerrit), 2017-05-25 11:54:51]

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

[commit-bot: I haz the power, 2017-05-25 11:55:00]

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-05-25 12:06:32]

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

[commit-bot: I haz the power, 2017-05-25 12:06:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Sigurður Ásgeirsson, 2017-05-25 12:54:21]

Looks good, though doesn't yet compile :).

It would make sense to fold the winheap_stubs into the default allocator
implementation. Those files don't serve any particular purpose now, and only
cause extra function calls to allocation.
I can take care of it in a separate CL if you like.

https://codereview.chromium.org/2907563002/diff/20001/base/process/memory_uni...
File base/process/memory_unittest.cc (right):

https://codereview.chromium.org/2907563002/diff/20001/base/process/memory_uni...
base/process/memory_unittest.cc:107: #if !defined(OS_OPENBSD) &&
BUILDFLAG(USE_ALLOCATOR_SHIM) && \
_EXPERIMENTAL_ missing?

https://codereview.chromium.org/2907563002/diff/20001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/2907563002/diff/20001/base/security_unittest....
base/security_unittest.cc:50: // - ADDRESS_SANITIZER they it has its own memory
allocator
wonky verbiage here.

https://codereview.chromium.org/2907563002/diff/20001/chrome/installer/setup/...
File chrome/installer/setup/memory_unittest.cc (right):

https://codereview.chromium.org/2907563002/diff/20001/chrome/installer/setup/...
chrome/installer/setup/memory_unittest.cc:17: #if
BUILDFLAG(USE_EXPERIMENTAL_ALLOCATOR_SHIM)
OT: is it not time to drop the _EXPERIMENTAL_ in this buildflag?

[Primiano Tucci (use gerrit), 2017-05-25 13:00:01]

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

[commit-bot: I haz the power, 2017-05-25 13:00:13]

Dry run: CQ is trying da patch.

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

[Primiano Tucci (use gerrit), 2017-05-25 13:01:16]

Ehm I made some rebase mess while fixing the media stuff. This CL is based on
crrev.com/2902043007 where I am getting rid of the EXPERIMENTAL_ 
 while copy/pasting pieces to fix media I re-injected it and this caused the
redness. Letmme retry again.

> It would make sense to fold the winheap_stubs into the default allocator
implementation. Those files don't serve any particular purpose now, and only
cause extra function calls to allocation.

Yup  I realized while cleaning up. was thinking to do that in a separate step to
see if this sticks first. If you want to give a stab to that, that is more than
welcome :)

https://codereview.chromium.org/2907563002/diff/20001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/2907563002/diff/20001/base/security_unittest....
base/security_unittest.cc:50: // - ADDRESS_SANITIZER they it has its own memory
allocator
On 2017/05/25 12:54:21, Sigurður Ásgeirsson wrote:
> wonky verbiage here.

Done.

[Primiano Tucci (use gerrit), 2017-05-25 15:02:34]

Okay now it's green, ptal

[Sigurður Ásgeirsson, 2017-05-25 15:17:31]

lgtm

[Primiano Tucci (use gerrit), 2017-05-25 15:25:31]

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

[Primiano Tucci (use gerrit), 2017-05-25 15:25:32]

+dalecurtis for 5 lines changes in media/base

[Primiano Tucci (use gerrit), 2017-05-25 15:27:29]

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

[Primiano Tucci (use gerrit), 2017-05-25 15:27:29]

+gab for:
4 lines change in chrome/installer/setup/
2 lines in base/process/memory_unittest.cc
8 lines in base/security_unittest.cc

[commit-bot: I haz the power, 2017-05-25 15:40:36]

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

[commit-bot: I haz the power, 2017-05-25 15:40:37]

Dry run: This issue passed the CQ dry run.

[gab, 2017-05-25 16:48:48]

r-s lgtm with question from someone that has no idea what's going on in the
allocator world..!

https://codereview.chromium.org/2907563002/diff/40001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/2907563002/diff/40001/base/security_unittest....
base/security_unittest.cc:51: #if defined(OS_LINUX) && !defined(NO_TCMALLOC) &&
!defined(ADDRESS_SANITIZER)
Comment above says CrOS by this ifdef doesn't? Should this be OS_POSIX &&
!OS_MACOSX?

[Primiano Tucci (use gerrit), 2017-05-25 18:00:14]

https://codereview.chromium.org/2907563002/diff/40001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/2907563002/diff/40001/base/security_unittest....
base/security_unittest.cc:51: #if defined(OS_LINUX) && !defined(NO_TCMALLOC) &&
!defined(ADDRESS_SANITIZER)
On 2017/05/25 16:48:48, gab wrote:
> Comment above says CrOS by this ifdef doesn't? 
there is no such thing like OS_CROS. OS_LINUX means "any linux desktop
environment", so Linux AND cros (but not Android)

> Should this be OS_POSIX && !OS_MACOSX?
Oh no, that will start pulling in OS_NACL, OS_IOS, OS_FUCSIA etc.
I once had a friend who wrote a CL in //base that ended up accidentally being
built as part of nacl. He has been sucked into a black hole and disappeared form
the galaxy :)

[gab, 2017-05-25 18:13:20]

https://codereview.chromium.org/2907563002/diff/40001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/2907563002/diff/40001/base/security_unittest....
base/security_unittest.cc:51: #if defined(OS_LINUX) && !defined(NO_TCMALLOC) &&
!defined(ADDRESS_SANITIZER)
On 2017/05/25 18:00:14, Primiano Tucci wrote:
> On 2017/05/25 16:48:48, gab wrote:
> > Comment above says CrOS by this ifdef doesn't? 
> there is no such thing like OS_CROS. OS_LINUX means "any linux desktop
> environment", so Linux AND cros (but not Android)

OS_CHROMEOS :)
https://cs.chromium.org/search/?q=OS_CHROMEOS&sq=package:chromium&type=cs

but hadn't realized OS_LINUX included OS_CHROMEOS?

> 
> > Should this be OS_POSIX && !OS_MACOSX?
> Oh no, that will start pulling in OS_NACL, OS_IOS, OS_FUCSIA etc.
> I once had a friend who wrote a CL in //base that ended up accidentally being
> built as part of nacl. He has been sucked into a black hole and disappeared
form
> the galaxy :)

[DaleCurtis, 2017-05-25 18:37:35]

lgtm

[Primiano Tucci (use gerrit), 2017-05-25 18:51:08]

https://codereview.chromium.org/2907563002/diff/40001/base/security_unittest.cc
File base/security_unittest.cc (right):

https://codereview.chromium.org/2907563002/diff/40001/base/security_unittest....
base/security_unittest.cc:51: #if defined(OS_LINUX) && !defined(NO_TCMALLOC) &&
!defined(ADDRESS_SANITIZER)
On 2017/05/25 18:13:20, gab wrote:
> On 2017/05/25 18:00:14, Primiano Tucci wrote:
> > On 2017/05/25 16:48:48, gab wrote:
> > > Comment above says CrOS by this ifdef doesn't? 
> > there is no such thing like OS_CROS. OS_LINUX means "any linux desktop
> > environment", so Linux AND cros (but not Android)
> 
> OS_CHROMEOS :)
> https://cs.chromium.org/search/?q=OS_CHROMEOS&sq=package:chromium&type=cs
> 

^__^ "back in my days there was no OS_CHROMEOS" (insert old man meme here) 

> but hadn't realized OS_LINUX included OS_CHROMEOS?
Yeah that is just tied to__linux__ which any Linux-based desktop toolchain sets.

I'm fact there are a bunch of if defined(Os_linux)  && !is_chromeos around the
codebase for that reason  

> 
> > 
> > > Should this be OS_POSIX && !OS_MACOSX?
> > Oh no, that will start pulling in OS_NACL, OS_IOS, OS_FUCSIA etc.
> > I once had a friend who wrote a CL in //base that ended up accidentally
being
> > built as part of nacl. He has been sucked into a black hole and disappeared
> form
> > the galaxy :)
>

[Will Harris, 2017-05-26 01:40:00]

Can the TODO in build/config/allocator.gni be removed now?

[Primiano Tucci (use gerrit), 2017-05-26 09:36:56]

On 2017/05/26 01:40:00, Will Harris (SYD) wrote:
> Can the TODO in build/config/allocator.gni be removed now?

Ah, well spotted thanks.
Yes, I just removed the comment. However the flag has to stay there.
There are some very niche project that build chrome with very esoteric
configuration (like internal projects that fork chromium and build with with
uClibc, ping me on IM for more details) which still need to be able to set
use_allocator_shim=false as an escape hatch.

[Primiano Tucci (use gerrit), 2017-05-26 09:37:29]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2017-05-26 09:37:29]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
siggi@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2907563002/#ps60001
(title: "remove comment")

[commit-bot: I haz the power, 2017-05-26 09:37:39]

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-05-26 09:45:10]

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

[commit-bot: I haz the power, 2017-05-26 09:45:11]

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-05-26 15:43:24]

Description was changed from

==========
allocator/Windows: delete the old non-unified allocator-shim

There are two "allocator shims" on windows.
1) The deprecated one in allocator_shim_win.cc
2) The current one ("unified shim") in
   allocator_shim_override_ucrt_symbols_win.h
They work in the same exact way, that is, redefining ucrt
symbols like malloc/realloc and routing them to
winheap_stubs_win.cc.

The only reason of the dual shim is purely historical:
wfh@ was developing the VS2015 interposition-based shim while
primiano@ was developing the unified shim on other platforms.
Back then we consciously agreed to proceed in parallel as both
changes were risky.
Later, in crrev.com/2164653002 (54.0.2831.0) siggi@ made the work
to port the unified shim to Windows and enabled the flag that made
it default. Back then we decided to keep the old code around in
case of reverts.

The unified shim has been now been enabled by default for 9 months,
it is time to cleanup 1. This CL, hence, has no functional changes
(other than fixing #ifdefs in tests) and is just removing old
code that has not been used in the last 9 months.

BUG=550886
==========

to

==========
allocator/Windows: delete the old non-unified allocator-shim

There are two "allocator shims" on windows.
1) The deprecated one in allocator_shim_win.cc
2) The current one ("unified shim") in
   allocator_shim_override_ucrt_symbols_win.h
They work in the same exact way, that is, redefining ucrt
symbols like malloc/realloc and routing them to
winheap_stubs_win.cc.

The only reason of the dual shim is purely historical:
wfh@ was developing the VS2015 interposition-based shim while
primiano@ was developing the unified shim on other platforms.
Back then we consciously agreed to proceed in parallel as both
changes were risky.
Later, in crrev.com/2164653002 (54.0.2831.0) siggi@ made the work
to port the unified shim to Windows and enabled the flag that made
it default. Back then we decided to keep the old code around in
case of reverts.

The unified shim has been now been enabled by default for 9 months,
it is time to cleanup 1. This CL, hence, has no functional changes
(other than fixing #ifdefs in tests) and is just removing old
code that has not been used in the last 9 months.

BUG=550886
TBR=thakis
==========

[Primiano Tucci (use gerrit), 2017-05-26 15:43:24]

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

[Primiano Tucci (use gerrit), 2017-05-26 15:43:45]

TBR thakis for removing 2 comment lines from build/config/allocator.gni

[Primiano Tucci (use gerrit), 2017-05-26 15:43:50]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2017-05-26 15:44:17]

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-05-26 15:48:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495813430557430,
"parent_rev": "72c95d62a11bebbf3e0940806eabd3aaaf5e9e72", "commit_rev":
"3038b2f84f7244cda0fce882a108cb315af4b478"}

[commit-bot: I haz the power, 2017-05-26 15:48:46]

Description was changed from

==========
allocator/Windows: delete the old non-unified allocator-shim

There are two "allocator shims" on windows.
1) The deprecated one in allocator_shim_win.cc
2) The current one ("unified shim") in
   allocator_shim_override_ucrt_symbols_win.h
They work in the same exact way, that is, redefining ucrt
symbols like malloc/realloc and routing them to
winheap_stubs_win.cc.

The only reason of the dual shim is purely historical:
wfh@ was developing the VS2015 interposition-based shim while
primiano@ was developing the unified shim on other platforms.
Back then we consciously agreed to proceed in parallel as both
changes were risky.
Later, in crrev.com/2164653002 (54.0.2831.0) siggi@ made the work
to port the unified shim to Windows and enabled the flag that made
it default. Back then we decided to keep the old code around in
case of reverts.

The unified shim has been now been enabled by default for 9 months,
it is time to cleanup 1. This CL, hence, has no functional changes
(other than fixing #ifdefs in tests) and is just removing old
code that has not been used in the last 9 months.

BUG=550886
TBR=thakis
==========

to

==========
allocator/Windows: delete the old non-unified allocator-shim

There are two "allocator shims" on windows.
1) The deprecated one in allocator_shim_win.cc
2) The current one ("unified shim") in
   allocator_shim_override_ucrt_symbols_win.h
They work in the same exact way, that is, redefining ucrt
symbols like malloc/realloc and routing them to
winheap_stubs_win.cc.

The only reason of the dual shim is purely historical:
wfh@ was developing the VS2015 interposition-based shim while
primiano@ was developing the unified shim on other platforms.
Back then we consciously agreed to proceed in parallel as both
changes were risky.
Later, in crrev.com/2164653002 (54.0.2831.0) siggi@ made the work
to port the unified shim to Windows and enabled the flag that made
it default. Back then we decided to keep the old code around in
case of reverts.

The unified shim has been now been enabled by default for 9 months,
it is time to cleanup 1. This CL, hence, has no functional changes
(other than fixing #ifdefs in tests) and is just removing old
code that has not been used in the last 9 months.

BUG=550886
TBR=thakis

Review-Url: https://codereview.chromium.org/2907563002
Cr-Commit-Position: refs/heads/master@{#475012}
Committed:
https://chromium.googlesource.com/chromium/src/+/3038b2f84f7244cda0fce882a108...
==========

[commit-bot: I haz the power, 2017-05-26 15:48:48]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3038b2f84f7244cda0fce882a108...