Patch to try dump-on-DCHECK.

Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the process, to only uploading a crash dump, and only on the first failed DCHECK in each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows official builds.

Note that this CL is intended to be landed, a branch cut to release from, and then immediately reverted; it is not intended to be landed in Chromium for any longer period.

BUG=596231
Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}

Committed: https://crrev.com/b80aa8f5c269c9ee4f3a00b03840ea3df68f77d1
Cr-Commit-Position: refs/heads/master@{#385413}

[Wez, 2016-03-20 22:13:49]

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

[Wez, 2016-03-20 22:13:50]

tl;dr: We'd like to land this for a single Canary branch sometime this week,
then revert.

CC'd you on mail thread w/ more context.

[Nico, 2016-03-20 23:10:40]

mention the "land and revert soon after" bit in the cl description please

https://codereview.chromium.org/1814423002/diff/60001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/1814423002/diff/60001/base/logging.cc#newcode346
base/logging.cc:346: static bool dump_on_dcheck = true;
this is racy (doubt it matters though)

https://codereview.chromium.org/1814423002/diff/60001/base/logging.cc#newcode348
base/logging.cc:348: dump_on_dcheck = !base::debug::DumpWithoutCrashing();
for stacks in renderer processes, this will dump once per renderer process, not
once total. is that what you want?

https://codereview.chromium.org/1814423002/diff/60001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1814423002/diff/60001/build/config/BUILD.gn#n...
build/config/BUILD.gn:91: # In Windows official builds, enable DCHECKs to
dump-without-crashing.
windows official builds are still built with gyp, so since you're not modifying
any gyp files this patch has no effect on real official binaries atm

[scottmg, 2016-03-21 17:28:11]

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

[scottmg, 2016-03-21 17:28:12]

https://codereview.chromium.org/1814423002/diff/60001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/1814423002/diff/60001/base/logging.cc#newcode64
base/logging.cc:64: #if defined(OFFICIAL_BUILD)
Remove the #if here.

https://codereview.chromium.org/1814423002/diff/60001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1814423002/diff/60001/base/logging.h#newcode685
base/logging.h:685: #define DCHECK(condition)  (condition) ? (void) 0 :
logging::DCheckDumpWithoutCrashing(), EAT_STREAM_PARAMETERS
extra spaces and 80col

https://codereview.chromium.org/1814423002/diff/60001/base/logging.h#newcode710
base/logging.h:710: #define DCHECK_OP(name, op, val1, val2)
DCHECK(logging::Check##name##Impl((val1), (val2), #val1 " " #op " " #val2))
80col

[jam, 2016-03-21 17:40:03]

On 2016/03/20 22:13:50, Wez wrote:
> tl;dr: We'd like to land this for a single Canary branch sometime this week,
> then revert.
> 
> CC'd you on mail thread w/ more context.

can you share the mail thread? it's really not clear to me that we want to do
this.

what's the specific motivation?

[jam, 2016-03-21 19:47:13]

On 2016/03/21 17:40:03, jam wrote:
> On 2016/03/20 22:13:50, Wez wrote:
> > tl;dr: We'd like to land this for a single Canary branch sometime this week,
> > then revert.
> > 
> > CC'd you on mail thread w/ more context.
> 
> can you share the mail thread? it's really not clear to me that we want to do
> this.
> 
> what's the specific motivation?

oh i didn't realize this won't crash, but just take a dump. ignore my comment
then.

[Wez, 2016-03-23 04:18:13]

PTAL

https://codereview.chromium.org/1814423002/diff/60001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/1814423002/diff/60001/base/logging.cc#newcode64
base/logging.cc:64: #if defined(OFFICIAL_BUILD)
On 2016/03/21 17:28:12, scottmg wrote:
> Remove the #if here.

You prefer to include even if it won't be used?

Done.

https://codereview.chromium.org/1814423002/diff/60001/base/logging.cc#newcode346
base/logging.cc:346: static bool dump_on_dcheck = true;
On 2016/03/20 23:10:40, Nico wrote:
> this is racy (doubt it matters though)

Extended comment to clarify that yes, that's understood & acceptable.

https://codereview.chromium.org/1814423002/diff/60001/base/logging.cc#newcode348
base/logging.cc:348: dump_on_dcheck = !base::debug::DumpWithoutCrashing();
On 2016/03/20 23:10:40, Nico wrote:
> for stacks in renderer processes, this will dump once per renderer process,
not
> once total. is that what you want?

Yes. Extended comment should make that clearer.

https://codereview.chromium.org/1814423002/diff/60001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1814423002/diff/60001/base/logging.h#newcode685
base/logging.h:685: #define DCHECK(condition)  (condition) ? (void) 0 :
logging::DCheckDumpWithoutCrashing(), EAT_STREAM_PARAMETERS
On 2016/03/21 17:28:12, scottmg wrote:
> extra spaces and 80col

Done.

https://codereview.chromium.org/1814423002/diff/60001/base/logging.h#newcode710
base/logging.h:710: #define DCHECK_OP(name, op, val1, val2)
DCHECK(logging::Check##name##Impl((val1), (val2), #val1 " " #op " " #val2))
On 2016/03/21 17:28:12, scottmg wrote:
> 80col

Done.

(git cl format does macros - awesome!)

https://codereview.chromium.org/1814423002/diff/60001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1814423002/diff/60001/build/config/BUILD.gn#n...
build/config/BUILD.gn:91: # In Windows official builds, enable DCHECKs to
dump-without-crashing.
On 2016/03/20 23:10:40, Nico wrote:
> windows official builds are still built with gyp, so since you're not
modifying
> any gyp files this patch has no effect on real official binaries atm

Acknowledged.

Added some ugly GYP hackery.

[Wez, 2016-03-23 17:18:52]

wez@chromium.org changed reviewers:
+ jam@chromium.org
- scottmg@chromium.org, thakis@chromium.org

[Wez, 2016-03-23 17:18:53]

John, could you review, since Nico is OOO?

[jam, 2016-03-23 20:02:08]

On 2016/03/23 17:18:53, Wez wrote:
> John, could you review, since Nico is OOO?

this really should be reviewed by an owner in base/

[Wez, 2016-03-23 20:40:53]

wez@chromium.org changed reviewers:
+ danakj@chromium.org
- jam@chromium.org

[Wez, 2016-03-23 20:40:54]

Good point!

Dana, can you do the honours?

[Wez, 2016-03-23 20:41:47]

Description was changed from

==========
Patch to try dump-on-DCHECK.

Enables dump-without-crashing in response to the first failed DCHECK in
each Crashpad-enabled process, in official Windows builds.

BUG=596231
==========

to

==========
Patch to try dump-on-DCHECK.

Enables dump-without-crashing in response to the first failed DCHECK in
each Crashpad-enabled process, in official Windows builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231
==========

[Wez, 2016-03-23 22:52:15]

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

[Wez, 2016-03-23 22:52:16]

danakj: Please review base/
scottmg: Please review build/

Thanks!

[scottmg, 2016-03-23 23:20:41]

build/ lgtm

[danakj, 2016-03-24 00:10:34]

Why aren't you just changing here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.cc&l=735
?

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode669
base/logging.h:669: #define DCHECK(condition)                                   
           \
But in this case it won't log?

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode686
base/logging.h:686: (condition) ? (void)0 :
logging::DCheckDumpWithoutCrashing(), \
No DCHECK_IS_ON() check here? This will make DCHECK always run.

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode713
base/logging.h:713: DCHECK(logging::Check##name##Impl((val1), (val2), #val1 " "
#op " " #val2))
No DCHECK_IS_ON() check here?

[Wez, 2016-03-24 00:33:57]

On 2016/03/24 00:10:34, danakj wrote:
> Why aren't you just changing here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.cc&l=735
> ?

That would have been simpler, but would have meant that:
1. CHECKs would no longer crash, which does not seem acceptable in released
builds.
2. DCHECK-logging appends (via operator<<) would still be compiled in.
3. DCHECK would still do actual logging.

AFAIK 2 & 3 have potentially significant performance impact over and above the
conditional test.

I considered adding a new severity level but that wouldn't help with the
performance issues.

> https://codereview.chromium.org/1814423002/diff/180001/base/logging.h
> File base/logging.h (right):
> 
>
https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode669
> base/logging.h:669: #define DCHECK(condition)                                 
 
>            \
> But in this case it won't log?

Correct; this patch switches DCHECK_ALWAYS_ON but then also flips DCHECK's
behaviour from LOG+dump+crash to just dump.

>
https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode686
> base/logging.h:686: (condition) ? (void)0 :
> logging::DCheckDumpWithoutCrashing(), \
> No DCHECK_IS_ON() check here? This will make DCHECK always run.

Yes; this patch forces DCHECK_ALWAYS_ON, so DCHECK_IS_ON() is always true.

>
https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode713
> base/logging.h:713: DCHECK(logging::Check##name##Impl((val1), (val2), #val1 "
"
> #op " " #val2))
> No DCHECK_IS_ON() check here?

See above.

[Wez, 2016-03-24 00:36:59]

Description was changed from

==========
Patch to try dump-on-DCHECK.

Enables dump-without-crashing in response to the first failed DCHECK in
each Crashpad-enabled process, in official Windows builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231
==========

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231
==========

[Wez, 2016-03-24 00:53:49]

Balh; looks like there are a variety of death-tests that rely on DCHECK & pals
actually crashing the binary ;-(

[danakj, 2016-03-24 21:29:41]

I don't like tying this flag to DCHECK_ALWAYS_ON, I would make it a separate GN
flag and #define.

[Wez, 2016-03-25 00:10:26]

On 2016/03/24 21:29:41, danakj wrote:
> I don't like tying this flag to DCHECK_ALWAYS_ON, I would make it a separate
GN
> flag and #define.

That will required trawling through the codebase for the locations where checks
are compiled in dependent on DCHECK_ALWAYS_ON, and adding the new flag to those
cases, which seemed fragile for a land & revert patch.

How would you feel about a separate patch that migrates those checks to use
DCHECK_ALWAYS_ON(), and introduces a DCHECK_IS_CHECK() macro that the relevant
death-tests can use, then rebasing this patch on that?

[Wez, 2016-03-25 00:11:36]

Meaning that, with those patches in place, this one can set
DCHECK_IS_CHECK()=false, DCHECK_IS_ON()=true and thereby avoid patching any
tests, etc

[danakj, 2016-03-25 00:20:11]

On Thu, Mar 24, 2016 at 5:10 PM, <wez@chromium.org> wrote:

> On 2016/03/24 21:29:41, danakj wrote:
> > I don't like tying this flag to DCHECK_ALWAYS_ON, I would make it a
> separate
> GN
> > flag and #define.
>
> That will required trawling through the codebase for the locations where
> checks
> are compiled in dependent on DCHECK_ALWAYS_ON, and adding the new flag to
> those
> cases, which seemed fragile for a land & revert patch.
>

Er, why? You would presumably build your for-public-consumption binary with
DHECK_ALWAYS_ON, no? I am missing something.

1. You want DCHECKs to be enabled in release build: We have
DCHECK_ALWAYS_ON to do that already.

2. You want DCHECKs to log and not crash the first time: Add a new GN
setting/define for that.

There were some nice proposals for getting DCHECKs on users machines
without requiring us to disable DCHECKs sort-of but not quite. Like
enabling them for 1/100 browser runs or something. What about stuff like
that?


> How would you feel about a separate patch that migrates those checks to use
> DCHECK_ALWAYS_ON(), and introduces a DCHECK_IS_CHECK() macro that the
> relevant
> death-tests can use, then rebasing this patch on that?
>

-- 
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.

[Wez, 2016-03-25 01:03:49]

As discussed off-line (sorry, only just saw this response):

There are a number of code paths that check #if defined(NDEBUG) ||
defined(DCHECK_ALWAYS_ON) - I'll send a separate patch that cleans those up
to use DCHECK_IS_ON().

There are death-tests conditional on DCHECK_IS_ON() and which expect that
if DCHECK is on then it'll crash, except it won't... - since we are
landing/branching/reverting, and want to avoid this patch getting bloaty,
we can basically just ignore those failures on the official builders.

The latest patch does add a new GYP/GN setting
dcheck_is_dump_without_crash, but it also hard-wires them based on buld
type & OS, since we don't want to modify the builder configuration just for
this patch.

The patch has no effect on non-debug CHECKs etc; so besides the overhead of
debug-only fields, code & conditionals, these builds should be functionally
equivalent to our normal builds.

If we decide to re-run the experiment in future we can revisit whether/how
to make this a flippable switch; for a land&revert I think this is the
cleanest approach.

On 24 March 2016 at 17:19, Dana Jansens <danakj@chromium.org> wrote:

> On Thu, Mar 24, 2016 at 5:10 PM, <wez@chromium.org> wrote:
>
>> On 2016/03/24 21:29:41, danakj wrote:
>> > I don't like tying this flag to DCHECK_ALWAYS_ON, I would make it a
>> separate
>> GN
>> > flag and #define.
>>
>> That will required trawling through the codebase for the locations where
>> checks
>> are compiled in dependent on DCHECK_ALWAYS_ON, and adding the new flag to
>> those
>> cases, which seemed fragile for a land & revert patch.
>>
>
> Er, why? You would presumably build your for-public-consumption binary
> with DHECK_ALWAYS_ON, no? I am missing something.
>
> 1. You want DCHECKs to be enabled in release build: We have
> DCHECK_ALWAYS_ON to do that already.
>
> 2. You want DCHECKs to log and not crash the first time: Add a new GN
> setting/define for that.
>
> There were some nice proposals for getting DCHECKs on users machines
> without requiring us to disable DCHECKs sort-of but not quite. Like
> enabling them for 1/100 browser runs or something. What about stuff like
> that?
>
>
>> How would you feel about a separate patch that migrates those checks to
>> use
>> DCHECK_ALWAYS_ON(), and introduces a DCHECK_IS_CHECK() macro that the
>> relevant
>> death-tests can use, then rebasing this patch on that?
>>
>
>
>

-- 
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.

[danakj, 2016-03-25 23:21:33]

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode666
base/logging.h:666: #if defined(_PREFAST_) && defined(OS_WIN)
Seems like dumping should win here.

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode686
base/logging.h:686: (condition) ? (void)0 :
logging::DCheckDumpWithoutCrashing(), \
On 2016/03/24 00:10:34, danakj wrote:
> No DCHECK_IS_ON() check here? This will make DCHECK always run.

I still think this should be

DCHECK_IS_ON() && (condition) ? (void)0 : logging::DCheckDump

We talked about this a bunch yesterday, and I thought we agreed that this
shouldn't change the enabled-ness of DCHECK(). Am I misremembering?

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode711
base/logging.h:711: // Use logging::Check*Impl() to ensure that operator<<()s
don't go unused.
Didn't you use DCHECK() to ensure that rather? DCHECK has the
EAT_STREAM_PARAMETERS.

You used Check*Impl because that's how you do DCHECK_OP, to compare the two
inputs, no?

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode732
base/logging.h:732: #endif
comment what you're ending

[Wez, 2016-03-25 23:57:30]

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode666
base/logging.h:666: #if defined(_PREFAST_) && defined(OS_WIN)
On 2016/03/25 23:21:32, danakj wrote:
> Seems like dumping should win here.

I've added an #error to break things explicitly if both flags are set, since we
don't expect that will happen; better than going ahead and compiling and having
weird Prefast output.

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode669
base/logging.h:669: #define DCHECK(condition)                                   
           \
On 2016/03/24 00:10:34, danakj wrote:
> But in this case it won't log?

Not sure what you mean, but PREFAST + DCHECK_IS_DUMP is now officially an
unsupported config!

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode686
base/logging.h:686: (condition) ? (void)0 :
logging::DCheckDumpWithoutCrashing(), \
On 2016/03/25 23:21:32, danakj wrote:
> On 2016/03/24 00:10:34, danakj wrote:
> > No DCHECK_IS_ON() check here? This will make DCHECK always run.
> 
> I still think this should be
> 
> DCHECK_IS_ON() && (condition) ? (void)0 : logging::DCheckDump
> 
> We talked about this a bunch yesterday, and I thought we agreed that this
> shouldn't change the enabled-ness of DCHECK(). Am I misremembering? 

The patch forces DCHECK on, so the effect is the same, but I've added a
DCHECK_IS_ON condition to the #elif, so that in the off case we compile the
normal way, rather than risk DCHECK_IS_DUMP changing the behaviour of the "off"
case in some way.

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode711
base/logging.h:711: // Use logging::Check*Impl() to ensure that operator<<()s
don't go unused.
On 2016/03/25 23:21:32, danakj wrote:
> Didn't you use DCHECK() to ensure that rather? DCHECK has the
> EAT_STREAM_PARAMETERS.
> 
> You used Check*Impl because that's how you do DCHECK_OP, to compare the two
> inputs, no?

No; you could implement this as DCHECK((val1) op (val2)) but I found that that
in one case the operator<<() for a particular DCHECK() << value logging case
went unused - I think DCHECK guarantees to evaluate (condition) even if it is
off, but not to evaluate the trailing logging, necessarily.

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode713
base/logging.h:713: DCHECK(logging::Check##name##Impl((val1), (val2), #val1 " "
#op " " #val2))
On 2016/03/24 00:10:34, danakj wrote:
> No DCHECK_IS_ON() check here?

Added a #if block check instead.

https://codereview.chromium.org/1814423002/diff/180001/base/logging.h#newcode732
base/logging.h:732: #endif
On 2016/03/25 23:21:32, danakj wrote:
> comment what you're ending

Done.

[danakj, 2016-03-26 00:28:01]

OK LGTM let's see what data we can collect.

[Wez, 2016-03-29 20:22:32]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-03-29 20:22:33]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1814423002/#ps200001
(title: "Address comments")

[commit-bot: I haz the power, 2016-03-29 20:23:11]

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

[commit-bot: I haz the power, 2016-03-29 23:59:25]

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

[commit-bot: I haz the power, 2016-03-29 23:59:26]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Wez, 2016-03-29 23:59:44]

The CQ bit was checked by wez@chromium.org

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

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

[commit-bot: I haz the power, 2016-03-30 01:26:07]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231
==========

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231
==========

[commit-bot: I haz the power, 2016-03-30 01:26:08]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-30 01:28:31]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231
==========

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}
==========

[commit-bot: I haz the power, 2016-03-30 01:28:32]

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

[shinyak, 2016-03-30 04:15:40]

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

The reason for reverting is: This might be breaking official build. .

[Wez, 2016-03-30 04:38:14]

"This might be breaking official build" is not a sufficient rationale for
revert - please provide at least a link to the build break so that the CL
author (me) knows what broke. :P
On Mar 29, 2016 9:15 PM, "shinyak@chromium.org via codereview.chromium.org"
<reply@chromiumcodereview-hr.appspotmail.com> wrote:

> A revert of this CL (patchset #11 id:200001) has been created in
> https://codereview.chromium.org/1842563008/ by shinyak@chromium.org.
>
> The reason for reverting is: This might be breaking official build. .
>
> https://codereview.chromium.org/1814423002/
>

-- 
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.

[Wez, 2016-03-30 05:01:04]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}
==========

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}
==========

[shinyak, 2016-03-30 05:05:04]

Sorry that insufficient information.

Tracking bug is here
https://bugs.chromium.org/p/chromium/issues/detail?id=598955

https://build.chromium.org/p/chromium.perf/builders/Win%20Builder/
https://build.chromium.org/p/chromium.perf/builders/Win%20x64%20Builder/
https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win

[Wez, 2016-03-30 05:16:23]

Thanks!

On 29 March 2016 at 22:05, shinyak@chromium.org via codereview.chromium.org
<reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Sorry that insufficient information.
>
> Tracking bug is here
> https://bugs.chromium.org/p/chromium/issues/detail?id=598955
>
> https://build.chromium.org/p/chromium.perf/builders/Win%20Builder/
> https://build.chromium.org/p/chromium.perf/builders/Win%20x64%20Builder/
> https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win
>
>
>
> https://codereview.chromium.org/1814423002/
>

-- 
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.

[Wez, 2016-03-30 17:00:00]

brucedawson@ believes the failure was a toolchain flake, and I cannot repro
issue locally, so going to re-land and watch bots & clobber if it fails again.

[Wez, 2016-03-30 17:00:07]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-03-30 17:00:30]

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

[commit-bot: I haz the power, 2016-03-30 17:06:01]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}
==========

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}
==========

