Retry fetching OAuth1 request token on failure

Retry fetching OAuth1 request token on failure

When OAuth1 request token fetching fails, there may be intermittent
network issue, and it is worth try again.  This actually happened on
Chrome OS on the first login OOBE when switching network for the new
profile.

BUG=chromium:128592
TEST=manual

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

[kochi, 2012-05-25 08:17:26]

Hi Nikita,

Could you review this?

[kochi, 2012-05-25 09:53:57]

In case Nikita is OOO and can't review today, Zelidrag,
can you take a look at this?

Dave, once this gets LGTM, could you handle OWNERS lgtm?

Thanks,

On 2012/05/25 08:17:26, Takayoshi Kochi wrote:
> Hi Nikita,
> 
> Could you review this?

[zel, 2012-05-25 15:19:36]

http://chromiumcodereview.appspot.com/10446033/diff/9002/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://chromiumcodereview.appspot.com/10446033/diff/9002/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:1303: // If the request failed due
to network flakiness, try again.
Since this process is no longer super simple, let's get OAuth1 token fetching
login out of LoginUtilsImpl class - just as we did with its verification.

http://chromiumcodereview.appspot.com/10446033/diff/9002/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:1457: void
LoginUtilsImpl::OnConnectionTypeChanged(
when we detect nw changes, you should be able to restart OAuth1 token fetching
(not just verification) if it hasn't been completed already

[Nikita (slow), 2012-05-25 18:12:08]

http://chromiumcodereview.appspot.com/10446033/diff/9002/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://chromiumcodereview.appspot.com/10446033/diff/9002/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:1304: if (error.state() ==
GoogleServiceAuthError::SERVICE_UNAVAILABLE &&
error.state() == GoogleServiceAuthError::CONNECTION_FAILED ||
error.state() == GoogleServiceAuthError::SERVICE_UNAVAILABLE ||
error.state() == GoogleServiceAuthError::REQUEST_CANCELED)

See RetryOnError()

[kochi, 2012-05-28 08:46:12]

Hi Nikita, Zel,

Thanks for the review.

As Zel suggested, I split OAuth1 token fetcher as a class
from LoginUtilsImpl.

Please take another look.

Thanks!

http://chromiumcodereview.appspot.com/10446033/diff/9002/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://chromiumcodereview.appspot.com/10446033/diff/9002/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:1303: // If the request failed due
to network flakiness, try again.
On 2012/05/25 15:19:36, zel wrote:
> Since this process is no longer super simple, let's get OAuth1 token fetching
> login out of LoginUtilsImpl class - just as we did with its verification. 

Done.

http://chromiumcodereview.appspot.com/10446033/diff/9002/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:1304: if (error.state() ==
GoogleServiceAuthError::SERVICE_UNAVAILABLE &&
On 2012/05/25 18:12:08, Nikita Kostylev wrote:
> error.state() == GoogleServiceAuthError::CONNECTION_FAILED ||
> error.state() == GoogleServiceAuthError::SERVICE_UNAVAILABLE ||
> error.state() == GoogleServiceAuthError::REQUEST_CANCELED)
> 
> See RetryOnError()

Done.

http://chromiumcodereview.appspot.com/10446033/diff/9002/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:1457: void
LoginUtilsImpl::OnConnectionTypeChanged(
This is done in the new class.

On 2012/05/25 15:19:36, zel wrote:
> when we detect nw changes, you should be able to restart OAuth1 token fetching
> (not just verification) if it hasn't been completed already

[Nikita (slow), 2012-05-28 10:41:47]

lgtm

http://chromiumcodereview.appspot.com/10446033/diff/3005/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://chromiumcodereview.appspot.com/10446033/diff/3005/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:1506: LOG(WARNING) << "Failed to
fetch OAuth1 access token.";
LOG(ERROR)?

[kochi, 2012-05-28 10:57:07]

Thanks Nikita,
log severity is changed.


Zel,
Could you take another look?
I know it's holiday in US, though.

http://chromiumcodereview.appspot.com/10446033/diff/3005/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://chromiumcodereview.appspot.com/10446033/diff/3005/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:1506: LOG(WARNING) << "Failed to
fetch OAuth1 access token.";
On 2012/05/28 10:41:48, Nikita Kostylev wrote:
> LOG(ERROR)?

Done.

[zel, 2012-05-29 15:30:47]

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:547: class OAuth1TokenFetcher
we should move all these OAuthSoemthing classes to another set of files. If you
are going to merge this into M20, you can leave it here, but please add TODO for
that part.

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:569:
net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
DCHECK(delegate)

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:592: if ((error.state() ==
GoogleServiceAuthError::CONNECTION_FAILED ||
if network is disconnected, it does not make sense to increase retry_count_ and
retry. we should rather way until we get connected and repeat the whole sequence

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:595: retry_count_++ <
kMaxOAuth1TokenRequestAttemptCount) {
align

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:598:
base::Bind(&OAuth1TokenFetcher::Start, base::Unretained(this)),
base::Unretained(this) could be a source of trouble. I would rather like to see
weakptr here to make sure we are not calling this method when the object is
nuked.

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:638: if (type !=
net::NetworkChangeNotifier::CONNECTION_NONE &&
what about captive portal? do we check that case anywhere?

[kochi, 2012-05-29 16:48:43]

Thanks for the review!

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:547: class OAuth1TokenFetcher
On 2012/05/29 15:30:47, zel wrote:
> we should move all these OAuthSoemthing classes to another set of files. If
you
> are going to merge this into M20, you can leave it here, but please add TODO
for
> that part.

Done for OAuthLoginVerifier, PolicyOAuthFetcher, OAuth1TokenFetcher.

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:569:
net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
On 2012/05/29 15:30:47, zel wrote:
> DCHECK(delegate)

Done.

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:592: if ((error.state() ==
GoogleServiceAuthError::CONNECTION_FAILED ||
Changed the logic.

We check the GAIA server reacheability at Start(), and wait for online.
This function is called only when we got some error from the fetcher.

On 2012/05/29 15:30:47, zel wrote:
> if network is disconnected, it does not make sense to increase retry_count_
and
> retry. we should rather way until we get connected and repeat the whole
sequence

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:595: retry_count_++ <
kMaxOAuth1TokenRequestAttemptCount) {
On 2012/05/29 15:30:47, zel wrote:
> align

Already aligned (see enclosing parentheses around all error.state() == ...).

http://chromiumcodereview.appspot.com/10446033/diff/4007/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:598:
base::Bind(&OAuth1TokenFetcher::Start, base::Unretained(this)),
On 2012/05/29 15:30:47, zel wrote:
> base::Unretained(this) could be a source of trouble. I would rather like to
see
> weakptr here to make sure we are not calling this method when the object is
> nuked.

Done.

[zel, 2012-05-29 17:26:44]

lgtm

[commit-bot: I haz the power, 2012-05-30 01:20:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/10446033/13007