Various logging-related cleanups & reformatting.

Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.  They include
some minor test and formatting changes.

BUG=596231
Review-Url: https://codereview.chromium.org/2712823003
Cr-Commit-Position: refs/heads/master@{#480316}
Committed: https://chromium.googlesource.com/chromium/src/+/a245bd07a1e59556dc6c967a1c53c439c3dcad9e

[Wez, 2017-02-23 05:53:57]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 05:54:24]

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

[Wez, 2017-02-23 06:13:11]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These were encountered while preparing the dump-on-DCHECK patch.

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

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

Some reformatting changes as per git cl format:


Some functional changes to support dump-on-DCHECK:

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

[commit-bot: I haz the power, 2017-02-23 06:14:56]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-02-23 06:14:57]

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

[Wez, 2017-02-23 06:17:03]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

Some reformatting changes as per git cl format:


Some functional changes to support dump-on-DCHECK:

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

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

Besides reformatting and naming cleanups there is a functional change
to PartitionAllocator, to enable cookie-padding only in Debug builds,
rather than in all DCHECK-enabled builds.

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

[Wez, 2017-02-23 06:23:50]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 06:24:09]

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

[Wez, 2017-02-23 06:24:29]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 06:24:44]

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

[Wez, 2017-02-23 06:36:21]

[email protected] changed reviewers:
+ [email protected], [email protected], [email protected]

[Wez, 2017-02-23 06:36:22]

palmer: As discussed, this makes PartitionAllocator cookie-padding conditional
on Debug vs Release, rather than DCHECK-is-on.

haraken: PTAL WTF & PartitionAlloc.
dcheng: PTAL everything else.

[dcheng, 2017-02-23 08:19:26]

//base LGTM

[haraken, 2017-02-23 08:39:15]

LGTM but...

On 2017/02/23 06:36:22, Wez wrote:
> palmer: As discussed, this makes PartitionAllocator cookie-padding conditional
> on Debug vs Release, rather than DCHECK-is-on.

would you help me understand why you want to restrict the cookie-padding to
Debug builds only?

[commit-bot: I haz the power, 2017-02-23 11:13:51]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-02-23 11:13:51]

Dry run: This issue passed the CQ dry run.

[palmer, 2017-02-23 18:53:24]

> would you help me understand why you want to restrict the cookie-padding to
Debug builds only?

That's my question, too. I *think* it might be the case that some fuzzers use
Release builds with DCHECK-is-on to get some of the checks but still be
faster/smaller. I think. Let me check with the fuzzing crew.

[Wez, 2017-02-23 19:48:46]

As we discussed on the dump-on-DCHECK patch, Chris, the issue is that the
DCHECK in there is actually security-sensitive, so if someone were to
enable DCHECKs but not have them crash then we've lost that protection.

The alternatives would be to (1) fix PartitionAlloc to have explicit
sanity-checks independent of build config or (2) change the DCHECK to a
CHECK so it'll crash even in dump-on-DCHECK builds.

On 23 February 2017 at 10:53, <[email protected]> wrote:

> > would you help me understand why you want to restrict the cookie-padding
> to
> Debug builds only?
>
> That's my question, too. I *think* it might be the case that some fuzzers
> use
> Release builds with DCHECK-is-on to get some of the checks but still be
> faster/smaller. I think. Let me check with the fuzzing crew.
>
> https://codereview.chromium.org/2712823003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].

[Wez, 2017-02-23 19:48:47]

As we discussed on the dump-on-DCHECK patch, Chris, the issue is that the
DCHECK in there is actually security-sensitive, so if someone were to
enable DCHECKs but not have them crash then we've lost that protection.

The alternatives would be to (1) fix PartitionAlloc to have explicit
sanity-checks independent of build config or (2) change the DCHECK to a
CHECK so it'll crash even in dump-on-DCHECK builds.

On 23 February 2017 at 10:53, <[email protected]> wrote:

> > would you help me understand why you want to restrict the cookie-padding
> to
> Debug builds only?
>
> That's my question, too. I *think* it might be the case that some fuzzers
> use
> Release builds with DCHECK-is-on to get some of the checks but still be
> faster/smaller. I think. Let me check with the fuzzing crew.
>
> https://codereview.chromium.org/2712823003/
>

-- 
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 [email protected].

[haraken, 2017-02-23 21:27:03]

