Re-land the password store work from bug 8205, with changes that should fix b...

Re-land the password store work from bug 8205, with changes that should fix bug 12479. The Linux pieces are still disabled, however.

BUG=8205
TEST=Password autofill should continue to work on Windows.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=17273

[stuartmorgan, 2009-05-28 22:35:56]

This is just a reversal of the backout in http://codereview.chromium.org/113871,
with changes to the Windows bits, so the profile, password_manager, and linux
pieces don't need review (having been reviewed in
http://codereview.chromium.org/113189).

The reliability test crash that caused the backout appears to have been the race
condition you were concerned about with the WebDataService callbacks; despite
the longevity of the PasswordStore it could still happen during exit teardown,
so the reliability tests were very sensitive to it.

Cancelling the requests doesn't seem to be feasible, since the destructor would
not be running on the same thread as the WebDataService callbacks, so there's a
very tricky race around when we would run the destructor vs a callback
happening. I couldn't see a foolproof way to handle that, so instead I made sure
that the password store is retained until the callback occurs (and completes). I
also added a check to make sure we don't kick of IE7 password requests to the
WebDataService once it's shut down.

[tim (not reviewing), 2009-05-29 00:07:19]

Hm.  It's unfortunate this is all because of an unnecessary dispatch across the
PasswordStore thread; the WebDataService already provides an asynchronous API so
it's safe to call from the ui loop, but here we first dispatch to an
intermediate.  But that aside, could you use ReleaseSoon from the ui loop to
delete the store from the same thread as the callbacks, so it could then easily
cancel the outstanding request before being destroyed?

On 2009/05/28 22:35:56, stuartmorgan wrote:
> This is just a reversal of the backout in
http://codereview.chromium.org/113871,
> with changes to the Windows bits, so the profile, password_manager, and linux
> pieces don't need review (having been reviewed in
> http://codereview.chromium.org/113189).
> 
> The reliability test crash that caused the backout appears to have been the
race
> condition you were concerned about with the WebDataService callbacks; despite
> the longevity of the PasswordStore it could still happen during exit teardown,
> so the reliability tests were very sensitive to it.
> 
> Cancelling the requests doesn't seem to be feasible, since the destructor
would
> not be running on the same thread as the WebDataService callbacks, so there's
a
> very tricky race around when we would run the destructor vs a callback
> happening. I couldn't see a foolproof way to handle that, so instead I made
sure
> that the password store is retained until the callback occurs (and completes).
I
> also added a check to make sure we don't kick of IE7 password requests to the
> WebDataService once it's shut down.

[tim (not reviewing), 2009-05-29 00:17:59]

On 2009/05/29 00:07:19, timsteele wrote:
> Hm.  It's unfortunate this is all because of an unnecessary dispatch across
the
> PasswordStore thread; the WebDataService already provides an asynchronous API
so
> it's safe to call from the ui loop, but here we first dispatch to an
> intermediate.  But that aside, could you use ReleaseSoon from the ui loop to
> delete the store from the same thread as the callbacks, so it could then
easily
> cancel the outstanding request before being destroyed?
> 
> On 2009/05/28 22:35:56, stuartmorgan wrote:
> > This is just a reversal of the backout in
> http://codereview.chromium.org/113871,
> > with changes to the Windows bits, so the profile, password_manager, and
linux
> > pieces don't need review (having been reviewed in
> > http://codereview.chromium.org/113189).
> > 
> > The reliability test crash that caused the backout appears to have been the
> race
> > condition you were concerned about with the WebDataService callbacks;
despite
> > the longevity of the PasswordStore it could still happen during exit
teardown,
> > so the reliability tests were very sensitive to it.
> > 
> > Cancelling the requests doesn't seem to be feasible, since the destructor
> would
> > not be running on the same thread as the WebDataService callbacks, so
there's
> a
> > very tricky race around when we would run the destructor vs a callback
> > happening. I couldn't see a foolproof way to handle that, so instead I made
> sure
> > that the password store is retained until the callback occurs (and
completes).
> I
> > also added a check to make sure we don't kick of IE7 password requests to
the
> > WebDataService once it's shut down.

Also, I think you're missing password_store.{h,cc}

[stuartmorgan, 2009-05-29 00:25:46]

On 2009/05/29 00:07:19, timsteele wrote:
> Hm.  It's unfortunate this is all because of an unnecessary dispatch across
the
> PasswordStore thread; the WebDataService already provides an asynchronous API
so
> it's safe to call from the ui loop, but here we first dispatch to an
> intermediate.

