Annotate calls to SQLite functions - they have to be executed on a thread allowing IO access.

Annotate calls to SQLite functions - they have to be executed on a thread allowing IO access.

Expand scope of ScopedAllowIO in the only place where SQLite functions are used on UI thread.


Patch from Pavel Ivanov <paivanof@gmail.com>;


BUG=75232, 52909
TEST=no test fails with message "Function marked as IO-only was called from a thread that disallows IO!"

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

[pivanof, 2012-06-26 15:53:57]

ping

[Scott Hess - ex-Googler, 2012-06-26 16:39:47]

Looks like a good idea, though for this one you probably should post a try run,
because I bet it's going to cause issues.  If you can't do that, let me know and
I'll figure out how I should do it.

https://chromiumcodereview.appspot.com/10540155/diff/5001/sql/connection.cc
File sql/connection.cc (right):

https://chromiumcodereview.appspot.com/10540155/diff/5001/sql/connection.cc#n...
sql/connection.cc:79: AssertIOAllowed();
In cases like this, I would prefer the assert move to the top of the function,
so that it's part of the API contract.  Otherwise there is a chance that
unrelated code changes remove a duplicate Close(), and we'll find out that one
caller was calling Close() on the right thread, and the other on the wrong
thread.  Better to knock those all out up front.  Let me know if this didn't
make sense to you and I can elaborate.

Unfortunately, this makes the bright line of "AssertIOAllowed() before any
sqlite3_*() function call" less bright.

[pivanof, 2012-06-26 16:57:55]

> Looks like a good idea, though for this one you probably should post a try
run,
> because I bet it's going to cause issues.  If you can't do that, let me know
and
> I'll figure out how I should do it.

I've checked browser_tests locally and they didn't show any problems. But anyway
I'm not a committer, so I believe I can't do try runs by myself. Could you do
that for me?

> In cases like this, I would prefer the assert move to the top of the function,
> so that it's part of the API contract.

When I prepared this patch I thought that calling AssertIOAllowed() as close to
actual IO as possible (as comment on the method suggests) could be a good idea -
maybe there can be some deliberate usage from wrong thread when it's known that
it shouldn't do actual IO. But now I think I'll agree with you. I guess I should
apply similar change to all other places too, including moving it out of
condition "if (db_)", right?
I'll make that change tonight.

> Unfortunately, this makes the bright line of "AssertIOAllowed() before any
> sqlite3_*() function call" less bright.

I'm not sure I understand what did you mean by this.

[Scott Hess - ex-Googler, 2012-06-26 17:09:18]

On 2012/06/26 16:57:55, pivanof wrote:
> > Looks like a good idea, though for this one you probably should post a try
> run,
> > because I bet it's going to cause issues.  If you can't do that, let me know
> and
> > I'll figure out how I should do it.
> 
> I've checked browser_tests locally and they didn't show any problems. But
anyway
> I'm not a committer, so I believe I can't do try runs by myself. Could you do
> that for me?

OK, I _think_ I've posted a try job.
> Results will be emailed to: paivanof@gmail.com
> Patch 'issue_10540155' sent to try server: win_rel, android, linux_clang,
mac_rel, linux_rel

You should get email about linux_clang relatively quickly (I think it's
compile-only).  I think there's a good bet that you'll flush out a number of
points where people are doing the wrong thing, unfortunately.  Well, fingers
crossed.

> > In cases like this, I would prefer the assert move to the top of the
function,
> > so that it's part of the API contract.
> 
> When I prepared this patch I thought that calling AssertIOAllowed() as close
to
> actual IO as possible (as comment on the method suggests) could be a good idea
-
> maybe there can be some deliberate usage from wrong thread when it's known
that
> it shouldn't do actual IO. But now I think I'll agree with you. I guess I
should
> apply similar change to all other places too, including moving it out of
> condition "if (db_)", right?
> I'll make that change tonight.
>
> > Unfortunately, this makes the bright line of "AssertIOAllowed() before any
> > sqlite3_*() function call" less bright.
> 
> I'm not sure I understand what did you mean by this.