On 2017/02/23 19:48:47, Wez wrote:
> As we discussed on the dump-on-DCHECK patch, Chris, the issue is that the
> DCHECK in there is actually security-sensitive, so if someone were to
> enable DCHECKs but not have them crash then we've lost that protection.
> 
> The alternatives would be to (1) fix PartitionAlloc to have explicit
> sanity-checks independent of build config or (2) change the DCHECK to a
> CHECK so it'll crash even in dump-on-DCHECK builds.
> 
> On 23 February 2017 at 10:53, <mailto:[email protected]> wrote:
> 
> > > would you help me understand why you want to restrict the cookie-padding
> > to
> > Debug builds only?
> >
> > That's my question, too. I *think* it might be the case that some fuzzers
> > use
> > Release builds with DCHECK-is-on to get some of the checks but still be
> > faster/smaller. I think. Let me check with the fuzzing crew.
> >
> > https://codereview.chromium.org/2712823003/
> >
> 
> -- 
> 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 mailto:[email protected].

Note that the cookie-padding should not be enabled on production builds because
it affects performance and memory.

[Wez, 2017-02-24 01:04:22]

On 2017/02/23 21:27:03, haraken wrote:
> On 2017/02/23 19:48:47, Wez wrote:
> > As we discussed on the dump-on-DCHECK patch, Chris, the issue is that the
> > DCHECK in there is actually security-sensitive, so if someone were to
> > enable DCHECKs but not have them crash then we've lost that protection.
> > 
> > The alternatives would be to (1) fix PartitionAlloc to have explicit
> > sanity-checks independent of build config or (2) change the DCHECK to a
> > CHECK so it'll crash even in dump-on-DCHECK builds.
> > 
> > On 23 February 2017 at 10:53, <mailto:[email protected]> wrote:
> > 
> > > > would you help me understand why you want to restrict the cookie-padding
> > > to
> > > Debug builds only?
> > >
> > > That's my question, too. I *think* it might be the case that some fuzzers
> > > use
> > > Release builds with DCHECK-is-on to get some of the checks but still be
> > > faster/smaller. I think. Let me check with the fuzzing crew.
> > >
> > > https://codereview.chromium.org/2712823003/
> > >
> > 
> > -- 
> > 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 mailto:[email protected].
> 
> Note that the cookie-padding should not be enabled on production builds
because
> it affects performance and memory.

For the purposes of dump-on-DCHECK that restriction would actually be sufficient
to "fix" things; DoD builds would have DCHECK_IS_ON() but would still not get
cookie-padding.

Rather than make every one of the PA conditionals test both, how would you feel
about something like:

#if !defined(ENABLE_PARTITION_ALLOC_DEBUG)
#if DCHECK_IS_ON() && !OFFICIAL_BUILD
#define ENABLE_PARTITION_ALLOC_DEBUG() true
#else
#define ENABLE_PARTITION_ALLOC_DEBUG() false
#endif
#endif

[palmer, 2017-02-24 02:28:42]

> As we discussed on the dump-on-DCHECK patch, Chris,

Ah yes, I had to page that back in. I had forgotten!

> the issue is that the
> DCHECK in there is actually security-sensitive, so if someone were to
> enable DCHECKs but not have them crash then we've lost that protection.
> 
> The alternatives would be to (1) fix PartitionAlloc to have explicit
> sanity-checks independent of build config or (2) change the DCHECK to a
> CHECK so it'll crash even in dump-on-DCHECK builds.

Isn't (2) basically 1 way of doing (1)? Either way, fine by me.

I ran this by the fuzzing crew and they said they do not rely on Release builds
that have DCHECK_IS_ON. So no problem there.

[palmer, 2017-02-24 02:30:32]

lgtm

[haraken, 2017-02-24 02:33:06]

Getting back to the original question, what is a reason we have to drop the
cookie-padding from DCHECK builds?

The intention of the cookie-padding is to enable the security protection on all
non-production builds.

[Wez, 2017-02-25 22:28:28]

The problem is that PartitionAlloc doesn't currently have a single place
where it sanity-checks the size parameter; when DCHECK_IS_ON() is enabled
we enable cookie-padding, and rely on this DCHECK actually crashing the
calling process, rather than allowing overrun to occur.  With
dump-on-DCHECK the DCHECK won't crash, hence the problem