[commit-bot: I haz the power, 2016-03-30 17:06:03]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-30 17:07:34]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}
==========

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

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

[commit-bot: I haz the power, 2016-03-30 17:07:36]

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

[Nico, 2016-03-30 19:34:21]

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

The reason for reverting is: Speculative, looks like this breaks Google Chrome
Win:
https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/b...

obj\third_party\WebKit\Source\core\webcore_dom.lib : fatal error LNK1107:
invalid or corrupt file: cannot read at 0x62BBDB03

(To repro, you probably need to do an official build to trigger PGO).

[Wez, 2016-03-30 19:55:48]

Please clobber rather than reverting - brucedawson@ believes that this is a
toolchain flake.

On 30 March 2016 at 12:34, <thakis@chromium.org> wrote:

> A revert of this CL (patchset #11 id:200001) has been created in
> https://codereview.chromium.org/1850473002/ by thakis@chromium.org.
>
> The reason for reverting is: Speculative, looks like this breaks Google
> Chrome
> Win:
>
>
https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/b...
>
> obj\third_party\WebKit\Source\core\webcore_dom.lib : fatal error LNK1107:
> invalid or corrupt file: cannot read at 0x62BBDB03
>
> (To repro, you probably need to do an official build to trigger PGO).
>
> https://codereview.chromium.org/1814423002/
>

-- 
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.

[Wez, 2016-03-30 20:00:33]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

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

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

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

[Wez, 2016-03-31 01:11:14]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-03-31 01:11:39]

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