The as-close-to-IO is what I meant.  It's easier to make sure that every
sqlite3_ function call is prefixed with AssertIOAllowed(), as opposed to
separating the assertion by putting it at the top of the function.  But I think
that putting it at the top of the function makes it clearer that the caller
shouldn't call the function from the wrong thread in the first place.

Unfortunately, I think the cases where someone deliberately calls from the wrong
thread and knows what they are doing are very few.  Mostly people call from the
wrong thread without knowing what they're doing!

Thanks,
scott

[pivanof, 2012-06-26 18:02:21]

I see quite a few failures related to the patch, all in
net::DefaultServerBoundCertStore::InitStore().
Now what should I do next? Should I include in the patch temporary ScopedAllowIO
in relevant places and then create issues for its removal?

[Scott Hess - ex-Googler, 2012-06-26 18:06:26]

On 2012/06/26 18:02:21, pivanof wrote:
> I see quite a few failures related to the patch, all in
> net::DefaultServerBoundCertStore::InitStore().
> Now what should I do next? Should I include in the patch temporary
ScopedAllowIO
> in relevant places and then create issues for its removal?

If you put in  ScopedAllowIO, best practice is something like logging a bug
about needing to fix the potentially blocking code, finding some likely
candidates to cc: on the bug, then referencing the bug in a TODO() comment at
the point where the ScopedAllowIO is.

I think having just one place to fix it pretty good results :-).

[pivanof, 2012-06-27 02:57:08]

shess: I've updated placement of AssertIOAllowed. Please review and submit a
try.

erikwright: You are owner of chrome/browser/net, your review is needed.

All the rest: I've considered you the main people from http://crbug.com/52909
and thus you probably would be interested to see a change in
SQLiteServerBoundCertStore::Backend::Load().

[pivanof, 2012-07-02 19:33:03]

ping

[erikwright (departed), 2012-07-10 13:44:04]

Hi,

I'm only a reviewer for cookie related stuff. The cert store guy is mattm.

Cheers,

Erik

[mattm, 2012-07-10 21:51:00]

I just submitted a try run for it.

The sqlite_server_bound_cert_store.cc change seems fine, lets see how the latest
try run goes.

http://codereview.chromium.org/10540155/diff/25001/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_server_bound_cert_store.cc (right):

http://codereview.chromium.org/10540155/diff/25001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_server_bound_cert_store.cc:164: // moved to the DB
thread as part of http://crbug.com/52909.
Updating this to crbug.com/89665 would be better, it's the matching bug for the
cert store.

[pivanof, 2012-07-11 02:56:02]

On 2012/07/10 21:51:00, mattm wrote:
> I just submitted a try run for it.
> 
> The sqlite_server_bound_cert_store.cc change seems fine, lets see how the
latest
> try run goes.

It turns out moving AssertIOAllowed() to the beginning of Close() methods was a
bad idea as they must be called from destructors which almost certainly will be
called on wrong thread.

I've fixed that. Could you please submit another try?


>
http://codereview.chromium.org/10540155/diff/25001/chrome/browser/net/sqlite_...
> File chrome/browser/net/sqlite_server_bound_cert_store.cc (right):
> 
>
http://codereview.chromium.org/10540155/diff/25001/chrome/browser/net/sqlite_...
> chrome/browser/net/sqlite_server_bound_cert_store.cc:164: // moved to the DB
> thread as part of http://crbug.com/52909.
> Updating this to crbug.com/89665 would be better, it's the matching bug for
the
> cert store.

Done.

[Scott Hess - ex-Googler, 2012-07-11 03:33:18]