Also, bear in mind that many developers rely on Release builds having
more-or-less the same performance as Official builds (modulo PGO effects),
so we should be careful about introducing differences in performance based
on whether the build is "Official"/production.

On 23 February 2017 at 18:33, <[email protected]> wrote:

> Getting back to the original question, what is a reason we have to drop the
> cookie-padding from DCHECK builds?
>
> The intention of the cookie-padding is to enable the security protection
> on all
> non-production builds.
>
>
> https://codereview.chromium.org/2712823003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].

[Wez, 2017-02-25 22:28:29]

The problem is that PartitionAlloc doesn't currently have a single place
where it sanity-checks the size parameter; when DCHECK_IS_ON() is enabled
we enable cookie-padding, and rely on this DCHECK actually crashing the
calling process, rather than allowing overrun to occur.  With
dump-on-DCHECK the DCHECK won't crash, hence the problem

Also, bear in mind that many developers rely on Release builds having
more-or-less the same performance as Official builds (modulo PGO effects),
so we should be careful about introducing differences in performance based
on whether the build is "Official"/production.

On 23 February 2017 at 18:33, <[email protected]> wrote:

> Getting back to the original question, what is a reason we have to drop the
> cookie-padding from DCHECK builds?
>
> The intention of the cookie-padding is to enable the security protection
> on all
> non-production builds.
>
>
> https://codereview.chromium.org/2712823003/
>

-- 
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 [email protected].

[haraken, 2017-02-26 16:14:03]

On 2017/02/25 22:28:29, Wez wrote:
> The problem is that PartitionAlloc doesn't currently have a single place
> where it sanity-checks the size parameter; when DCHECK_IS_ON() is enabled
> we enable cookie-padding, and rely on this DCHECK actually crashing the
> calling process, rather than allowing overrun to occur.  With
> dump-on-DCHECK the DCHECK won't crash, hence the problem

So is the CL the only way to circumvent the problem? Then I'm okay with it.

> 
> Also, bear in mind that many developers rely on Release builds having
> more-or-less the same performance as Official builds (modulo PGO effects),
> so we should be careful about introducing differences in performance based
> on whether the build is "Official"/production.

This is a wrong assumption though. Release builds can sometimes be much slower
than official builds, so we must use official builds to measure
performance/memory.

[Wez, 2017-06-03 00:00:51]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-06-03 00:01:33]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-03 00:16:19]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-06-03 00:16:19]

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

[Wez, 2017-06-17 00:59:54]

The CQ bit was checked by [email protected] to run a CQ dry run

[commit-bot: I haz the power, 2017-06-17 01:00:16]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-17 03:27:20]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2017-06-17 03:27:21]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-06-18 04:22:59]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

Besides reformatting and naming cleanups there is a functional change
to PartitionAllocator, to enable cookie-padding only in Debug builds,
rather than in all DCHECK-enabled builds.

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

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

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

[Wez, 2017-06-18 04:25:10]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.

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

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.  They include
some minor test and formatting changes.

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

[Wez, 2017-06-18 04:25:17]

The CQ bit was checked by [email protected]

[Wez, 2017-06-18 04:25:18]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected],
[email protected], [email protected]
Link to the patchset: https://codereview.chromium.org/2712823003/#ps60001
(title: "Fix base unit-tests")

[commit-bot: I haz the power, 2017-06-18 04:25:28]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-18 23:26:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497759917761130,
"parent_rev": "eb339abb6c31f0f6a02d7b7038031ced69898eea", "commit_rev":
"a245bd07a1e59556dc6c967a1c53c439c3dcad9e"}

[commit-bot: I haz the power, 2017-06-18 23:26:47]

Description was changed from

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.  They include
some minor test and formatting changes.

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

to

==========
Various logging-related cleanups & reformatting.

These changes were part of the dump-on-DCHECK patch,  (see
https://codereview.chromium.org/1884023002/) but are really just
cleanup/reformatting, so should land separately.  They include
some minor test and formatting changes.

BUG=596231

Review-Url: https://codereview.chromium.org/2712823003
Cr-Commit-Position: refs/heads/master@{#480316}
Committed:
https://chromium.googlesource.com/chromium/src/+/a245bd07a1e59556dc6c967a1c53...
==========

[commit-bot: I haz the power, 2017-06-18 23:26:49]

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