[Wez, 2016-03-31 01:12:12]

Bruce has landed https://codereview.chromium.org/1842203003/ to shard
webcore_dom.lib, which was close to 2GB in size before this patch -> relanding!

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

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

[commit-bot: I haz the power, 2016-03-31 01:33:24]

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

[Wez, 2016-03-31 01:40:38]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-03-31 01:40:56]

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

[commit-bot: I haz the power, 2016-03-31 02:50:19]

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

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

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

[Wez, 2016-03-31 06:18:02]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-03-31 06:18:42]

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

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

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

[commit-bot: I haz the power, 2016-03-31 07:13:22]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Wez, 2016-03-31 16:14:45]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-03-31 16:15:05]

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

[Wez, 2016-03-31 16:36:42]

The CQ bit was unchecked by wez@chromium.org

[Wez, 2016-03-31 17:19:09]

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

[Wez, 2016-03-31 17:19:10]

+thakis for third_party/WebKit/Source/wtf/OWNERS

[Wez, 2016-03-31 17:19:33]

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

[commit-bot: I haz the power, 2016-03-31 17:19:53]

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

[hans, 2016-03-31 17:35:39]

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

[hans, 2016-03-31 17:35:40]

Have you looked at the impact this will have on binary size?

I almost fell off my chair seeing the spike this caused on the 32-bit Clang
Windows size chart :-)