On 2012/07/11 02:56:02, pivanof wrote:
> On 2012/07/10 21:51:00, mattm wrote:
> > I just submitted a try run for it.
> > 
> > The sqlite_server_bound_cert_store.cc change seems fine, lets see how the
> latest
> > try run goes.
> 
> It turns out moving AssertIOAllowed() to the beginning of Close() methods was
a
> bad idea as they must be called from destructors which almost certainly will
be
> called on wrong thread.

Could you log a bug, cc'ing (at least) me to fix this?  Having the pattern of
calling Close() on the right thread before calling Close() on the wrong thread
as part of destruction is error-prone, as it means that the assertion is based
on dynamic operation of the program, not correct use of the API.  It's possible
that the problem is insoluble, but this one doesn't feel that way.  Either a
scoped_ptr<> could be used to cause destruction on the right thread, or the API
could be changed to make destructing the object without calling Close() an API
violation.

[pivanof, 2012-07-11 03:51:24]

On 2012/07/11 03:33:18, shess wrote:
> On 2012/07/11 02:56:02, pivanof wrote:
> > It turns out moving AssertIOAllowed() to the beginning of Close() methods
was
> a
> > bad idea as they must be called from destructors which almost certainly will
> be
> > called on wrong thread.
> 
> Could you log a bug, cc'ing (at least) me to fix this?  Having the pattern of
> calling Close() on the right thread before calling Close() on the wrong thread
> as part of destruction is error-prone, as it means that the assertion is based
> on dynamic operation of the program, not correct use of the API.  It's
possible
> that the problem is insoluble, but this one doesn't feel that way.  Either a
> scoped_ptr<> could be used to cause destruction on the right thread, or the
API
> could be changed to make destructing the object without calling Close() an API
> violation.

I think I can fix this as part of this patch by splitting Close() into part that
have to be executed in destructor even if connection is already closed and part
that make actual closing. And then destructor will call second part only if
connection is opened.

[Scott Hess - ex-Googler, 2012-07-11 04:25:17]

On 2012/07/11 03:51:24, pivanof wrote:
> On 2012/07/11 03:33:18, shess wrote:
> > On 2012/07/11 02:56:02, pivanof wrote:
> > > It turns out moving AssertIOAllowed() to the beginning of Close() methods
> was
> > a
> > > bad idea as they must be called from destructors which almost certainly
will
> > be
> > > called on wrong thread.
> > 
> > Could you log a bug, cc'ing (at least) me to fix this?  Having the pattern
of
> > calling Close() on the right thread before calling Close() on the wrong
thread
> > as part of destruction is error-prone, as it means that the assertion is
based
> > on dynamic operation of the program, not correct use of the API.  It's
> possible
> > that the problem is insoluble, but this one doesn't feel that way.  Either a
> > scoped_ptr<> could be used to cause destruction on the right thread, or the
> API
> > could be changed to make destructing the object without calling Close() an
API
> > violation.
> 
> I think I can fix this as part of this patch by splitting Close() into part
that
> have to be executed in destructor even if connection is already closed and
part
> that make actual closing. And then destructor will call second part only if
> connection is opened.

I don't think that really fixes the problem.  Really, if the owner of the
database object lives in a different thread from the accesses to that database
object, the object should probably be held with a scoped_ptr<> so that the owner
can schedule it to be destructed in the right thread.  Alternately, the database
object destructor could assert that Close() was already called, which parallels
the construct/Open pairing.

Anyhow, I'm back tomorrow from some leave which has, unfortunately, interrupted
unrelated work I was supposed to be doing, so I'm going to be overwhelmed with
stuff for a week or two - then I'm scheduled to be OOO another week after that. 
I think logging a bug and working around the problem is probably a more sensible
short-term solution, I don't want to tie you up unduly with half-realized
philosophical noodling.

[pivanof, 2012-07-11 04:53:55]

Hm... I thought that if Connection owner called Close() on the correct thread
then it can destroy Connection object on any thread...

But okay, I've created http://crbug.com/136655 (I can't edit owner and labels,
so please adjust accordingly) and put issue link into sources.

