New test and off-by-one fix for data persisted to disk.

New test and off-by-one fix for data persisted to disk.

Writing to disk via data()/used() creates a file of the minimum
possible size which exposed an off-by-one bug in a corruption
check.

BUG=

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

[bcwhite, 2016-02-01 13:53:48]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2016-02-08 21:43:26]

https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
File base/metrics/persistent_memory_allocator_unittest.cc (right):

https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
base/metrics/persistent_memory_allocator_unittest.cc:448: #if !DCHECK_IS_ON() 
// DCHECK builds die at a various debug checks.
Aren't most of our tests run with DCHECKs? So this is disabling testing for
majority of our infrastructure.

Which DCHECKs does this hit? Can the code be changed to avoid hitting those
DCHECKs instead of not running this code if DCHECKs are on?

[bcwhite, 2016-02-10 16:28:58]

https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
File base/metrics/persistent_memory_allocator_unittest.cc (right):

https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
base/metrics/persistent_memory_allocator_unittest.cc:448: #if !DCHECK_IS_ON() 
// DCHECK builds die at a various debug checks.
On 2016/02/08 21:43:26, Alexei Svitkine wrote:
> Aren't most of our tests run with DCHECKs? So this is disabling testing for
> majority of our infrastructure.
> 
> Which DCHECKs does this hit? Can the code be changed to avoid hitting those
> DCHECKs instead of not running this code if DCHECKs are on?

I think all the bot-tests run off a release build.

The checks being hit are the "hardening" code that protects against malicious
actors.  This test ends up producing invalid memory segments which hit those
NOTREACHED() statements.  This is intentional, but we need to verify that it
does the right thing by handling the situation whereas in a real build, we want
to be notified immediately that something happened that shouldn't in a
well-behaved system.

[Alexei Svitkine (slow), 2016-02-10 16:35:34]

https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
File base/metrics/persistent_memory_allocator_unittest.cc (right):

https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
base/metrics/persistent_memory_allocator_unittest.cc:448: #if !DCHECK_IS_ON() 
// DCHECK builds die at a various debug checks.
On 2016/02/10 16:28:58, bcwhite wrote:
> On 2016/02/08 21:43:26, Alexei Svitkine wrote:
> > Aren't most of our tests run with DCHECKs? So this is disabling testing for
> > majority of our infrastructure.
> > 
> > Which DCHECKs does this hit? Can the code be changed to avoid hitting those
> > DCHECKs instead of not running this code if DCHECKs are on?
> 
> I think all the bot-tests run off a release build.
> 
> The checks being hit are the "hardening" code that protects against malicious
> actors.  This test ends up producing invalid memory segments which hit those
> NOTREACHED() statements.  This is intentional, but we need to verify that it
> does the right thing by handling the situation whereas in a real build, we
want
> to be notified immediately that something happened that shouldn't in a
> well-behaved system.

I believe the bots run Release with DCHECK on.

I would prefer a solution that lets us run these tests with DCHECKs enabled.
Perhaps the NOTREACHED() can be done by the caller when it gets a return value
that indicates a failure? Or the test can mock out a function that otherwise
would do a NOTREACHED()?

[bcwhite, 2016-02-10 16:54:10]

https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
File base/metrics/persistent_memory_allocator_unittest.cc (right):

https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
base/metrics/persistent_memory_allocator_unittest.cc:448: #if !DCHECK_IS_ON() 
// DCHECK builds die at a various debug checks.
On 2016/02/10 16:35:34, Alexei Svitkine wrote:
> On 2016/02/10 16:28:58, bcwhite wrote:
> > On 2016/02/08 21:43:26, Alexei Svitkine wrote:
> > > Aren't most of our tests run with DCHECKs? So this is disabling testing
for
> > > majority of our infrastructure.
> > > 
> > > Which DCHECKs does this hit? Can the code be changed to avoid hitting
those
> > > DCHECKs instead of not running this code if DCHECKs are on?
> > 
> > I think all the bot-tests run off a release build.
> > 
> > The checks being hit are the "hardening" code that protects against
malicious
> > actors.  This test ends up producing invalid memory segments which hit those
> > NOTREACHED() statements.  This is intentional, but we need to verify that it
> > does the right thing by handling the situation whereas in a real build, we
> want
> > to be notified immediately that something happened that shouldn't in a
> > well-behaved system.
> 
> I believe the bots run Release with DCHECK on.
> 
> I would prefer a solution that lets us run these tests with DCHECKs enabled.
> Perhaps the NOTREACHED() can be done by the caller when it gets a return value
> that indicates a failure? Or the test can mock out a function that otherwise
> would do a NOTREACHED()?

