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.

Also expanded scope of ScopedAllowIO in
SQLiteServerBoundCertStore::Backend::Load() to cover SQLite functions.
And added ScopedAllowIO to PasswordStoreFactory::BuildServiceInstanceFor() --
it calls LoginDatabase::Init() which should be executed on DB thread.

This is a reland of
https://src.chromium.org/viewvc/chrome?view=rev&revision=147309
which was reverted because of missing ScopedAllowIO in PasswordStoreFactory.


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


BUG=75232, 52909, 137961, 138903
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=148788

[pivanof, 2012-07-25 04:58:15]

shess: please triage and CC http://crbug.com/138903 appropriately. And you'll
need to commit this manually again.

erikwright: look at chrome/browser/net/*.

mdm: look at chrome/browser/password_manager/*.

[Mike Mammarella, 2012-07-25 21:18:24]

PasswordStoreFactory change LGTM

On 2012/07/25 04:58:15, pivanof wrote:
> shess: please triage and CC http://crbug.com/138903 appropriately. And you'll
> need to commit this manually again.
> 
> erikwright: look at chrome/browser/net/*.
> 
> mdm: look at chrome/browser/password_manager/*.

[pivanof, 2012-07-25 21:25:53]

mdm: I've just realized that you are probably a better person to triage
http://crbug.com/138903 appropriately. Could you do that please?

[Scott Hess - ex-Googler, 2012-07-27 00:54:37]

I have a client all set up for landing this manually tomorrow.  I double-checked
the diffs from the earlier CL, and all that's new is what Mike checked off on,
so I don't think Erik needs to additionally LGTM again for chrome/browser/net
OWNERS, unless he wishes to.  I have to force-land anyhow because of the
presubmit check forbidding new ScopedAllowIO...

[pivanof, 2012-07-27 01:09:25]

On 2012/07/27 00:54:37, shess wrote:
> I double-checked
> the diffs from the earlier CL, and all that's new is what Mike checked off on,
> so I don't think Erik needs to additionally LGTM again for chrome/browser/net

BTW, what should be the usual procedure in such cases? Did I need maybe to
revive the initial CL, so that there are already all approvals of the code that
wasn't changed? I don't know if it's possible though and what will gcl tell me
if I try to do that.

[Scott Hess - ex-Googler, 2012-07-27 20:01:32]

I've landed this, but I seem to recall that last time it took a few hours before
it failed.  I looped in a sheriff, in case they notice problems before me.

On 2012/07/27 01:09:25, pivanof wrote:
> On 2012/07/27 00:54:37, shess wrote:
> > I double-checked
> > the diffs from the earlier CL, and all that's new is what Mike checked off
on,
> > so I don't think Erik needs to additionally LGTM again for
chrome/browser/net
> 
> BTW, what should be the usual procedure in such cases? Did I need maybe to
> revive the initial CL, so that there are already all approvals of the code
that
> wasn't changed? I don't know if it's possible though and what will gcl tell me
> if I try to do that.

I've gone either way with this.  Technically, it's no problem to re-open an old
CL which was reverted.  Then you can update it and move on.  You might have to
mess with .git/config , I don't recall.

Culturally, I think it's mixed.  I think in this case, it would have been
reasonable, because it was a very minor addition with obvious reasoning.  But if
you needed to make more major changes, like moving code around or something, I'd
be likely to just make a new CL and refer to the original in the description or
something.  Or if it's likely to provoke discussion, better to start with a
clean-ish slate.

Also, a very useful approach with a new CL is to first post the original version
as a new CL, then post your updates, to make it clear what you needed to change.
 That way you can (hopefully) not re-open everything to re-review :-).

[Mike Mammarella, 2012-07-27 20:07:42]

Sorry for the delay. I've marked the new bug as fixed.

Previously I've heard very strong objections to reusing the existing code review
- in general you should always start a new one. But yes, having the first patch
set be the old code, and then changing it in subsequent patch sets, makes it
much easier to see the revisions made to address why it was reverted the first
time.

[pivanof, 2012-07-27 20:12:59]

On 2012/07/27 20:07:42, Mike Mammarella wrote:
> Sorry for the delay. I've marked the new bug as fixed.

No, it's not fixed. It should be fixed only when ScopedAllowIO (added here) is
removed. I've asked to triage it meaning to set appropriate status, CCs and
maybe owner who have enough knowledge to fix it later. Also I think it should be
blocking http://crbug.com/119550.