[sql] Prevent recovery against a closed database.

[sql] Prevent recovery against a closed database.

The referenced bug is a crash which appears to happen when recovery is
attempted against a poisoned (or closed) database.  Modify
sql::Recovery::Begin() to explicitly short-circuit recovery if the
passed connection handle is not open.  Otherwise the recovery code will
work until the recovered data is restored over the original, and then
fail.

Also modify the sql::Connection::ReportDiagnosticInfo() to clear the
error callback while doing any integrity check.  This is a bit of a
hack in the interests of not rooting out the crazy error-callback
assumptions at this time.

BUG=583106
Committed: https://crrev.com/874ea1bd6ec404bc536c45098910694dea5adb34
Cr-Commit-Position: refs/heads/master@{#372896}

[Scott Hess - ex-Googler, 2016-02-02 01:00:47]

shess@chromium.org changed reviewers:
+ rch@chromium.org

[Scott Hess - ex-Googler, 2016-02-02 01:00:48]

Awhile back you offered to be in sql/OWNERS.  Still up for that?  I have some
cleanup in the sql::Recovery code I'd like to land, which will involve a
half-dozen or so CLs.  Thanks.

[Before you ask if there's someone else more-familiar with the code, nobody but
me works in here, so whoever gets to review is starting at basically the same
level.]

[Ryan Hamilton, 2016-02-02 05:00:07]

LGTM

Well, this change looks simple enough. If the other dozen CLs are like this,
that sounds great :>

https://codereview.chromium.org/1657593005/diff/20001/sql/recovery_unittest.cc
File sql/recovery_unittest.cc (right):

https://codereview.chromium.org/1657593005/diff/20001/sql/recovery_unittest.c...
sql/recovery_unittest.cc:113: ASSERT_FALSE(recovery.get());
Presumably, this test failed or crashed with the old code?

[Scott Hess - ex-Googler, 2016-02-02 05:06:48]

On 2016/02/02 05:00:07, Ryan Hamilton wrote:
> LGTM
> 
> Well, this change looks simple enough. If the other dozen CLs are like this,
> that sounds great :>

Unfortunately, probably not.  But I will try hard to isolate the "moving things
around to make things sane" from the "oh, yes, this is a subtle change" bits.

> https://codereview.chromium.org/1657593005/diff/20001/sql/recovery_unittest.cc
> File sql/recovery_unittest.cc (right):
> 
>
https://codereview.chromium.org/1657593005/diff/20001/sql/recovery_unittest.c...
> sql/recovery_unittest.cc:113: ASSERT_FALSE(recovery.get());
> Presumably, this test failed or crashed with the old code?

Yep.  A bit arbitrary, but testing it in a way which simulates the actual
problem from the bug is intrusive and not really representative, so I figured
just test that the path to the problem isn't blocked, and be done.

[Scott Hess - ex-Googler, 2016-02-02 05:07:16]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2016-02-02 05:08:32]

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

[commit-bot: I haz the power, 2016-02-02 05:15:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-02 05:16:30]

Description was changed from

==========
[sql] Prevent recovery against a closed database.

The referenced bug is a crash which appears to happen when recovery is
attempted against a poisoned (or closed) database.  Modify
sql::Recovery::Begin() to explicitly short-circuit recovery if the
passed connection handle is not open.  Otherwise the recovery code will
work until the recovered data is restored over the original, and then
fail.

Also modify the sql::Connection::ReportDiagnosticInfo() to clear the
error callback while doing any integrity check.  This is a bit of a
hack in the interests of not rooting out the crazy error-callback
assumptions at this time.

BUG=583106
==========

to

==========
[sql] Prevent recovery against a closed database.

The referenced bug is a crash which appears to happen when recovery is
attempted against a poisoned (or closed) database.  Modify
sql::Recovery::Begin() to explicitly short-circuit recovery if the
passed connection handle is not open.  Otherwise the recovery code will
work until the recovered data is restored over the original, and then
fail.

Also modify the sql::Connection::ReportDiagnosticInfo() to clear the
error callback while doing any integrity check.  This is a bit of a
hack in the interests of not rooting out the crazy error-callback
assumptions at this time.

BUG=583106

Committed: https://crrev.com/874ea1bd6ec404bc536c45098910694dea5adb34
Cr-Commit-Position: refs/heads/master@{#372896}
==========

[commit-bot: I haz the power, 2016-02-02 05:16:30]

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