[Chrome OS] Attempt offline and online login simultaneously

[Chrome OS] Attempt offline and online login simultaneously

This feature is behind the switch "--parallel-auth"

BUG=chromium-os:4929
TEST=unit testss

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

[Chris Masone, 2010-10-06 07:02:06]

On 2010/10/05 16:41:34, Chris Masone wrote:
> 

Tried this out today on my device, and offline: NO -> online: OK; offline: OK ->
online: canceled; and offline: NO -> online: NO all worked as expected.  

I think this change is ready for review now...

[Nikita (slow), 2010-10-06 15:03:20]

Patched this CL, got crash on case of changed password.
Entered NEW password, in the selected dialog selected 1st option (sync all).


#0  DebugUtil::BreakDebugger () at base/debug_util_posix.cc:268
#1  0x6b8ea1ee in logging::LogMessage::~LogMessage (this=0x77f8d160) at
base/logging.cc:656
#2  0x6a801d9d in chromeos::ParallelAuthenticator::ResyncEncryptedData
(this=0x70bbacb0, credentials=...)
    at chrome/browser/chromeos/login/parallel_authenticator.cc:229
#3  0x6af1bf79 in DispatchToMethod<chromeos::Authenticator, void
(chromeos::Authenticator::*)(GaiaAuthConsumer::ClientLoginResult const&),
GaiaAuthConsumer::ClientLoginResult> (obj=0x70bbacb0, method=..., arg=...) at
./base/tuple.h:547
#4  0x6af1ba15 in RunnableMethod<chromeos::Authenticator, void
(chromeos::Authenticator::*)(GaiaAuthConsumer::ClientLoginResult const&),
Tuple1<GaiaAuthConsumer::ClientLoginResult> >::Run (this=0x70b23cc8) at
./base/task.h:330
#5  0x6b8ed9b2 in MessageLoop::RunTask (this=0x708f6e88, task=0x70b23cc8) at
base/message_loop.cc:410
#6  0x6b8edabb in MessageLoop::DeferOrRunPendingTask (this=0x708f6e88,
pending_task=...) at base/message_loop.cc:419
#7  0x6b8ee0bd in MessageLoop::DoWork (this=0x708f6e88) at
base/message_loop.cc:526
#8  0x6b971960 in base::MessagePumpForUI::RunWithDispatcher (this=0x708f6f80,
delegate=0x708f6e88, dispatcher=0x77f8d758)
    at base/message_pump_glib.cc:215
#9  0x6b8ecdfb in MessageLoop::RunInternal (this=0x708f6e88) at
base/message_loop.cc:253
#10 0x6b8ecc9f in MessageLoop::RunHandler (this=0x708f6e88) at
base/message_loop.cc:230
#11 0x6b8ee928 in MessageLoopForUI::Run (this=0x708f6e88, dispatcher=0x77f8d758)
at base/message_loop.cc:656
#12 0x6a791c30 in (anonymous namespace)::RunUIMessageLoop
(browser_process=0x708f89c8) at chrome/browser/browser_main.cc:469
#13 0x6a795196 in BrowserMain (parameters=...) at
chrome/browser/browser_main.cc:1467
#14 0x6a61f489 in ChromeMain (argc=18, argv=0x77f8ea24) at
chrome/app/chrome_dll_main.cc:947
#15 0x6a6201e2 in main (argc=1781272576, argv=0x0) at
chrome/app/chrome_exe_main_gtk.cc:49

http://codereview.chromium.org/3442009/diff/18001/19009
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/3442009/diff/18001/19009#newcode33
chrome/browser/chromeos/login/parallel_authenticator.h:33: class
GaiaAuthenticator2;
nit: not sorted

http://codereview.chromium.org/3442009/diff/18001/19009#newcode50
chrome/browser/chromeos/login/parallel_authenticator.h:50: CREATE_NEW,
Please add comments for non-obvious states at this one.

http://codereview.chromium.org/3442009/diff/18001/19009#newcode92
chrome/browser/chromeos/login/parallel_authenticator.h:92: void
set_attempt_state(TestAttemptState* new_state) {  // takes ownership.
Could it be private with test class marked as friend?

[Nikita (slow), 2010-10-06 15:05:57]

crash in the same case, but selecting 2nd option (migrate), enter correct old
password


#0  DebugUtil::BreakDebugger () at base/debug_util_posix.cc:268
#1  0x6b9bd1ee in logging::LogMessage::~LogMessage (this=0x77ff4b20) at
base/logging.cc:656
#2  0x6a8d4b7c in chromeos::ParallelAuthenticator::RecoverEncryptedData
(this=0x7148c220, old_password=..., credentials=...)
    at chrome/browser/chromeos/login/parallel_authenticator.cc:215
#3  0x6afef04c in DispatchToMethod<chromeos::Authenticator, void
(chromeos::Authenticator::*)(std::string const&,
GaiaAuthConsumer::ClientLoginResult const&), std::string,
GaiaAuthConsumer::ClientLoginResult> (obj=0x7148c220, method=..., arg=...) at
./base/tuple.h:554
#4  0x6afeeb89 in RunnableMethod<chromeos::Authenticator, void
(chromeos::Authenticator::*)(std::string const&,
GaiaAuthConsumer::ClientLoginResult const&), Tuple2<std::string,
GaiaAuthConsumer::ClientLoginResult> >::Run (this=0x7149e448) at
./base/task.h:330
#5  0x6b9c09b2 in MessageLoop::RunTask (this=0x71096e88, task=0x7149e448) at
base/message_loop.cc:410
#6  0x6b9c0abb in MessageLoop::DeferOrRunPendingTask (this=0x71096e88,
pending_task=...) at base/message_loop.cc:419
#7  0x6b9c10bd in MessageLoop::DoWork (this=0x71096e88) at
base/message_loop.cc:526
#8  0x6ba44960 in base::MessagePumpForUI::RunWithDispatcher (this=0x71096f80,
delegate=0x71096e88, dispatcher=0x77ff5118)
    at base/message_pump_glib.cc:215
