Add run-time CHECK to smoke-test allocator overrides

Add run-time CHECK to smoke-test allocator overrides

As part of the refactoring work on the allocator, it emerged that it
would be nice to have a test that ensures that we don't accidentally
break things along the way and not detected that.
This CL adds a runtime CHECK() that verifies that:
 - On Windows (non-component build) the shim layer has been
   initialized. Concretely this checks that chrome called the shimmed
   _heap_init() method and not the one from libcmt.
 - On Linux desktop, the malloc symbols are being overridden by tcmalloc
   (only when USE_TCMALLOC is defined).

The rationale of this test is: if _heap_init was shimmed there are
very good chances that malloc (& friends) were shimmed as well.
Likewise on Linux for mallopt() <-> malloc().

BUG=564618
Committed: https://crrev.com/be9d6fce01061afde845fd7aa2335e5757d6a421
Cr-Commit-Position: refs/heads/master@{#369007}

[Primiano Tucci (use gerrit), 2016-01-11 16:38:08]

Description was changed from

==========
Add run-time check to shim layer

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

to

==========
Add run-time CHECK to detect the presence of the shim layer

As part of the refactoring work on the allocator, it emerged that it
would be nice to have a test that ensures that we don't accidentally
lose the shim layer along the way.
The shim layer overrides various symbols from libcmt, among which
_heap_init.
This CL adds a runtime CHECK to the common startup code (SetupCRT()
in base/win) that suicides chrome if it detects that chrome did NOT
call the shimmed _heap_init() method.

The rationale of this test is: if _heap_init was shimmed there are
very good chances that malloc (& friends) were shimmed as well.

BUG=564618
==========

[Primiano Tucci (use gerrit), 2016-01-11 16:38:08]

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

[Primiano Tucci (use gerrit), 2016-01-11 16:42:11]

Will, I tried to give a shoot at the test issue.
I first tried with dumpbin, but I found the following problems:
 - AFAICT dumpbin /SYMBOLS requires the .pdb, not just the dll. But GOMA builds
don't produce any pdb. And I'm pretty sure the release bots use goma, so this
would be hard to run on the bots.
 - Even if it worked, what should we test? in both cases (if the shim layer is
there or something went wrong) we would see a definition of malloc. In one case
it will come from libcmt, in the other from our shim layer. How would we
distinguish the two cases?

in this CL I tried to opt for a run-time check instead.
Conversely to what I was thinking, we don't need gross hacks. Turns out that we
are already shimming also _heap_init (which is not a fastpath, IIUC). So I'm
adding a check there, speculating that if _heap_init shimming worked, also
malloc is shimmed.

WDYT?

[Primiano Tucci (use gerrit), 2016-01-11 18:44:29]

Description was changed from

==========
Add run-time CHECK to detect the presence of the shim layer

As part of the refactoring work on the allocator, it emerged that it
would be nice to have a test that ensures that we don't accidentally
lose the shim layer along the way.
The shim layer overrides various symbols from libcmt, among which
_heap_init.
This CL adds a runtime CHECK to the common startup code (SetupCRT()
in base/win) that suicides chrome if it detects that chrome did NOT
call the shimmed _heap_init() method.

The rationale of this test is: if _heap_init was shimmed there are
very good chances that malloc (& friends) were shimmed as well.

BUG=564618
==========

to

==========
Add run-time CHECK to smoke-test allocator overrides

As part of the refactoring work on the allocator, it emerged that it
would be nice to have a test that ensures that we don't accidentally
break things along the way and not detected that.
This CL adds a runtime CHECK() that verifies that:
 - On Windows (non-component build) the shim layer has been 
   initialized. Concretely this checks that chrome called the shimmed
   _heap_init() method and not the one from libcmt.
 - On Linux desktop, the malloc symbols are being overridden by tcmalloc
   (only when USE_TCMALLOC is defined).

The rationale of this test is: if _heap_init was shimmed there are
very good chances that malloc (& friends) were shimmed as well.
Likewise on Linux for mallopt() <-> malloc().

BUG=564618
==========

[Primiano Tucci (use gerrit), 2016-01-11 18:45:11]

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

[Primiano Tucci (use gerrit), 2016-01-11 18:45:12]

following +Nico suggestion (in a previous offline chat), adding also a check to
make sure that on Linux (component and not) when we are supposed to use tcmalloc
we are actually using that.
The motivation is the same: preventing that the upcoming cleanup doesn't
accidentally (and silently) break things.

[Will Harris, 2016-01-11 22:51:31]

https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocato...
File base/allocator/allocator_check.cc (right):

https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocato...
base/allocator/allocator_check.cc:23: #if defined(OS_WIN) &&
!defined(COMPONENT_BUILD)
I think this needs a MEMORY_TOOL_REPLACES_ALLOCATOR or a ALLOCATOR_SHIM to cover
all the cases correctly...?

[Primiano Tucci (use gerrit), 2016-01-11 23:32:05]

https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocato...
File base/allocator/allocator_check.cc (right):

https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocato...
base/allocator/allocator_check.cc:23: #if defined(OS_WIN) &&
!defined(COMPONENT_BUILD)
On 2016/01/11 22:51:31, Will Harris (in DC) wrote:
> I think this needs a MEMORY_TOOL_REPLACES_ALLOCATOR or a ALLOCATOR_SHIM to
cover
> all the cases correctly...?

Ah right. good spot. I'll fix it tomorrow EMEA morning.
Does the rest look ok?

[Primiano Tucci (use gerrit), 2016-01-12 11:36:02]

https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocato...
File base/allocator/allocator_check.cc (right):

https://codereview.chromium.org/1577883002/diff/40001/base/allocator/allocato...
base/allocator/allocator_check.cc:23: #if defined(OS_WIN) &&
!defined(COMPONENT_BUILD)
On 2016/01/11 22:51:31, Will Harris (in DC) wrote:
> I think this needs a MEMORY_TOOL_REPLACES_ALLOCATOR or a ALLOCATOR_SHIM to
cover
> all the cases correctly...?

Ok fixed.  ALLOCATOR_SHIM seems the way to go. I checked and it is not defined
when asan=1 or syzyasan=1, which are the one that on Windows cause
MEMORY_TOOL_REPLACES_ALLOCATOR to be defined.

[Will Harris, 2016-01-12 15:08:38]

please can you add a test that calls base::allocator::IsAllocatorInitialized()
somewhere in base_unittests - we'd want this to break the tests somewhere really
obvious to help diagnose it.

[Primiano Tucci (use gerrit), 2016-01-12 16:06:36]

On 2016/01/12 15:08:38, Will Harris (in DC) wrote:
> please can you add a test that calls base::allocator::IsAllocatorInitialized()
> somewhere in base_unittests - we'd want this to break the tests somewhere
really
> obvious to help diagnose it.

I uploaded a new patchset to deal with mallopt after our offline conversation.
As regards the tests, as this is invoked in content_main_runner ALL
content_browsertests will break if this goes wrong (I just tried locally), like
this (below) that should be make it pretty easy to diagnose:

[39/1189] DumpAccessibilityTreeTest.AccessibilityAriaDefinition (1568 ms)
[ RUN      ] DumpAccessibilityTreeTest.AccessibilityAriaDescribedBy
[14447:14447:0112/160355:715129377070:FATAL:content_main_runner.cc(662)] Check
failed: base::allocator::IsAllocatorInitialized().
#0 0x000003ce4bfe base::debug::StackTrace::StackTrace()
#1 0x000003cf94db logging::LogMessage::~LogMessage()
#2 0x00000311175f content::ContentMainRunnerImpl::Initialize()
#3 0x0000031107da content::ContentMain()
#4 0x00000082ac96 content::BrowserTestBase::SetUp()
#5 0x00000080d583 content::ContentBrowserTest::SetUp()
...

[Will Harris, 2016-01-12 16:22:40]

On 2016/01/12 16:06:36, Primiano Tucci wrote:
> On 2016/01/12 15:08:38, Will Harris (in DC) wrote:
> > please can you add a test that calls
base::allocator::IsAllocatorInitialized()
> > somewhere in base_unittests - we'd want this to break the tests somewhere
> really
> > obvious to help diagnose it.
> 
> I uploaded a new patchset to deal with mallopt after our offline conversation.
> As regards the tests, as this is invoked in content_main_runner ALL
> content_browsertests will break if this goes wrong (I just tried locally),
like
> this (below) that should be make it pretty easy to diagnose:
> 
> [39/1189] DumpAccessibilityTreeTest.AccessibilityAriaDefinition (1568 ms)
> [ RUN      ] DumpAccessibilityTreeTest.AccessibilityAriaDescribedBy
> [14447:14447:0112/160355:715129377070:FATAL:content_main_runner.cc(662)] Check
> failed: base::allocator::IsAllocatorInitialized().
> #0 0x000003ce4bfe base::debug::StackTrace::StackTrace()
> #1 0x000003cf94db logging::LogMessage::~LogMessage()
> #2 0x00000311175f content::ContentMainRunnerImpl::Initialize()
> #3 0x0000031107da content::ContentMain()
> #4 0x00000082ac96 content::BrowserTestBase::SetUp()
> #5 0x00000080d583 content::ContentBrowserTest::SetUp()
> ...

okay that looks good. I was worried that crashes that early in startup can cause
no test results to be produced at all (the bot just turns purple), I have seen
instances of that, lgtm if you're sure that won't happen here for a failure.

[Primiano Tucci (use gerrit), 2016-01-12 18:44:25]

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

[Primiano Tucci (use gerrit), 2016-01-12 18:44:26]

+sievers for +2 lines in content/app/content_main_runner.cc
+thakis for base/ build file changes

[no sievers, 2016-01-12 18:47:27]

On 2016/01/12 18:44:26, Primiano Tucci wrote:
> +sievers for +2 lines in content/app/content_main_runner.cc

lgtm 

> +thakis for base/ build file changes

[Nico, 2016-01-12 20:50:27]

lgtm

[Primiano Tucci (use gerrit), 2016-01-12 20:51:15]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2016-01-12 20:52:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577883002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577883002/100001

[commit-bot: I haz the power, 2016-01-12 20:57:38]

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

[commit-bot: I haz the power, 2016-01-12 20:57:38]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Primiano Tucci (use gerrit), 2016-01-12 21:02:04]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-01-12 21:02:05]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
sievers@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1577883002/#ps120001
(title: "Rebase")

