[sql] Callback and scoped-ignore tests for sql::Statement.

[sql] Callback and scoped-ignore tests for sql::Statement.

Test error callback and ScopedErrorIgnorer for running statements.
Share error-callback test code between statement and connection tests.
Test additional error-callback edge cases.

Also fix callback use-after-free case.

BUG=254584
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211281

[Scott Hess - ex-Googler, 2013-06-25 20:58:53]

Circling back on something Erik noted as being left-over after some recent
refactoring.  But Erik may-be/probably-is on vacation.  So if either of you can
grab this, perhaps pop up an ACK.  Thanks.

[Greg Billock, 2013-06-27 20:27:26]

https://codereview.chromium.org/17726002/diff/1/sql/statement_unittest.cc
File sql/statement_unittest.cc (right):

https://codereview.chromium.org/17726002/diff/1/sql/statement_unittest.cc#new...
sql/statement_unittest.cc:83: sql::ScopedErrorCallback sec(
The way I'd envision this being used is like this:

{
  sql::ScopedErrorCallback cb(&db());
  ASSERT_TRUE(db().Execute("CREATE TABLE ..."));
  EXPECT_EQ(SQLITE_OK, cb.getError());
}

That would keep the callback private inside the ScopedErrorCallback.

https://codereview.chromium.org/17726002/diff/1/sql/test/error_callback_suppo...
File sql/test/error_callback_support.cc (right):

https://codereview.chromium.org/17726002/diff/1/sql/test/error_callback_suppo...
sql/test/error_callback_support.cc:16: : db_(db) {
Any need to check if one has already been set? Perhaps that should be in
connection.h, check error_callback_.is_null ...

[Scott Hess - ex-Googler, 2013-06-27 20:59:45]

https://codereview.chromium.org/17726002/diff/1/sql/statement_unittest.cc
File sql/statement_unittest.cc (right):

https://codereview.chromium.org/17726002/diff/1/sql/statement_unittest.cc#new...
sql/statement_unittest.cc:83: sql::ScopedErrorCallback sec(
On 2013/06/27 20:27:26, Greg Billock wrote:
> The way I'd envision this being used is like this:
> 
> {
>   sql::ScopedErrorCallback cb(&db());
>   ASSERT_TRUE(db().Execute("CREATE TABLE ..."));
>   EXPECT_EQ(SQLITE_OK, cb.getError());
> }
> 
> That would keep the callback private inside the ScopedErrorCallback.

Hmm.  You have given me something to think about.  On the one hand, this makes
sense in terms of use cases like you describe above.  On the other hand, this
test is testing that the callback is called, as opposed to whether an error
happened.  Having the callback itself embedded in the scoper would result in a
little less API exposed.  Maybe I'll mock it up and look at it.

https://codereview.chromium.org/17726002/diff/1/sql/test/error_callback_suppo...
File sql/test/error_callback_support.cc (right):

https://codereview.chromium.org/17726002/diff/1/sql/test/error_callback_suppo...
sql/test/error_callback_support.cc:16: : db_(db) {
On 2013/06/27 20:27:26, Greg Billock wrote:
> Any need to check if one has already been set? Perhaps that should be in
> connection.h, check error_callback_.is_null ...

Yeah, probably.  I'm inclined towards a DCHECK for now, since I'm not sure if
nesting would be desirable.

[Scott Hess - ex-Googler, 2013-06-28 22:04:37]

Sorry for the delay - poking around in my overall sql-test-cleanup CLs, I found
another case which was specifically wrong in how error callbacks work.  I've
entered crbug.com/254584 about whether this is a general callback issue or not,
but it seems reasonable for testing the issue (and fixing it) to be landed in
this same code (since it's about actually testing the callback machinery).

I'm OOT next week, so I'm not sure if this is high priority or not.  Like if you
look at it and go "Yes, this is correct", then I guess it's high priority, but
if it's like "Scott, you're being dumb over here," then I'm unlikely to circle
back on it for the next week.

Thanks,
scott

https://codereview.chromium.org/17726002/diff/1/sql/statement_unittest.cc
File sql/statement_unittest.cc (right):

https://codereview.chromium.org/17726002/diff/1/sql/statement_unittest.cc#new...
sql/statement_unittest.cc:83: sql::ScopedErrorCallback sec(
On 2013/06/27 20:59:45, shess wrote:
> On 2013/06/27 20:27:26, Greg Billock wrote:
> > The way I'd envision this being used is like this:
> > 
> > {
> >   sql::ScopedErrorCallback cb(&db());
> >   ASSERT_TRUE(db().Execute("CREATE TABLE ..."));
> >   EXPECT_EQ(SQLITE_OK, cb.getError());
> > }
> > 
> > That would keep the callback private inside the ScopedErrorCallback.
> 
> Hmm.  You have given me something to think about.  On the one hand, this makes
> sense in terms of use cases like you describe above.  On the other hand, this
> test is testing that the callback is called, as opposed to whether an error
> happened.  Having the callback itself embedded in the scoper would result in a
> little less API exposed.  Maybe I'll mock it up and look at it.

I found a new case where error callbacks were broken, which needs different
callback behavior.  So I'm keeping the scoper separate from the trivial callback
for now.

I do think it might be reasonable to shift the generally-useful scoper to a
specific header/impl file, and just pull the trivial error-capture callback into
both unittest files.  WDYT?

https://codereview.chromium.org/17726002/diff/1/sql/test/error_callback_suppo...
File sql/test/error_callback_support.cc (right):

https://codereview.chromium.org/17726002/diff/1/sql/test/error_callback_suppo...
sql/test/error_callback_support.cc:16: : db_(db) {
On 2013/06/27 20:59:45, shess wrote:
> On 2013/06/27 20:27:26, Greg Billock wrote:
> > Any need to check if one has already been set? Perhaps that should be in
> > connection.h, check error_callback_.is_null ...
> 
> Yeah, probably.  I'm inclined towards a DCHECK for now, since I'm not sure if
> nesting would be desirable.

A DCHECK would result in moving set_error_callback() from .h to .cc, and then I
think it needs renamed to SetErrorCallback(), in which case consistency probably
wants ResetErrorCallback().

I think that having the scoper displace an existing callback is almost certainly
wrong, but I'm not entirely convinced that a real-world user should be required
to reset the callback before setting a new one.  So I've adjusted things so that
this code EXPECTs to not have an existing callback in place.

[erikwright (departed), 2013-07-05 19:16:54]

https://codereview.chromium.org/17726002/diff/13001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/17726002/diff/13001/sql/connection.cc#newcode804
sql/connection.cc:804: // Fire from a copy of the callback in case of reentry
into
I would argue that this is a bug in Callback.

Should the storage _really_ be freed while invoking the method? I'd argue that
it should be temporarily add-ref'd.

Suggestion: ask ajwong@ for his opinion.

[Scott Hess - ex-Googler, 2013-07-08 21:15:35]

https://codereview.chromium.org/17726002/diff/13001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/17726002/diff/13001/sql/connection.cc#newcode804
sql/connection.cc:804: // Fire from a copy of the callback in case of reentry
into
On 2013/07/05 19:16:55, erikwright wrote:
> I would argue that this is a bug in Callback.
> 
> Should the storage _really_ be freed while invoking the method? I'd argue that
> it should be temporarily add-ref'd.
> 
> Suggestion: ask ajwong@ for his opinion.

I had logged http://crbug.com/254584 , but since I've not seen any action on it
I'll follow up in case it hit a filter.

[Scott Hess - ex-Googler, 2013-07-10 22:03:19]

BTW - AFAICT, if the callback code is modified to implicitly addref, I believe
that my solution will continue to work, so this should be safe.  I've added a
bug ref and BUG= so that someone knows to circle back on it.

Are either of you willing to move forward on this?  Or do I need to make further
changes?  The only thing I'm really debating internally is whether
error_callback_support.h and .cc should instead just be test_support.h and .cc,
to make them an explicit grab-bag destination for test helpers.

Thanks,
scott


On 2013/07/08 21:15:35, shess wrote:
> https://codereview.chromium.org/17726002/diff/13001/sql/connection.cc
> File sql/connection.cc (right):
> 
>
https://codereview.chromium.org/17726002/diff/13001/sql/connection.cc#newcode804
> sql/connection.cc:804: // Fire from a copy of the callback in case of reentry
> into
> On 2013/07/05 19:16:55, erikwright wrote:
> > I would argue that this is a bug in Callback.
> > 
> > Should the storage _really_ be freed while invoking the method? I'd argue
that
> > it should be temporarily add-ref'd.
> > 
> > Suggestion: ask ajwong@ for his opinion.
> 
> I had logged http://crbug.com/254584 , but since I've not seen any action on
it
> I'll follow up in case it hit a filter.

[Greg Billock, 2013-07-10 22:16:05]

lgtm

https://codereview.chromium.org/17726002/diff/1/sql/statement_unittest.cc
File sql/statement_unittest.cc (right):

https://codereview.chromium.org/17726002/diff/1/sql/statement_unittest.cc#new...
sql/statement_unittest.cc:83: sql::ScopedErrorCallback sec(
On 2013/06/28 22:04:37, shess wrote:
> On 2013/06/27 20:59:45, shess wrote:
> > On 2013/06/27 20:27:26, Greg Billock wrote:
> > > The way I'd envision this being used is like this:
> > > 
> > > {
> > >   sql::ScopedErrorCallback cb(&db());
> > >   ASSERT_TRUE(db().Execute("CREATE TABLE ..."));
> > >   EXPECT_EQ(SQLITE_OK, cb.getError());
> > > }
> > > 
> > > That would keep the callback private inside the ScopedErrorCallback.
> > 
> > Hmm.  You have given me something to think about.  On the one hand, this
makes
> > sense in terms of use cases like you describe above.  On the other hand,
this
> > test is testing that the callback is called, as opposed to whether an error
> > happened.  Having the callback itself embedded in the scoper would result in
a
> > little less API exposed.  Maybe I'll mock it up and look at it.
> 
> I found a new case where error callbacks were broken, which needs different
> callback behavior.  So I'm keeping the scoper separate from the trivial
callback
> for now.

sounds good

> I do think it might be reasonable to shift the generally-useful scoper to a
> specific header/impl file, and just pull the trivial error-capture callback
into
> both unittest files.  WDYT?

Hmm. Sounds a bit meh. :-)

[commit-bot: I haz the power, 2013-07-11 21:33:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/17726002/29002

[commit-bot: I haz the power, 2013-07-12 01:39:02]

Change committed as 211281