#9  0x6b9bfdfb in MessageLoop::RunInternal (this=0x71096e88) at
base/message_loop.cc:253
#10 0x6b9bfc9f in MessageLoop::RunHandler (this=0x71096e88) at
base/message_loop.cc:230
#11 0x6b9c1928 in MessageLoopForUI::Run (this=0x71096e88, dispatcher=0x77ff5118)
at base/message_loop.cc:656
#12 0x6a864c30 in (anonymous namespace)::RunUIMessageLoop
(browser_process=0x710989c8) at chrome/browser/browser_main.cc:469
#13 0x6a868196 in BrowserMain (parameters=...) at
chrome/browser/browser_main.cc:1467
#14 0x6a6f2489 in ChromeMain (argc=18, argv=0x77ff63e4) at
chrome/app/chrome_dll_main.cc:947
#15 0x6a6f31e2 in main (argc=1782136832, argv=0x0) at
chrome/app/chrome_exe_main_gtk.cc:49

[Chris Masone, 2010-10-06 15:19:34]

Those are both CHECK failures, because Resync and Recover want to be called
on the IO thread.  I can turn those into wrappers to enable them to be
called on the UI thread, if you like.

On Wed, Oct 6, 2010 at 8:05 AM, <nkostylev@chromium.org> wrote:

> crash in the same case, but selecting 2nd option (migrate), enter correct
> old
> password
>
>
>
> #0  DebugUtil::BreakDebugger () at base/debug_util_posix.cc:268
> #1  0x6b9bd1ee in logging::LogMessage::~LogMessage (this=0x77ff4b20) at
> base/logging.cc:656
> #2  0x6a8d4b7c in chromeos::ParallelAuthenticator::RecoverEncryptedData
> (this=0x7148c220, old_password=..., credentials=...)
>    at chrome/browser/chromeos/login/parallel_authenticator.cc:215
> #3  0x6afef04c in DispatchToMethod<chromeos::Authenticator, void
> (chromeos::Authenticator::*)(std::string const&,
> GaiaAuthConsumer::ClientLoginResult const&), std::string,
> GaiaAuthConsumer::ClientLoginResult> (obj=0x7148c220, method=..., arg=...)
> at
> ./base/tuple.h:554
> #4  0x6afeeb89 in RunnableMethod<chromeos::Authenticator, void
> (chromeos::Authenticator::*)(std::string const&,
> GaiaAuthConsumer::ClientLoginResult const&), Tuple2<std::string,
> GaiaAuthConsumer::ClientLoginResult> >::Run (this=0x7149e448) at
> ./base/task.h:330
> #5  0x6b9c09b2 in MessageLoop::RunTask (this=0x71096e88, task=0x7149e448)
> at
>
> base/message_loop.cc:410
> #6  0x6b9c0abb in MessageLoop::DeferOrRunPendingTask (this=0x71096e88,
>
> pending_task=...) at base/message_loop.cc:419
> #7  0x6b9c10bd in MessageLoop::DoWork (this=0x71096e88) at
>
> base/message_loop.cc:526
> #8  0x6ba44960 in base::MessagePumpForUI::RunWithDispatcher
> (this=0x71096f80,
> delegate=0x71096e88, dispatcher=0x77ff5118)
>
>    at base/message_pump_glib.cc:215
> #9  0x6b9bfdfb in MessageLoop::RunInternal (this=0x71096e88) at
>
> base/message_loop.cc:253
> #10 0x6b9bfc9f in MessageLoop::RunHandler (this=0x71096e88) at
>
> base/message_loop.cc:230
> #11 0x6b9c1928 in MessageLoopForUI::Run (this=0x71096e88,
> dispatcher=0x77ff5118)
>
> at base/message_loop.cc:656
> #12 0x6a864c30 in (anonymous namespace)::RunUIMessageLoop
> (browser_process=0x710989c8) at chrome/browser/browser_main.cc:469
> #13 0x6a868196 in BrowserMain (parameters=...) at
>
> chrome/browser/browser_main.cc:1467
> #14 0x6a6f2489 in ChromeMain (argc=18, argv=0x77ff63e4) at
>
> chrome/app/chrome_dll_main.cc:947
> #15 0x6a6f31e2 in main (argc=1782136832, argv=0x0) at
>
> chrome/app/chrome_exe_main_gtk.cc:49
>
>
> http://codereview.chromium.org/3442009/show
>

[Nikita (slow), 2010-10-06 15:21:56]

6 октября 2010 г. 19:19 пользователь Chris Masone <cmasone@chromium.org>написал:

