Enable handle verifier for tests and add some tests.

Enable handle verifier for tests and add some tests.

Disable handle verifier hooks if running under drmemory.

BUG=571304
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_chromium_dbg_ng

Committed: https://crrev.com/5556e56290c41891b0830af9b6b9153b745dd226
Cr-Commit-Position: refs/heads/master@{#372877}

[Will Harris, 2016-01-18 23:29:16]

Description was changed from

==========
Enable handle verifier for tests and add some tests.

BUG=571304
==========

to

==========
Enable handle verifier for tests and add some tests.

BUG=571304
==========

[Will Harris, 2016-01-18 23:29:16]

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

[Will Harris, 2016-01-18 23:29:24]

wfh@chromium.org changed reviewers:
+ cpu@chromium.org
- jam@chromium.org

[Will Harris, 2016-01-18 23:31:07]

PTAL

[cpu_(ooo_6.6-7.5), 2016-01-19 22:53:49]

https://codereview.chromium.org/1580873003/diff/80001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1580873003/diff/80001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:204: channel ==
version_info::Channel::UNKNOWN) {
do we need this for the tests?

[Will Harris, 2016-01-19 23:06:34]

https://codereview.chromium.org/1580873003/diff/80001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1580873003/diff/80001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:204: channel ==
version_info::Channel::UNKNOWN) {
On 2016/01/19 22:53:49, cpu wrote:
> do we need this for the tests?

AFAIK the bots run on UNKNOWN channel. This is what you get back if you have no
Omaha installed... I can double check this.

[Will Harris, 2016-01-20 06:53:59]

On 2016/01/19 23:06:34, Will Harris wrote:
>
https://codereview.chromium.org/1580873003/diff/80001/chrome/app/chrome_main_...
> File chrome/app/chrome_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/1580873003/diff/80001/chrome/app/chrome_main_...
> chrome/app/chrome_main_delegate.cc:204: channel ==
> version_info::Channel::UNKNOWN) {
> On 2016/01/19 22:53:49, cpu wrote:
> > do we need this for the tests?
> 
> AFAIK the bots run on UNKNOWN channel. This is what you get back if you have
no
> Omaha installed... I can double check this.

I verified all the bots were running on UNKNOWN channel, as expected - see
https://codereview.chromium.org/1608133002/ so enabling handle verifier on
UNKNOWN seems like the right thing to do to ensure full bot coverage.

[Will Harris, 2016-01-21 18:04:17]

ping cpu

[cpu_(ooo_6.6-7.5), 2016-01-21 19:12:48]

I guess my question was more about having this code when the rest of the CL is
about unit testing. Does unit testing rely on the channel?

[Will Harris, 2016-01-21 19:25:13]

On 2016/01/21 19:12:48, cpu wrote:
> I guess my question was more about having this code when the rest of the CL is
> about unit testing. Does unit testing rely on the channel?

Hmm - my understanding was that the ChromeMainDelegate::BasicStartupComplete
code in chrome/app/chrome_main_delegate.cc is called from tests like
content_browsertests, unit_tests, browser_tests which don't use TestSuite
directly, so was needed to get full coverage.

[Will Harris, 2016-01-22 01:38:09]

On 2016/01/21 19:25:13, Will Harris wrote:
> On 2016/01/21 19:12:48, cpu wrote:
> > I guess my question was more about having this code when the rest of the CL
is
> > about unit testing. Does unit testing rely on the channel?
> 
> Hmm - my understanding was that the ChromeMainDelegate::BasicStartupComplete
> code in chrome/app/chrome_main_delegate.cc is called from tests like
> content_browsertests, unit_tests, browser_tests which don't use TestSuite
> directly, so was needed to get full coverage.

my understanding is wrong, all the tests including those above go through the
TestSuite codepath so handle verifier is always enabled in tests - no need to
also enable for the UNKNOWN channel, so I took that out. Thanks. PTAL.

[cpu_(ooo_6.6-7.5), 2016-01-22 04:28:12]

lgtm

[Will Harris, 2016-01-22 04:36:10]

On 2016/01/22 04:28:12, cpu wrote:
> lgtm

Wow the bots turned super red...

[cpu_(ooo_6.6-7.5), 2016-01-22 17:18:46]

Cool. Those are probably bugs right there.

[Will Harris, 2016-01-22 18:30:39]

also I had a dream last night about a different way of coping on windows 7
rather than just disabling the tests, so I'm going to try that, if I change the
code I'll send for a re-review.

https://codereview.chromium.org/1580873003/diff/100001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/1580873003/diff/100001/base/test/test_suite.c...
base/test/test_suite.cc:323: __debugbreak();
would help if I removed my debugging __debugbreak()

[Will Harris, 2016-01-22 21:58:23]

I can't even repro the debug failures on win7 now, but I know they happen on the
bots, so it must be very specific to some kind of configuration, so I couldn't
test my plan. I don't think it would have worked though since the handle
verifier was tripping in the test process and not the death process. I will
commit this now.

https://codereview.chromium.org/1580873003/diff/100001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/1580873003/diff/100001/base/test/test_suite.c...
base/test/test_suite.cc:323: __debugbreak();
On 2016/01/22 18:30:39, Will Harris wrote:
> would help if I removed my debugging __debugbreak()

Done.

[Will Harris, 2016-01-22 21:59:45]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2016-01-22 21:59:45]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/1580873003/#ps120001
(title: "remove debugbreak")