Agreed, but I'm assuming this is a temporary shim--the fact that password
storage is in WebDataService is entirely a Windows-ism at this point--and that
once PasswordStore is fully fleshed out someone will rewrite the Windows side of
this to have its own internal mini-database so the WDS password stuff can go
away. (I'd rather not be that someone, since my Windows dev environment is
painful, but I'm willing to be if necessary.)

> But that aside, could you use ReleaseSoon from the ui loop to
> delete the store from the same thread as the callbacks, so it could then
easily
> cancel the outstanding request before being destroyed?

Hm... only if I muck with the PasswordStore interface, it looks like; I'm not
sure we want to change the API to do something that's only needed by the weird
temporary shim layer.

[stuartmorgan, 2009-05-29 00:29:41]

On 2009/05/29 00:17:59, timsteele wrote:
> Also, I think you're missing password_store.{h,cc}

Oops! Fixed.

[tim (not reviewing), 2009-05-29 20:45:54]

OK, sorry for delay.  I'm okay with it as is, for a temporary shim layer.

http://codereview.chromium.org/114057/diff/2010/2016
File chrome/browser/password_manager/password_store.cc (right):

http://codereview.chromium.org/114057/diff/2010/2016#newcode61
Line 61: scoped_ptr<GetLoginsRequest> request_ptr(request);
could you add a DCHECK in here that the current message loop is the owned
threads' message loop?

http://codereview.chromium.org/114057/diff/2010/2017
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/114057/diff/2010/2017#newcode109
Line 109: Lock pending_requests_lock_;
I may be missing something, but I don't see what the lock is needed for here
because it is only accessed from the consumer loop which afaik should always be
the main ui loop.  I suppose you could use it in NotifyConsumer to short-circuit
if the request as been cancelled (and void posting the task), but if there is no
other use of the lock I'd rather nuke it if we can.

[stuartmorgan, 2009-05-29 21:29:12]

John, can you weigh in on the lock question?

http://codereview.chromium.org/114057/diff/2010/2016
File chrome/browser/password_manager/password_store.cc (right):

http://codereview.chromium.org/114057/diff/2010/2016#newcode61
Line 61: scoped_ptr<GetLoginsRequest> request_ptr(request);
On 2009/05/29 20:45:56, timsteele wrote:
> could you add a DCHECK in here that the current message loop is the owned
> threads' message loop?

Done.

http://codereview.chromium.org/114057/diff/2010/2017
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/114057/diff/2010/2017#newcode109
Line 109: Lock pending_requests_lock_;
On 2009/05/29 20:45:56, timsteele wrote:
> I may be missing something, but I don't see what the lock is needed for here
> because it is only accessed from the consumer loop which afaik should always
be
> the main ui loop.  I suppose you could use it in NotifyConsumer to
short-circuit
> if the request as been cancelled (and void posting the task), but if there is
no
> other use of the lock I'd rather nuke it if we can.

I haven't changed PasswordStore at all from the original patch, so I don't know
the reasoning here. Perhaps CancelLoginsQuery is intended to be threadsafe?

[davidsansome, 2009-05-29 21:43:25]

http://codereview.chromium.org/114057/diff/2010/2017
File chrome/browser/password_manager/password_store.h (right):

http://codereview.chromium.org/114057/diff/2010/2017#newcode109
Line 109: Lock pending_requests_lock_;
On 2009/05/29 21:29:12, stuartmorgan wrote:
> On 2009/05/29 20:45:56, timsteele wrote:
> > I may be missing something, but I don't see what the lock is needed for here
> > because it is only accessed from the consumer loop which afaik should always
> be
> > the main ui loop.  I suppose you could use it in NotifyConsumer to
> short-circuit
> > if the request as been cancelled (and void posting the task), but if there
is
> no
> > other use of the lock I'd rather nuke it if we can.
> 
> I haven't changed PasswordStore at all from the original patch, so I don't
know
> the reasoning here. Perhaps CancelLoginsQuery is intended to be threadsafe?

I wrote this bit of John's original patch.

I was anticipating that multiple threads might use the PasswordStore, but if
it's only the one UI thread then it should be fine to remove the lock.

[tim (not reviewing), 2009-05-29 21:46:24]

yay! less locking :) Stuart, if you don't want to do it right now you can add
TODOs, I won't force you, since it isn't really your fight :)  I can do it in a
follow up.

This LGTM otherwise.

On 2009/05/29 21:43:25, davidsansome wrote:
> http://codereview.chromium.org/114057/diff/2010/2017
> File chrome/browser/password_manager/password_store.h (right):
> 
> http://codereview.chromium.org/114057/diff/2010/2017#newcode109
> Line 109: Lock pending_requests_lock_;
> On 2009/05/29 21:29:12, stuartmorgan wrote:
> > On 2009/05/29 20:45:56, timsteele wrote:
> > > I may be missing something, but I don't see what the lock is needed for
here
> > > because it is only accessed from the consumer loop which afaik should
always
> > be
> > > the main ui loop.  I suppose you could use it in NotifyConsumer to
> > short-circuit
> > > if the request as been cancelled (and void posting the task), but if there
> is
> > no
> > > other use of the lock I'd rather nuke it if we can.
> > 
> > I haven't changed PasswordStore at all from the original patch, so I don't
> know
> > the reasoning here. Perhaps CancelLoginsQuery is intended to be threadsafe?
> 
> I wrote this bit of John's original patch.
> 
> I was anticipating that multiple threads might use the PasswordStore, but if
> it's only the one UI thread then it should be fine to remove the lock.

[stuartmorgan, 2009-05-29 22:18:40]

Done, and added a comment to the class doc to make it explicit that it's not
threadsafe. (handle_ was incremented outside the lock, so I don't think it was
actually completely threadsafe anyway).

I also added a TODO comment to the shim class explaining that it is not intended
to be a permanent solution.

[tim (not reviewing), 2009-05-29 22:27:28]

thanks! LGTM