> Those are both CHECK failures, because Resync and Recover want to be called
> on the IO thread.  I can turn those into wrappers to enable them to be
> called on the UI thread, if you like.


Would be useful. Were the same DCHECKs firing in GoogleAuthenticator?

-- 
Nikita

[Chris Masone, 2010-10-06 15:23:37]

On 2010/10/06 15:21:56, Nikita Kostylev wrote:
> 6 октября 2010 г. 19:19 пользователь Chris Masone
<cmasone@chromium.org>написал:
> 
> > Those are both CHECK failures, because Resync and Recover want to be called
> > on the IO thread.  I can turn those into wrappers to enable them to be
> > called on the UI thread, if you like.
> 
> 
> Would be useful. Were the same DCHECKs firing in GoogleAuthenticator?
> 
> -- 
> Nikita
> 

They were not.  So I'll change it so you can call them on the UI thread safely.

[Chris Masone, 2010-10-06 21:01:18]

I have also enable Resync and Recover to be called on the UI thread.

http://codereview.chromium.org/3442009/diff/18001/19009
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/3442009/diff/18001/19009#newcode33
chrome/browser/chromeos/login/parallel_authenticator.h:33: class
GaiaAuthenticator2;
On 2010/10/06 15:03:20, Nikita Kostylev wrote:
> nit: not sorted

Done.

http://codereview.chromium.org/3442009/diff/18001/19009#newcode50
chrome/browser/chromeos/login/parallel_authenticator.h:50: CREATE_NEW,
On 2010/10/06 15:03:20, Nikita Kostylev wrote:
> Please add comments for non-obvious states at this one.

Done.

http://codereview.chromium.org/3442009/diff/18001/19009#newcode92
chrome/browser/chromeos/login/parallel_authenticator.h:92: void
set_attempt_state(TestAttemptState* new_state) {  // takes ownership.
On 2010/10/06 15:03:20, Nikita Kostylev wrote:
> Could it be private with test class marked as friend?

Done.

[Nikita (slow), 2010-10-06 22:12:14]

http://codereview.chromium.org/3442009/diff/27001/28006
File chrome/browser/chromeos/login/mock_url_fetchers.cc (right):

http://codereview.chromium.org/3442009/diff/27001/28006#newcode12
chrome/browser/chromeos/login/mock_url_fetchers.cc:12: #include
"chrome/common/net/http_return.h"
nit: alpha sort

http://codereview.chromium.org/3442009/diff/27001/28006#newcode77
chrome/browser/chromeos/login/mock_url_fetchers.cc:77: LOG(INFO) << url_.spec();
Is LOG needed for mock fetcher?

http://codereview.chromium.org/3442009/diff/27001/28006#newcode92
chrome/browser/chromeos/login/mock_url_fetchers.cc:92: const GURL& url,
indent

http://codereview.chromium.org/3442009/diff/27001/28006#newcode98
chrome/browser/chromeos/login/mock_url_fetchers.cc:98: LOG(INFO) << url_.spec();
same here

http://codereview.chromium.org/3442009/diff/27001/28007
File chrome/browser/chromeos/login/mock_url_fetchers.h (right):

http://codereview.chromium.org/3442009/diff/27001/28007#newcode58
chrome/browser/chromeos/login/mock_url_fetchers.h:58: const GURL& url,
indent

http://codereview.chromium.org/3442009/diff/27001/28009
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/3442009/diff/27001/28009#newcode1
chrome/browser/chromeos/login/parallel_authenticator.h:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
BTW, in OnlineAttempt

 if (error.state() == GoogleServiceAuthError::TWO_FACTOR) {
    LOG(WARNING) << "Two factor authenticated. Sync will not work.";
    TriggerResolve(GaiaAuthConsumer::ClientLoginResult(),
                   LoginFailure::None());

Should we still pass it as LoginFailure::NETWORK_AUTH_FAILED?

http://codereview.chromium.org/3442009/diff/27001/28009#newcode26
chrome/browser/chromeos/login/parallel_authenticator.h:26: // Authenticates a
Chromium OS user against the Google Accounts ClientLogin API.
Please put these comments before class.

It would be also helpful to shortly describe main parts of big picture like
- OnlineAttempt calls PA Resolve

http://codereview.chromium.org/3442009/diff/27001/28009#newcode38
chrome/browser/chromeos/login/parallel_authenticator.h:38: class
ParallelAuthenticatorTest;
nit: alpha sort

http://codereview.chromium.org/3442009/diff/27001/28009#newcode67
chrome/browser/chromeos/login/parallel_authenticator.h:67: //
consumer_->OnLoginSuccess() with the |username| and a vector of
doesn't mention new consumer call protocol (OnLoginSuccess(request_pending =
true) / OnLoginSuccess(request_pending = false) etc.

http://codereview.chromium.org/3442009/diff/27001/28009#newcode100
chrome/browser/chromeos/login/parallel_authenticator.h:100: // Call these
methods on the IO thread.
Not a restriction anymore.

[Nikita (slow), 2010-10-06 23:28:27]

http://codereview.chromium.org/3442009/diff/27001/28008
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/3442009/diff/27001/28008#newcode82
chrome/browser/chromeos/login/parallel_authenticator.cc:82: false /* don't
create */);
Don't create? If it's a new user?
Ok, seems to be taken care of @ 456