chrome_child.dll grew 16 MB, or 32.7%.

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

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wez, 2016-03-31 21:15:28]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-03-31 21:15:28]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1814423002/#ps220001
(title: "Disable Blink ASSERT in dump-without-DCHECK builds")

[Wez, 2016-03-31 21:15:30]

The CQ bit was unchecked by wez@chromium.org

[Wez, 2016-03-31 21:23:27]

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

[commit-bot: I haz the power, 2016-03-31 21:24:16]

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

[Nico, 2016-03-31 22:11:18]

lgtm

https://codereview.chromium.org/1814423002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1814423002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/Assertions.h:55: /* Enable ASSERT* macros if
DCHECK is on, unless it dumps-without-crashing. */
nit: use // comments
also, don't repeat the line above in comment form, say _why_ this is disabled in
dump-without-crashing mode

[Wez, 2016-03-31 23:13:37]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-03-31 23:13:38]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
thakis@chromium.org, scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/1814423002/#ps240001
(title: "Tweak comment")

[commit-bot: I haz the power, 2016-03-31 23:14:15]

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

[Wez, 2016-03-31 23:14:52]

On 2016/03/31 22:11:18, Nico wrote:
> lgtm
> 
>
https://codereview.chromium.org/1814423002/diff/220001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/wtf/Assertions.h (right):
> 
>
https://codereview.chromium.org/1814423002/diff/220001/third_party/WebKit/Sou...
> third_party/WebKit/Source/wtf/Assertions.h:55: /* Enable ASSERT* macros if
> DCHECK is on, unless it dumps-without-crashing. */
> nit: use // comments
> also, don't repeat the line above in comment form, say _why_ this is disabled
in
> dump-without-crashing mode