[commit-bot: I haz the power, 2016-01-22 22:00:46]

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

[commit-bot: I haz the power, 2016-01-22 23:28:26]

Description was changed from

==========
Enable handle verifier for tests and add some tests.

BUG=571304
==========

to

==========
Enable handle verifier for tests and add some tests.

BUG=571304
==========

[commit-bot: I haz the power, 2016-01-22 23:28:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-22 23:30:09]

Description was changed from

==========
Enable handle verifier for tests and add some tests.

BUG=571304
==========

to

==========
Enable handle verifier for tests and add some tests.

BUG=571304

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

[commit-bot: I haz the power, 2016-01-22 23:30:09]

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

[oshima, 2016-01-25 20:47:44]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1633623003/ by oshima@chromium.org.

The reason for reverting is: A lot of test are crashing on DrMemory x64 bot, so
I'm reverting this speculatively. I'll reland this if it didn't help..

[Will Harris, 2016-01-31 21:42:16]

Description was changed from

==========
Enable handle verifier for tests and add some tests.

BUG=571304

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

to

==========
Enable handle verifier for tests and add some tests.

BUG=571304

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

[Will Harris, 2016-02-01 00:57:26]

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

[Will Harris, 2016-02-01 00:58:04]

+scottmg for gyp

I'm looking for a review of diff between PS8 and PS9, which disables handle
hooks if drmemory is specified.

Please say if you prefer me to upload into a new CL instead.

Thanks.

[Will Harris, 2016-02-01 01:08:51]

Patchset #9 (id:160001) has been deleted

[scottmg, 2016-02-01 16:45:31]

https://codereview.chromium.org/1580873003/diff/180001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/1580873003/diff/180001/base/test/test_suite.c...
base/test/test_suite.cc:323: #if defined(_DEBUG)
It's sort of weird that sometimes it's disabled here, and sometimes in the body
of the function.

[Will Harris, 2016-02-01 19:16:13]

PTAL

https://codereview.chromium.org/1580873003/diff/180001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/1580873003/diff/180001/base/test/test_suite.c...
base/test/test_suite.cc:323: #if defined(_DEBUG)
On 2016/02/01 16:45:31, scottmg wrote:
> It's sort of weird that sometimes it's disabled here, and sometimes in the
body
> of the function.

Done.

Another option is expose something like base::debug::AreHandleHooksInPlace() but
since this is only going to be used in very few places I'm not sure it's worth
it.

[scottmg, 2016-02-01 19:26:27]

lgtm

Consider switching to
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5c4ySpPsV14/dis...
though (or as a follow up) or Brett might yell.

[Will Harris, 2016-02-01 19:29:31]

On 2016/02/01 19:26:27, scottmg wrote:
> lgtm
> 
> Consider switching to
>
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/5c4ySpPsV14/dis...
> though (or as a follow up) or Brett might yell.