http://codereview.chromium.org/3442009/diff/27001/28008#newcode131
chrome/browser/chromeos/login/parallel_authenticator.cc:131: LOG(INFO) <<
"Online login success";
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));

http://codereview.chromium.org/3442009/diff/27001/28008#newcode148
chrome/browser/chromeos/login/parallel_authenticator.cc:148: // Send
notification of success
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));

http://codereview.chromium.org/3442009/diff/27001/28008#newcode246
chrome/browser/chromeos/login/parallel_authenticator.cc:246: void
ParallelAuthenticator::RetryAuth(Profile* profile,
Couldn't find when RetryAuth get called?

http://codereview.chromium.org/3442009/diff/27001/28008#newcode273
chrome/browser/chromeos/login/parallel_authenticator.cc:273: break;
Comment?
One of temporary states, waiting for the results?

http://codereview.chromium.org/3442009/diff/27001/28008#newcode320
chrome/browser/chromeos/login/parallel_authenticator.cc:320: if
(!already_reported_success_) {
Is it for the case when we receive online auth result first (Failed) and then
offline (OK)?

http://codereview.chromium.org/3442009/diff/27001/28008#newcode448
chrome/browser/chromeos/login/parallel_authenticator.cc:448: return
LOGIN_FAILED;
For a failed unlock returning LOGIN_FAILED but in line 365 it'll pass
current_state_->online_outcome().
Online login doesn't happen go unlock, is it ok to pass this param?

http://codereview.chromium.org/3442009/diff/27001/28008#newcode450
chrome/browser/chromeos/login/parallel_authenticator.cc:450:
chromeos::kCryptohomeMountErrorKeyFailure) {
nit: indent with conditional

http://codereview.chromium.org/3442009/diff/27001/28008#newcode490
chrome/browser/chromeos/login/parallel_authenticator.cc:490: return NEED_NEW_PW;
NEED_NEW_PW is misleading. It'll report LoginFailure. In case of
INVALID_GAIA_CREDENTIALS - is password change.

In case of other error codes - it's not a password change case.
Like CAPTCHA_REQUIRED, ACCOUNT_DELETED.

http://codereview.chromium.org/3442009/diff/27001/28009
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/3442009/diff/27001/28009#newcode41
chrome/browser/chromeos/login/parallel_authenticator.h:41: class
ParallelAuthenticator : public Authenticator,
We intend to keep only ParallelAuthenticator after it's tested, right?
Otherwise common methods need to be moved to Authenticator (TODO).

http://codereview.chromium.org/3442009/diff/27001/28009#newcode44
chrome/browser/chromeos/login/parallel_authenticator.h:44: enum AuthState {
Do you have nice state graph drawn somewhere to make sure that all possible
transitions are covered?

[Chris Masone, 2010-10-07 01:19:05]

http://codereview.chromium.org/3442009/diff/27001/28006
File chrome/browser/chromeos/login/mock_url_fetchers.cc (right):

http://codereview.chromium.org/3442009/diff/27001/28006#newcode12
chrome/browser/chromeos/login/mock_url_fetchers.cc:12: #include
"chrome/common/net/http_return.h"
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> nit: alpha sort

Done.

http://codereview.chromium.org/3442009/diff/27001/28006#newcode77
chrome/browser/chromeos/login/mock_url_fetchers.cc:77: LOG(INFO) << url_.spec();
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> Is LOG needed for mock fetcher?

Done.

http://codereview.chromium.org/3442009/diff/27001/28006#newcode92
chrome/browser/chromeos/login/mock_url_fetchers.cc:92: const GURL& url,
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> indent

Done.

http://codereview.chromium.org/3442009/diff/27001/28006#newcode98
chrome/browser/chromeos/login/mock_url_fetchers.cc:98: LOG(INFO) << url_.spec();
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> same here

Done.

http://codereview.chromium.org/3442009/diff/27001/28007
File chrome/browser/chromeos/login/mock_url_fetchers.h (right):

http://codereview.chromium.org/3442009/diff/27001/28007#newcode58
chrome/browser/chromeos/login/mock_url_fetchers.h:58: const GURL& url,
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> indent

Done.

http://codereview.chromium.org/3442009/diff/27001/28008
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/3442009/diff/27001/28008#newcode82
chrome/browser/chromeos/login/parallel_authenticator.cc:82: false /* don't
create */);
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> Don't create? If it's a new user?
> Ok, seems to be taken care of @ 456

Done.

http://codereview.chromium.org/3442009/diff/27001/28008#newcode131
chrome/browser/chromeos/login/parallel_authenticator.cc:131: LOG(INFO) <<
"Online login success";
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));

Done.

http://codereview.chromium.org/3442009/diff/27001/28008#newcode148
chrome/browser/chromeos/login/parallel_authenticator.cc:148: // Send
notification of success
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));

Done.