[pivanof, 2012-07-12 01:22:04]

So I believe we are okay with this patch as it is, right? If yes then could you
please submit a try and commit it?

[pivanof, 2012-07-18 02:41:48]

erikwright, gbillock: as co-owners of sql/* could you please review this patch?
shess apparently is too busy.

[Scott Hess - ex-Googler, 2012-07-18 03:47:50]

Apologies - your email was "below the fold", which means I'd never see it again
without a ping.  With the TODO and the bug, I'm happy with this, so LGTM and
I'll push the CQ button.

[commit-bot: I haz the power, 2012-07-18 03:48:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paivanof@gmail.com/10540155/37002

[commit-bot: I haz the power, 2012-07-18 03:48:09]

Presubmit check for 10540155-37002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/net

Banned functions were used.
    chrome/browser/net/sqlite_server_bound_cert_store.cc:165:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.

Presubmit checks took 1.1s to calculate.

[Scott Hess - ex-Googler, 2012-07-18 03:49:24]

On 2012/07/18 03:48:09, I haz the power (commit-bot) wrote:
> Presubmit check for 10540155-37002 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/net
> 
> Banned functions were used.
>     chrome/browser/net/sqlite_server_bound_cert_store.cc:165:
>       New code should not use ScopedAllowIO. Post a task to the blocking
>       pool or the FILE thread instead.
> 
> Presubmit checks took 1.1s to calculate.

erikwright is probably an appropriate OWNER for this.  Erik, feel free to press
the CQ button if/when you LGTM.

[If the CQ doesn't work, I can land it tomorrow.]

[erikwright (departed), 2012-07-18 03:58:09]

lgtm

LGTM.

[commit-bot: I haz the power, 2012-07-18 03:58:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paivanof@gmail.com/10540155/37002

[commit-bot: I haz the power, 2012-07-18 03:58:19]

Presubmit check for 10540155-37002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Banned functions were used.
    chrome/browser/net/sqlite_server_bound_cert_store.cc:165:
      New code should not use ScopedAllowIO. Post a task to the blocking
      pool or the FILE thread instead.

Presubmit checks took 1.0s to calculate.

[Scott Hess - ex-Googler, 2012-07-18 04:16:03]

On 2012/07/18 03:58:19, I haz the power (commit-bot) wrote:
> Presubmit check for 10540155-37002 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Banned functions were used.
>     chrome/browser/net/sqlite_server_bound_cert_store.cc:165:
>       New code should not use ScopedAllowIO. Post a task to the blocking
>       pool or the FILE thread instead.
> 
> Presubmit checks took 1.0s to calculate.

I should work on my reading comprehension :-).  This self-evidently is the point
of the TODO, so I'll figure out how to land this tomorrow.

[pivanof, 2012-07-18 10:53:36]

On 2012/07/18 04:16:03, shess wrote:
> On 2012/07/18 03:58:19, I haz the power (commit-bot) wrote:
> > Presubmit check for 10540155-37002 failed and returned exit status 1.
> > 
> > Running presubmit commit checks ...
> > 
> > ** Presubmit ERRORS **
> > Banned functions were used.
> >     chrome/browser/net/sqlite_server_bound_cert_store.cc:165:
> >       New code should not use ScopedAllowIO. Post a task to the blocking
> >       pool or the FILE thread instead.
> > 
> > Presubmit checks took 1.0s to calculate.
> 
> I should work on my reading comprehension :-).  This self-evidently is the
point
> of the TODO, so I'll figure out how to land this tomorrow.

Locally I had to pass --no_presubmit to gcl to be able to upload this patch. But
gcl itself suggested me to do so in the message about presubmit check failure.

[Scott Hess - ex-Googler, 2012-07-18 23:53:13]

Reverted because the page cycler was hitting these assertions.  See
http://crbug.com/137961 .  *sigh*.