[Primiano Tucci (use gerrit), 2016-01-12 21:02:44]

The CQ bit was unchecked by primiano@chromium.org

[commit-bot: I haz the power, 2016-01-12 21:03:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577883002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577883002/120001

[Primiano Tucci (use gerrit), 2016-01-12 21:03:59]

Patchset #8 (id:130001) has been deleted

[Primiano Tucci (use gerrit), 2016-01-12 21:05:01]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-01-12 21:05:02]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
sievers@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/1577883002/#ps150001
(title: "AAAAArgh, inconsistent trailing dot in README.chromium")

[commit-bot: I haz the power, 2016-01-12 21:06:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577883002/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577883002/150001

[commit-bot: I haz the power, 2016-01-12 22:16:19]

Description was changed from

==========
Add run-time CHECK to smoke-test allocator overrides

As part of the refactoring work on the allocator, it emerged that it
would be nice to have a test that ensures that we don't accidentally
break things along the way and not detected that.
This CL adds a runtime CHECK() that verifies that:
 - On Windows (non-component build) the shim layer has been 
   initialized. Concretely this checks that chrome called the shimmed
   _heap_init() method and not the one from libcmt.
 - On Linux desktop, the malloc symbols are being overridden by tcmalloc
   (only when USE_TCMALLOC is defined).

The rationale of this test is: if _heap_init was shimmed there are
very good chances that malloc (& friends) were shimmed as well.
Likewise on Linux for mallopt() <-> malloc().

BUG=564618
==========

to

==========
Add run-time CHECK to smoke-test allocator overrides

As part of the refactoring work on the allocator, it emerged that it
would be nice to have a test that ensures that we don't accidentally
break things along the way and not detected that.
This CL adds a runtime CHECK() that verifies that:
 - On Windows (non-component build) the shim layer has been 
   initialized. Concretely this checks that chrome called the shimmed
   _heap_init() method and not the one from libcmt.
 - On Linux desktop, the malloc symbols are being overridden by tcmalloc
   (only when USE_TCMALLOC is defined).

The rationale of this test is: if _heap_init was shimmed there are
very good chances that malloc (& friends) were shimmed as well.
Likewise on Linux for mallopt() <-> malloc().

BUG=564618
==========

[commit-bot: I haz the power, 2016-01-12 22:16:19]

Committed patchset #8 (id:150001)

[commit-bot: I haz the power, 2016-01-12 22:18:15]

Description was changed from

==========
Add run-time CHECK to smoke-test allocator overrides

As part of the refactoring work on the allocator, it emerged that it
would be nice to have a test that ensures that we don't accidentally
break things along the way and not detected that.
This CL adds a runtime CHECK() that verifies that:
 - On Windows (non-component build) the shim layer has been 
   initialized. Concretely this checks that chrome called the shimmed
   _heap_init() method and not the one from libcmt.
 - On Linux desktop, the malloc symbols are being overridden by tcmalloc
   (only when USE_TCMALLOC is defined).

The rationale of this test is: if _heap_init was shimmed there are
very good chances that malloc (& friends) were shimmed as well.
Likewise on Linux for mallopt() <-> malloc().

BUG=564618
==========

to

==========
Add run-time CHECK to smoke-test allocator overrides

As part of the refactoring work on the allocator, it emerged that it
would be nice to have a test that ensures that we don't accidentally
break things along the way and not detected that.
This CL adds a runtime CHECK() that verifies that:
 - On Windows (non-component build) the shim layer has been
   initialized. Concretely this checks that chrome called the shimmed
   _heap_init() method and not the one from libcmt.
 - On Linux desktop, the malloc symbols are being overridden by tcmalloc
   (only when USE_TCMALLOC is defined).

The rationale of this test is: if _heap_init was shimmed there are
very good chances that malloc (& friends) were shimmed as well.
Likewise on Linux for mallopt() <-> malloc().

BUG=564618

Committed: https://crrev.com/be9d6fce01061afde845fd7aa2335e5757d6a421
Cr-Commit-Position: refs/heads/master@{#369007}
==========

[commit-bot: I haz the power, 2016-01-12 22:18:16]

Patchset 8 (id:??) landed as
https://crrev.com/be9d6fce01061afde845fd7aa2335e5757d6a421
Cr-Commit-Position: refs/heads/master@{#369007}