This header has a comment at the top stating that it needs to be includable from
Objective C; presumably that's no longer the case but let's clean up the style
outside this CL. :)

[commit-bot: I haz the power, 2016-04-01 01:10:58]

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

[commit-bot: I haz the power, 2016-04-01 01:10:59]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Nico, 2016-04-01 01:15:51]

On Thu, Mar 31, 2016 at 7:14 PM, <wez@chromium.org> wrote:

> On 2016/03/31 22:11:18, Nico wrote:
> > lgtm
> >
> >
>
>
https://codereview.chromium.org/1814423002/diff/220001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/wtf/Assertions.h (right):
> >
> >
>
>
https://codereview.chromium.org/1814423002/diff/220001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/wtf/Assertions.h:55: /* Enable ASSERT* macros
> if
> > DCHECK is on, unless it dumps-without-crashing. */
> > nit: use // comments
> > also, don't repeat the line above in comment form, say _why_ this is
> disabled
> in
> > dump-without-crashing mode
>
> This header has a comment at the top stating that it needs to be
> includable from
> Objective C; presumably that's no longer the case but let's clean up the
> style
> outside this CL. :)
>

// comments work in Objective-C

-- 
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.

[Wez, 2016-04-01 16:54:22]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-04-01 16:54:42]

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

[commit-bot: I haz the power, 2016-04-01 20:18:30]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

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

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

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