http://codereview.chromium.org/3442009/diff/27001/28008#newcode246
chrome/browser/chromeos/login/parallel_authenticator.cc:246: void
ParallelAuthenticator::RetryAuth(Profile* profile,
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> Couldn't find when RetryAuth get called?

It's meant for you to call, after you get the user's new password. I forgot to
plumb it up to Authenticator, but I've done so now.

http://codereview.chromium.org/3442009/diff/27001/28008#newcode273
chrome/browser/chromeos/login/parallel_authenticator.cc:273: break;
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> Comment?
> One of temporary states, waiting for the results?

Done.

http://codereview.chromium.org/3442009/diff/27001/28008#newcode320
chrome/browser/chromeos/login/parallel_authenticator.cc:320: if
(!already_reported_success_) {
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> Is it for the case when we receive online auth result first (Failed) and then
> offline (OK)?

It is.  It seemed odd to behave differently depending on when we get the online
failure.  Commented.

http://codereview.chromium.org/3442009/diff/27001/28008#newcode448
chrome/browser/chromeos/login/parallel_authenticator.cc:448: return
LOGIN_FAILED;
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> For a failed unlock returning LOGIN_FAILED but in line 365 it'll pass
> current_state_->online_outcome().
> Online login doesn't happen go unlock, is it ok to pass this param?

yeah, it's initialized to something sane.

http://codereview.chromium.org/3442009/diff/27001/28008#newcode450
chrome/browser/chromeos/login/parallel_authenticator.cc:450:
chromeos::kCryptohomeMountErrorKeyFailure) {
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> nit: indent with conditional

Done.

http://codereview.chromium.org/3442009/diff/27001/28008#newcode490
chrome/browser/chromeos/login/parallel_authenticator.cc:490: return NEED_NEW_PW;
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> NEED_NEW_PW is misleading. It'll report LoginFailure. In case of
> INVALID_GAIA_CREDENTIALS - is password change.
> 
> In case of other error codes - it's not a password change case.
> Like CAPTCHA_REQUIRED, ACCOUNT_DELETED.

for simplicity, I feel it's best to drive these failures all through the same
code pathway.  I've added a new state, though, to distinguish.

http://codereview.chromium.org/3442009/diff/27001/28009
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/3442009/diff/27001/28009#newcode1
chrome/browser/chromeos/login/parallel_authenticator.h:1: // Copyright (c) 2010
The Chromium Authors. All rights reserved.
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> BTW, in OnlineAttempt
> 
>  if (error.state() == GoogleServiceAuthError::TWO_FACTOR) {
>     LOG(WARNING) << "Two factor authenticated. Sync will not work.";
>     TriggerResolve(GaiaAuthConsumer::ClientLoginResult(),
>                    LoginFailure::None());
> 
> Should we still pass it as LoginFailure::NETWORK_AUTH_FAILED?

No, because they did in fact succeed in authenticating online.  So, we'd want to
do stuff like prompt them for their old password if we think it changed, stuff
like that.

http://codereview.chromium.org/3442009/diff/27001/28009#newcode26
chrome/browser/chromeos/login/parallel_authenticator.h:26: // Authenticates a
Chromium OS user against the Google Accounts ClientLogin API.
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> Please put these comments before class.
> 
> It would be also helpful to shortly describe main parts of big picture like
> - OnlineAttempt calls PA Resolve

Done.

http://codereview.chromium.org/3442009/diff/27001/28009#newcode38
chrome/browser/chromeos/login/parallel_authenticator.h:38: class
ParallelAuthenticatorTest;
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> nit: alpha sort

Done.

http://codereview.chromium.org/3442009/diff/27001/28009#newcode41
chrome/browser/chromeos/login/parallel_authenticator.h:41: class
ParallelAuthenticator : public Authenticator,
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> We intend to keep only ParallelAuthenticator after it's tested, right?
> Otherwise common methods need to be moved to Authenticator (TODO).

I'm pretty positive that we do, yeah.  Which is why I didn't worry about the
copypasted.

http://codereview.chromium.org/3442009/diff/27001/28009#newcode44
chrome/browser/chromeos/login/parallel_authenticator.h:44: enum AuthState {
On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> Do you have nice state graph drawn somewhere to make sure that all possible
> transitions are covered?

I do.  It's on my whiteboard!

http://codereview.chromium.org/3442009/diff/27001/28009#newcode67
chrome/browser/chromeos/login/parallel_authenticator.h:67: //
consumer_->OnLoginSuccess() with the |username| and a vector of
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> doesn't mention new consumer call protocol (OnLoginSuccess(request_pending =
> true) / OnLoginSuccess(request_pending = false) etc.

Done.

http://codereview.chromium.org/3442009/diff/27001/28009#newcode100
chrome/browser/chromeos/login/parallel_authenticator.h:100: // Call these
methods on the IO thread.
On 2010/10/06 22:12:14, Nikita Kostylev wrote:
> Not a restriction anymore.

Done.

[Nikita (slow), 2010-10-07 14:23:50]

Crash (cryptohome is not mounted)

Change password, login with new password.
Select migrate option, enter correct old password.

#0  DebugUtil::BreakDebugger () at base/debug_util_posix.cc:268
#1  0x6b9eb77a in logging::LogMessage::~LogMessage (this=0x77e48394) at
base/logging.cc:656
#2  0x6abfb057 in ProfileManager::Observe (this=0x70c7b130, type=...,
source=..., details=...) at chrome/browser/profile_manager.cc:232
#3  0x6dc79000 in NotificationService::Notify (this=0x70c55258, type=...,
source=..., details=...) at chrome/common/notification_service.cc:113
#4  0x6a90bee5 in chromeos::UserManager::NotifyOnLogin (this=0x70cbd5c0) at
chrome/browser/chromeos/login/user_manager.cc:282
#5  0x6a90b228 in chromeos::UserManager::UserLoggedIn (this=0x70cbd5c0,
email=...) at chrome/browser/chromeos/login/user_manager.cc:195
#6  0x6a8efbb4 in chromeos::LoginUtilsImpl::CompleteLogin (this=0x70f04d88,
username=..., credentials=...)
    at chrome/browser/chromeos/login/login_utils.cc:127