I actually DID recall some sort of discussion on a change to build flags but
went to the GN docs trying to find them and couldn't, so stuck with this...  I
should have looked closer at chromium-dev.

I reached a dead end at
https://www.chromium.org/developers/gn-build-configuration

I have taken considerable care to only apply the #defines to the specific
components that need them, so changing e.g. win_disable_handle_verifier_hooks or
the GYP flag only recompiles base and base tests.

[Will Harris, 2016-02-01 19:43:21]

Description was changed from

==========
Enable handle verifier for tests and add some tests.

BUG=571304

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

to

==========
Enable handle verifier for tests and add some tests.

BUG=571304
CQ_INCLUDE_TRYBOTS=win_chromium_dbg_ng
==========

[Will Harris, 2016-02-01 19:43:28]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2016-02-01 19:43:28]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org
Link to the patchset: https://codereview.chromium.org/1580873003/#ps200001
(title: "code review, move code around")

[commit-bot: I haz the power, 2016-02-01 19:45:44]

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

[commit-bot: I haz the power, 2016-02-01 19:45:45]

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

[commit-bot: I haz the power, 2016-02-01 19:45:46]

Transient error: Invalid delimiter in "win_chromium_dbg_ng":
Correct syntax is "tryserver:bot1,bot2;tryserver2:bot3,bot4;".

[Will Harris, 2016-02-01 19:47:06]

Description was changed from

==========
Enable handle verifier for tests and add some tests.

BUG=571304
CQ_INCLUDE_TRYBOTS=win_chromium_dbg_ng
==========

to

==========
Enable handle verifier for tests and add some tests.

BUG=571304
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_chromium_dbg_ng
==========

[Will Harris, 2016-02-01 19:48:47]

The CQ bit was checked by wfh@chromium.org

[commit-bot: I haz the power, 2016-02-01 19:50:58]

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

[commit-bot: I haz the power, 2016-02-01 19:58:25]

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

[commit-bot: I haz the power, 2016-02-01 19:58:26]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Will Harris, 2016-02-02 00:01:56]

Description was changed from

==========
Enable handle verifier for tests and add some tests.

BUG=571304
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_chromium_dbg_ng
==========

to

==========
Enable handle verifier for tests and add some tests.

Disable handle verifier hooks if running under drmemory.

BUG=571304
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_chromium_dbg_ng
==========

[Will Harris, 2016-02-02 00:02:14]

The CQ bit was checked by wfh@chromium.org

[Will Harris, 2016-02-02 00:02:14]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1580873003/#ps220001
(title: "do not copy variable up scope")

[commit-bot: I haz the power, 2016-02-02 00:04:00]

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

[commit-bot: I haz the power, 2016-02-02 03:10:14]

Description was changed from

==========
Enable handle verifier for tests and add some tests.

Disable handle verifier hooks if running under drmemory.

BUG=571304
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_chromium_dbg_ng
==========

to

==========
Enable handle verifier for tests and add some tests.

Disable handle verifier hooks if running under drmemory.

BUG=571304
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_chromium_dbg_ng
==========

[commit-bot: I haz the power, 2016-02-02 03:10:15]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-02-02 03:13:20]

Description was changed from

==========
Enable handle verifier for tests and add some tests.

Disable handle verifier hooks if running under drmemory.

BUG=571304
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_chromium_dbg_ng
==========

to

==========
Enable handle verifier for tests and add some tests.

Disable handle verifier hooks if running under drmemory.

BUG=571304
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_chromium_dbg_ng

Committed: https://crrev.com/5556e56290c41891b0830af9b6b9153b745dd226
Cr-Commit-Position: refs/heads/master@{#372877}
==========

[commit-bot: I haz the power, 2016-02-02 03:13:21]

Patchset 11 (id:??) landed as
https://crrev.com/5556e56290c41891b0830af9b6b9153b745dd226
Cr-Commit-Position: refs/heads/master@{#372877}

[Will Harris, 2016-02-05 23:34:04]

A revert of this CL (patchset #11 id:220001) has been created in
https://codereview.chromium.org/1678553002/ by wfh@chromium.org.

The reason for reverting is: This breaks on win7 component builds, and also
win10..