[commit-bot: I haz the power, 2016-04-01 20:18:32]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-04-01 20:20:13]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

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

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}
==========

[commit-bot: I haz the power, 2016-04-01 20:20:15]

Patchset 13 (id:??) landed as
https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}

[Wez, 2016-04-01 22:33:21]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/1846373002/ by wez@chromium.org.

The reason for reverting is: We will run this trial the week of April 4th =>
reverting until then..

[Wez, 2016-04-01 22:34:13]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}
==========

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}
==========

[Wez, 2016-04-04 17:18:16]

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

[commit-bot: I haz the power, 2016-04-04 17:18:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1814423002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1814423002/240001

[commit-bot: I haz the power, 2016-04-04 21:05:55]

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

[commit-bot: I haz the power, 2016-04-04 21:05:56]

Dry run: This issue passed the CQ dry run.

[Wez, 2016-04-06 00:28:54]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-04-06 00:29:16]

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

[commit-bot: I haz the power, 2016-04-06 06:30:50]

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

[commit-bot: I haz the power, 2016-04-06 06:30:52]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Wez, 2016-04-06 07:35:43]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2016-04-06 07:36:02]

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

[commit-bot: I haz the power, 2016-04-06 09:12:22]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}
==========

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}
==========

[commit-bot: I haz the power, 2016-04-06 09:12:24]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-04-06 09:15:05]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}
==========

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}

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