#7  0x6b0038b8 in chromeos::ExistingUserController::OnLoginSuccess
(this=0x70cbdf88, username=..., credentials=..., pending_requests=false)
    at chrome/browser/chromeos/login/existing_user_controller.cc:486
#8  0x6b012650 in chromeos::LoginPerformer::OnLoginSuccess (this=0x70f14280,
username=..., credentials=..., pending_requests=false)
    at chrome/browser/chromeos/login/login_performer.cc:94
#9  0x6a8f539f in chromeos::ParallelAuthenticator::OnLoginSuccess
(this=0x70f13360, credentials=..., request_pending=false)
    at chrome/browser/chromeos/login/parallel_authenticator.cc:145
#10 0x6a8fc08a in DispatchToMethod<chromeos::ParallelAuthenticator, void
(chromeos::ParallelAuthenticator::*)(GaiaAuthConsumer::ClientLoginResult const&,
bool), GaiaAuthConsumer::ClientLoginResult, bool> (obj=0x70f13360, method=...,
arg=...) at ./base/tuple.h:554
#11 0x6a8fb773 in RunnableMethod<chromeos::ParallelAuthenticator, void
(chromeos::ParallelAuthenticator::*)(GaiaAuthConsumer::ClientLoginResult const&,
bool), Tuple2<GaiaAuthConsumer::ClientLoginResult, bool> >::Run
(this=0x70fe7a98) at ./base/task.h:330
#12 0x6b9eef3e in MessageLoop::RunTask (this=0x70c52ee8, task=0x70fe7a98) at
base/message_loop.cc:410
#13 0x6b9ef047 in MessageLoop::DeferOrRunPendingTask (this=0x70c52ee8,
pending_task=...) at base/message_loop.cc:419
#14 0x6b9ef649 in MessageLoop::DoWork (this=0x70c52ee8) at
base/message_loop.cc:526
#15 0x6ba732b2 in base::MessagePumpForUI::HandleDispatch (this=0x70c52fe0) at
base/message_pump_glib.cc:280
#16 0x6ba7263f in (anonymous namespace)::WorkSourceDispatch (source=0x70c53038,
unused_func=0, unused_data=0x0) at base/message_pump_glib.cc:109
#17 0x0099be04 in IA__g_main_context_dispatch (context=0x70c51d00) at
gmain.c:1960
#18 0x0099f815 in g_main_context_iterate (context=0x70c51d00, block=1,
dispatch=1, self=0x70c2e128) at gmain.c:2591
#19 0x0099f94e in IA__g_main_context_iteration (context=0x70c51d00, may_block=1)
at gmain.c:2654
#20 0x6ba72ed7 in base::MessagePumpForUI::RunWithDispatcher (this=0x70c52fe0,
delegate=0x70c52ee8, dispatcher=0x77e49528)
    at base/message_pump_glib.cc:211
#21 0x6b9ee387 in MessageLoop::RunInternal (this=0x70c52ee8) at
base/message_loop.cc:253
#22 0x6b9ee22b in MessageLoop::RunHandler (this=0x70c52ee8) at
base/message_loop.cc:230
#23 0x6b9efeb4 in MessageLoopForUI::Run (this=0x70c52ee8, dispatcher=0x77e49528)
at base/message_loop.cc:656
#24 0x6a885320 in (anonymous namespace)::RunUIMessageLoop
(browser_process=0x70c54a28) at chrome/browser/browser_main.cc:470
#25 0x6a888747 in BrowserMain (parameters=...) at
chrome/browser/browser_main.cc:1479
#26 0x6a7115d9 in ChromeMain (argc=18, argv=0x77e4a734) at
chrome/app/chrome_dll_main.cc:947
#27 0x6a712332 in main (argc=1782255616, argv=0x0) at
chrome/app/chrome_exe_main_gtk.cc:49

[Chris Masone, 2010-10-07 15:10:47]

Yes, because I never mounted the cryptohome following a key migration in this
case.  *facepalm*


