Always pass along the GAIA failure event, regardless of the local authenticat...

Always pass along the GAIA failure event, regardless of the local authentication condition.  This is necessary since the UI is waiting for the event, and has no effect on the other listeners.

BUG=19720
TEST=let server creds expire, then login with bad password; should get an error message not a hung UI


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

[John Gregg, 2010-07-13 00:26:50]

This seems like a too-simple fix for this problem, so perhaps I'm overlooking
something, but from studying the code paths that are listening for the
GAIA_AUTH_FAILED event and how they handle it, it seems those effects should
always happen even if there is a fallback to local authentication.  Doing so
also prevents the UI from hanging as in the bug.

[chron_chromium.org, 2010-07-13 00:32:53]

http://codereview.chromium.org/2988002/diff/1/2
File chrome/browser/sync/engine/auth_watcher.cc (right):

http://codereview.chromium.org/2988002/diff/1/2#newcode219
chrome/browser/sync/engine/auth_watcher.cc:219:
AuthenticateLocally(results.email, results.password)) {
This seems fine as long as we test the conditions where this event is thrown,
and everything still works fine. 

i.e. does offline use still work ok? or does about:sync display anything weird?
My guess for anything that might go weird is if we start the browser offline,
fail a gaia attempt, does sync continue to be enabled and track changes?

It looks fine assuming this is true.

[John Gregg, 2010-07-13 00:56:29]

On 2010/07/13 00:32:53, chron_chromium.org wrote:
> http://codereview.chromium.org/2988002/diff/1/2
> File chrome/browser/sync/engine/auth_watcher.cc (right):
> 
> http://codereview.chromium.org/2988002/diff/1/2#newcode219
> chrome/browser/sync/engine/auth_watcher.cc:219:
> AuthenticateLocally(results.email, results.password)) {
> This seems fine as long as we test the conditions where this event is thrown,
> and everything still works fine. 
> 
> i.e. does offline use still work ok? or does about:sync display anything
weird?
> My guess for anything that might go weird is if we start the browser offline,
> fail a gaia attempt, does sync continue to be enabled and track changes?
> 

This seems to work for me.  I did:
- disconnect the server
- fail the login
- add some bookmarks
- reconnect the server
- login successfully
- the new bookmarks appear on the server

[chron_chromium.org, 2010-07-13 01:07:05]

LGTM

On Mon, Jul 12, 2010 at 5:56 PM, <johnnyg@chromium.org> wrote:

> On 2010/07/13 00:32:53, chron_chromium.org wrote:
>
>> http://codereview.chromium.org/2988002/diff/1/2
>> File chrome/browser/sync/engine/auth_watcher.cc (right):
>>
>
>  http://codereview.chromium.org/2988002/diff/1/2#newcode219
>> chrome/browser/sync/engine/auth_watcher.cc:219:
>> AuthenticateLocally(results.email, results.password)) {
>> This seems fine as long as we test the conditions where this event is
>> thrown,
>> and everything still works fine.
>>
>
>  i.e. does offline use still work ok? or does about:sync display anything
>>
> weird?
>
>> My guess for anything that might go weird is if we start the browser
>> offline,
>> fail a gaia attempt, does sync continue to be enabled and track changes?
>>
>
>
> This seems to work for me.  I did:
> - disconnect the server
> - fail the login
> - add some bookmarks
> - reconnect the server
> - login successfully
> - the new bookmarks appear on the server
>
>
>
> http://codereview.chromium.org/2988002/show
>

[ncarter (slow), 2010-07-13 01:08:12]

It looks fine (at least under the standard of "better than what was there
before.") .

Do we see different error strings in the "bad password" and "server
unreachable" cases?

 - nick

On Mon, Jul 12, 2010 at 5:56 PM, <johnnyg@chromium.org> wrote:

> On 2010/07/13 00:32:53, chron_chromium.org wrote:
>
>> http://codereview.chromium.org/2988002/diff/1/2
>> File chrome/browser/sync/engine/auth_watcher.cc (right):
>>
>
>  http://codereview.chromium.org/2988002/diff/1/2#newcode219
>> chrome/browser/sync/engine/auth_watcher.cc:219:
>> AuthenticateLocally(results.email, results.password)) {
>> This seems fine as long as we test the conditions where this event is
>> thrown,
>> and everything still works fine.
>>
>
>  i.e. does offline use still work ok? or does about:sync display anything
>>
> weird?
>
>> My guess for anything that might go weird is if we start the browser
>> offline,
>> fail a gaia attempt, does sync continue to be enabled and track changes?
>>
>
>
> This seems to work for me.  I did:
> - disconnect the server
> - fail the login
> - add some bookmarks
> - reconnect the server
> - login successfully
> - the new bookmarks appear on the server
>
>
>
> http://codereview.chromium.org/2988002/show
>

[John Gregg, 2010-07-13 01:34:06]

On 2010/07/13 01:08:12, ncarter wrote:
> It looks fine (at least under the standard of "better than what was there
> before.") .

Yeah I know your original patch for this bug was more ambitious, but this
improves things just enough to fix the bug :)

> Do we see different error strings in the "bad password" and "server
> unreachable" cases?

Sorry, my steps were out of order.  I failed the login to exercise the codepath,
then disconnected the server.  You can't actually have a GAIA authentication
fail while disconnected, as far as i can tell, since you need the server to send
the failure.

If the server is unreachable when you try to submit credentials, it spins
forever on the login prompt (but allows you to cancel).