[commit-bot: I haz the power, 2016-04-06 09:15:07]

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

[gab, 2016-04-07 14:42:19]

Wasn't this supposed to be reverted right after the Canary cut? Surprised that
it's been live for 30 hours now.

[gab, 2016-04-07 14:44:45]

Description was changed from

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}

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

to

==========
Patch to try dump-on-DCHECK.

This patch does two things:

1. Adds a flag to switch DCHECK from logging, dumping, and then crashing the
process, to only uploading a crash dump, and only on the first failed DCHECK in
each process.

2. Forces that flag, and DCHECK_ALWAYS_ON, on in Windows official builds.

All non-debug e.g. CHECK behaviours remain unchanged; the intended effect is for
DCHECKs to switch from no-ops to uploading dumps without crashing, in Windows
official builds.

Note that this CL is intended to be landed, a branch cut to release from, and
then immediately reverted; it is not intended to be landed in Chromium for any
longer period.

BUG=596231

Committed: https://crrev.com/6436ac7ddec4b2b3aba4ee38aabe7dffe238a077
Cr-Commit-Position: refs/heads/master@{#383894}

Committed: https://crrev.com/bac26c8a840909a679a5a74557fa6f4f60ae9e07
Cr-Commit-Position: refs/heads/master@{#384011}

Committed: https://crrev.com/16502cb143f737bafad5d035b8ed6d76aabce288
Cr-Commit-Position: refs/heads/master@{#384675}

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

[laforge, 2016-04-07 14:52:56]

It's only been live (on Canary) for about 6 hours, but we should revert it
early today so that it's not on tomorrow's Canary.

Kind Regards,

Anthony Laforge
Technical Program Manager
Mountain View, CA

On Thu, Apr 7, 2016 at 7:42 AM, <gab@chromium.org> wrote:

> Wasn't this supposed to be reverted right after the Canary cut? Surprised
> that
> it's been live for 30 hours now.
>
> https://codereview.chromium.org/1814423002/
>

-- 
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.

[Wez, 2016-04-07 15:10:51]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/1870633003/ by wez@chromium.org.

The reason for reverting is: We have a Canary w/ dump-on-DCHECK - time to
revert!.

[Wez, 2016-04-14 01:14:48]

Uploaded a tweaked patch to crrev.com/1884023002, rather than keep re-using this
one. :)