On 2010/10/07 14:23:50, Nikita Kostylev wrote:
> Crash (cryptohome is not mounted)
> 
> Change password, login with new password.
> Select migrate option, enter correct old password.
> 
> #0  DebugUtil::BreakDebugger () at base/debug_util_posix.cc:268
> #1  0x6b9eb77a in logging::LogMessage::~LogMessage (this=0x77e48394) at
> base/logging.cc:656
> #2  0x6abfb057 in ProfileManager::Observe (this=0x70c7b130, type=...,
> source=..., details=...) at chrome/browser/profile_manager.cc:232
> #3  0x6dc79000 in NotificationService::Notify (this=0x70c55258, type=...,
> source=..., details=...) at chrome/common/notification_service.cc:113
> #4  0x6a90bee5 in chromeos::UserManager::NotifyOnLogin (this=0x70cbd5c0) at
> chrome/browser/chromeos/login/user_manager.cc:282
> #5  0x6a90b228 in chromeos::UserManager::UserLoggedIn (this=0x70cbd5c0,
> email=...) at chrome/browser/chromeos/login/user_manager.cc:195
> #6  0x6a8efbb4 in chromeos::LoginUtilsImpl::CompleteLogin (this=0x70f04d88,
> username=..., credentials=...)
>     at chrome/browser/chromeos/login/login_utils.cc:127
> #7  0x6b0038b8 in chromeos::ExistingUserController::OnLoginSuccess
> (this=0x70cbdf88, username=..., credentials=..., pending_requests=false)
>     at chrome/browser/chromeos/login/existing_user_controller.cc:486
> #8  0x6b012650 in chromeos::LoginPerformer::OnLoginSuccess (this=0x70f14280,
> username=..., credentials=..., pending_requests=false)
>     at chrome/browser/chromeos/login/login_performer.cc:94
> #9  0x6a8f539f in chromeos::ParallelAuthenticator::OnLoginSuccess
> (this=0x70f13360, credentials=..., request_pending=false)
>     at chrome/browser/chromeos/login/parallel_authenticator.cc:145
> #10 0x6a8fc08a in DispatchToMethod<chromeos::ParallelAuthenticator, void
> (chromeos::ParallelAuthenticator::*)(GaiaAuthConsumer::ClientLoginResult
const&,
> bool), GaiaAuthConsumer::ClientLoginResult, bool> (obj=0x70f13360, method=...,
> arg=...) at ./base/tuple.h:554
> #11 0x6a8fb773 in RunnableMethod<chromeos::ParallelAuthenticator, void
> (chromeos::ParallelAuthenticator::*)(GaiaAuthConsumer::ClientLoginResult
const&,
> bool), Tuple2<GaiaAuthConsumer::ClientLoginResult, bool> >::Run
> (this=0x70fe7a98) at ./base/task.h:330
> #12 0x6b9eef3e in MessageLoop::RunTask (this=0x70c52ee8, task=0x70fe7a98) at
> base/message_loop.cc:410
> #13 0x6b9ef047 in MessageLoop::DeferOrRunPendingTask (this=0x70c52ee8,
> pending_task=...) at base/message_loop.cc:419
> #14 0x6b9ef649 in MessageLoop::DoWork (this=0x70c52ee8) at
> base/message_loop.cc:526
> #15 0x6ba732b2 in base::MessagePumpForUI::HandleDispatch (this=0x70c52fe0) at
> base/message_pump_glib.cc:280
> #16 0x6ba7263f in (anonymous namespace)::WorkSourceDispatch
(source=0x70c53038,
> unused_func=0, unused_data=0x0) at base/message_pump_glib.cc:109
> #17 0x0099be04 in IA__g_main_context_dispatch (context=0x70c51d00) at
> gmain.c:1960
> #18 0x0099f815 in g_main_context_iterate (context=0x70c51d00, block=1,
> dispatch=1, self=0x70c2e128) at gmain.c:2591
> #19 0x0099f94e in IA__g_main_context_iteration (context=0x70c51d00,
may_block=1)
> at gmain.c:2654
> #20 0x6ba72ed7 in base::MessagePumpForUI::RunWithDispatcher (this=0x70c52fe0,
> delegate=0x70c52ee8, dispatcher=0x77e49528)
>     at base/message_pump_glib.cc:211
> #21 0x6b9ee387 in MessageLoop::RunInternal (this=0x70c52ee8) at
> base/message_loop.cc:253
> #22 0x6b9ee22b in MessageLoop::RunHandler (this=0x70c52ee8) at
> base/message_loop.cc:230
> #23 0x6b9efeb4 in MessageLoopForUI::Run (this=0x70c52ee8,
dispatcher=0x77e49528)
> at base/message_loop.cc:656
> #24 0x6a885320 in (anonymous namespace)::RunUIMessageLoop
> (browser_process=0x70c54a28) at chrome/browser/browser_main.cc:470
> #25 0x6a888747 in BrowserMain (parameters=...) at
> chrome/browser/browser_main.cc:1479
> #26 0x6a7115d9 in ChromeMain (argc=18, argv=0x77e4a734) at
> chrome/app/chrome_dll_main.cc:947
> #27 0x6a712332 in main (argc=1782255616, argv=0x0) at
> chrome/app/chrome_exe_main_gtk.cc:49

[Nikita (slow), 2010-10-07 16:20:15]

http://codereview.chromium.org/3442009/diff/27001/28008
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/3442009/diff/27001/28008#newcode246
chrome/browser/chromeos/login/parallel_authenticator.cc:246: void
ParallelAuthenticator::RetryAuth(Profile* profile,
On 2010/10/07 01:19:06, Chris Masone wrote:
> On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> > Couldn't find when RetryAuth get called?
> 
> It's meant for you to call, after you get the user's new password. I forgot to
> plumb it up to Authenticator, but I've done so now.

I'm just calling AuthenticateToLogin.
It won't work?

http://codereview.chromium.org/3442009/diff/27001/28009
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/3442009/diff/27001/28009#newcode44
chrome/browser/chromeos/login/parallel_authenticator.h:44: enum AuthState {
On 2010/10/07 01:19:06, Chris Masone wrote:
> On 2010/10/06 23:28:27, Nikita Kostylev wrote:
> > Do you have nice state graph drawn somewhere to make sure that all possible
> > transitions are covered?
> 
> I do.  It's on my whiteboard!

Could you send me a photo? :)

http://codereview.chromium.org/3442009/diff/37001/38013#newcode78
chrome/browser/chromeos/login/parallel_authenticator.h:78: const std::string&
login_captcha);
nit: Chromium?