Complicated.  Result to the caller is always "good".  Production code should
never have to call "IsCorrupt()" or check the return values any more that usual.

I could create a "IgnoreDChecksForTesting()" that sets a flag and then change
NOTREACHED() to DCHECK(testing).  Sound reasonable?

[Alexei Svitkine (slow), 2016-02-10 17:17:45]

That SGTM - as a setter on the allocator. Maybe make it more targeted (i.e.
specifically ignore "corrupted" dchecks).

On Wed, Feb 10, 2016 at 11:54 AM, <bcwhite@chromium.org> wrote:

>
>
https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
> File base/metrics/persistent_memory_allocator_unittest.cc (right):
>
https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...
> base/metrics/persistent_memory_allocator_unittest.cc:448
<https://codereview.chromium.org/1654053002/diff/1/base/metrics/persistent_mem...>:
#if
> !DCHECK_IS_ON()  // DCHECK builds die at a various debug checks.
> On 2016/02/10 16:35:34, Alexei Svitkine wrote:
> > On 2016/02/10 16:28:58, bcwhite wrote:
> > > On 2016/02/08 21:43:26, Alexei Svitkine wrote:
> > > > Aren't most of our tests run with DCHECKs? So this is disabling
> testing for
> > > > majority of our infrastructure.
> > > >
> > > > Which DCHECKs does this hit? Can the code be changed to avoid
> hitting those
> > > > DCHECKs instead of not running this code if DCHECKs are on?
> > >
> > > I think all the bot-tests run off a release build.
> > >
> > > The checks being hit are the "hardening" code that protects against
> malicious
> > > actors.  This test ends up producing invalid memory segments which
> hit those
> > > NOTREACHED() statements.  This is intentional, but we need to verify
> that it
> > > does the right thing by handling the situation whereas in a real
> build, we
> > want
> > > to be notified immediately that something happened that shouldn't in
> a
> > > well-behaved system.
> >
> > I believe the bots run Release with DCHECK on.
> >
> > I would prefer a solution that lets us run these tests with DCHECKs
> enabled.
> > Perhaps the NOTREACHED() can be done by the caller when it gets a
> return value
> > that indicates a failure? Or the test can mock out a function that
> otherwise
> > would do a NOTREACHED()?
>
> Complicated.  Result to the caller is always "good".  Production code
> should never have to call "IsCorrupt()" or check the return values any
> more that usual.
>
> I could create a "IgnoreDChecksForTesting()" that sets a flag and then
> change NOTREACHED() to DCHECK(testing).  Sound reasonable?
> https://codereview.chromium.org/1654053002/
>
>

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

[bcwhite, 2016-02-10 18:18:33]

> That SGTM - as a setter on the allocator. Maybe make it more targeted (i.e.
> specifically ignore "corrupted" dchecks).

Well...  That didn't work.  The crash is in the constructor which is before
setting any flag and I really don't want it to be a ctor parameter.  I just
removed the two checks that were crashing.  No big loss, I guess.

[Alexei Svitkine (slow), 2016-02-10 18:29:22]

lgtm

[bcwhite, 2016-02-10 20:01:41]

The CQ bit was checked by bcwhite@chromium.org

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

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

[commit-bot: I haz the power, 2016-02-10 21:56:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-16 22:31:24]

Description was changed from

==========
New test and off-by-one fix for data persisted to disk.

Writing to disk via data()/used() creates a file of the minimum
possible size which exposed an off-by-one bug in a corruption
check.

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

to

==========
New test and off-by-one fix for data persisted to disk.

Writing to disk via data()/used() creates a file of the minimum
possible size which exposed an off-by-one bug in a corruption
check.

BUG=

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

[commit-bot: I haz the power, 2016-02-16 22:31:24]

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