http://codereview.chromium.org/3442009/diff/37001/38013#newcode93
chrome/browser/chromeos/login/parallel_authenticator.h:93: void
OnLoginSuccess(const GaiaAuthConsumer::ClientLoginResult& credentials,
So in case offline ok, online failure OnPasswordChangeDetected() never got
called, right?

In LoginPerformer I'm fine with both:
- calling OnPasswordChangeDetected()
- calling OnLoginFailure() with INVALID_GAIA_CREDENTIALS

[Chris Masone, 2010-10-07 16:32:30]

I'm WFH today, so I can't send a photo right now :-/



As for the other question, I don't believe calling AuthenticateToLogin again
will work.  It'll try to mount the cryptohome again, among other things.

http://codereview.chromium.org/3442009/diff/37001/38013
File chrome/browser/chromeos/login/parallel_authenticator.h (right):

http://codereview.chromium.org/3442009/diff/37001/38013#newcode78
chrome/browser/chromeos/login/parallel_authenticator.h:78: // the Google
accounts servers and your Chrome OS device simultaneously.
On 2010/10/07 16:20:16, Nikita Kostylev wrote:
> nit: Chromium?

Done.

http://codereview.chromium.org/3442009/diff/37001/38013#newcode93
chrome/browser/chromeos/login/parallel_authenticator.h:93: //
consumer_->OnPasswordChangeDetected() is called.
On 2010/10/07 16:20:16, Nikita Kostylev wrote:
> So in case offline ok, online failure OnPasswordChangeDetected() never got
> called, right?

Exactly.  You'll get OnLoginSuccess(..., ..., true) followed by
OnLoginFailure(INVALID_GAIA_CREDENTIALS) to signal that they have a new
password, but logged in with their old one.

> 
> In LoginPerformer I'm fine with both:
> - calling OnPasswordChangeDetected()
> - calling OnLoginFailure() with INVALID_GAIA_CREDENTIALS

[Nikita (slow), 2010-10-07 22:43:06]

LGTM

Test cases suite look quite complete, great!

http://codereview.chromium.org/3442009/diff/37001/38014
File chrome/browser/chromeos/login/parallel_authenticator_unittest.cc (right):

http://codereview.chromium.org/3442009/diff/37001/38014#newcode370
chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:370:
chromeos::kCryptohomeMountErrorKeyFailure);
nit: indent

http://codereview.chromium.org/3442009/diff/37001/38014#newcode383
chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:383:
EXPECT_CALL(*mock_library_, AsyncMigrateKey(username_, _, hash_ascii_, _))
It's possible to test correct sequence of calls too.

http://codereview.chromium.org/3442009/diff/37001/38014#newcode443
chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:443:
LoginFailure::None());
nit: indent

http://codereview.chromium.org/3442009/diff/37001/38014#newcode471
chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:471:
LoginFailure::None());
nit: indent

http://codereview.chromium.org/3442009/diff/37001/38014#newcode487
chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:487:
LoginFailure::FromNetworkAuthFailure(error));
nit: indent

http://codereview.chromium.org/3442009/diff/37001/38014#newcode548
chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:548:
auth_->RetryAuth(&profile,
I'd add here another failure and then finally LoginSuccess.

[Chris Masone, 2010-10-07 22:45:37]

http://codereview.chromium.org/3442009/diff/37001/38014
File chrome/browser/chromeos/login/parallel_authenticator_unittest.cc (right):

http://codereview.chromium.org/3442009/diff/37001/38014#newcode548
chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:548:
auth_->RetryAuth(&profile,
On 2010/10/07 22:43:06, Nikita Kostylev wrote:
> I'd add here another failure and then finally LoginSuccess.

As in, do a RetryAuth, make it fail, do another one, and then have it succeed?

[Nikita (slow), 2010-10-07 22:55:36]

8 октября 2010 г. 2:45 пользователь <cmasone@chromium.org> написал:

>
> http://codereview.chromium.org/3442009/diff/37001/38014
> File chrome/browser/chromeos/login/parallel_authenticator_unittest.cc
> (right):
>
> http://codereview.chromium.org/3442009/diff/37001/38014#newcode548
> chrome/browser/chromeos/login/parallel_authenticator_unittest.cc:548:
> auth_->RetryAuth(&profile,
> On 2010/10/07 22:43:06, Nikita Kostylev wrote:
>
>> I'd add here another failure and then finally LoginSuccess.
>>
>
> As in, do a RetryAuth, make it fail, do another one, and then have it
> succeed?


Exactly.



>
> http://codereview.chromium.org/3442009/show
>



-- 
Nikita

[Nikita (slow), 2010-10-10 11:24:12]

I believe this CL be submitted.

[Nikita (slow), 2010-10-10 11:24:31]

Sorry, should be submitted.