The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c28b7a4b40b447aecf22d367e9ed278700c0bdc4

commit c28b7a4b40b447aecf22d367e9ed278700c0bdc4
Author: Colin Blundell <blundell@chromium.org>
Date: Wed Aug 01 15:31:06 2018

IdentityManager: Fix timing of token update observer callbacks

This CL fixes a timing issue with the implementation of
IdentityManager::Observer::OnRefreshTokenUpdatedForAccount():

- It is fired from IdentityManager::WillFireOnRefreshTokenAvailable()
- which is fired from
ProfileOAuth2TokenService::OnRefreshTokenAvailable()
- just *before* ProfileOAuth2TokenService calls
CancelRequestsForAccount().

The upshot is that any access token requests made from
IdentityManager::Observer::OnRefreshTokenUpdatedForAccount()
implementations will be immediately cancelled. Yikes! On desktop, it
also means that access token requests that are made from
ProfileOAuth2TokenService::Observer::OnRefreshTokenAvailable() clients
after any such cancellations will also be cancelled, as
MutableProfileOAuth2TokenServiceDelegate has a policy of immediately
canceling access token requests for some amount of time after it has
seen a cancelled access token request (this is part of its backoff
policy).

The fix is the following:
- Move the firing of the IdentityManager::Observer callbacks to its
  (new) implementation of the corresponding
  OAuth2TokenService::Observer callbacks.

Note that the updating of IdentityManager's internal state must still
be done in IdentityManager::WillFireOnRefreshToken{Available, Revoked};
this ensures that it updates its state before any observers of
ProfileOAuth2TokenService are notified of the state change, and thus
that any such observers see a consistent state between IdentityManager
and ProfileOAuth2TokenService. To decouple the internal update from
the firing of the observer callbacks, some state is now saved from
the internal updates firing to forward in the observer callbacks.

This change is unittested via extensions to the
PrimaryAccountAccessTokenFetcher unittests that ensure that the fetcher
does not have a request cancelled that is made in its implementation of
IdentityManager::Observer::OnRefreshTokenUpdatedForAccount(). Before
the production fix, the extended unittests would have failed, as the
fetchers were seeing a cancelled request and then retrying it (which
succeeded).

Bug:  869376 
Change-Id: Ic5d3208ef893ad0e821c38bbb88c713d68042a1a
Reviewed-on: https://chromium-review.googlesource.com/1152731
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579813}
[modify] https://crrev.com/c28b7a4b40b447aecf22d367e9ed278700c0bdc4/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/c28b7a4b40b447aecf22d367e9ed278700c0bdc4/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/c28b7a4b40b447aecf22d367e9ed278700c0bdc4/services/identity/public/cpp/identity_manager_unittest.cc
[modify] https://crrev.com/c28b7a4b40b447aecf22d367e9ed278700c0bdc4/services/identity/public/cpp/primary_account_access_token_fetcher.h
[modify] https://crrev.com/c28b7a4b40b447aecf22d367e9ed278700c0bdc4/services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This document describes the potential impact of the bug and the reasons that we believe that cherrypicking is appropriate: https://docs.google.com/document/d/1oyzXBl4Z-PLh36br9l9T2yhsHiPnZVmY3XF-B7wI2DI/edit#

I will be OOO from EOD Paris time today, but Mihai will be available all next week to answer questions and perform the cherrypick.

The cherrypick did not apply cleanly on 69; I resolved all the conflicts and uploaded it here: https://chromium-review.googlesource.com/c/chromium/src/+/1161934. That CL description describes the conflicts that were resolved.

Pls apply appropriate OSs label. Thank you.

Re #5: Unable to access -  https://docs.google.com/document/d/1oyzXBl4Z-PLh36br9l9T2yhsHiPnZVmY3XF-B7wI2DI/edit#.

Bfore we approve merge to M69, please answer followings:
* Is this M69 regression? Is it critical?
* Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M69?
* Any other important details to justify the merge.
Please note M69 is already in Beta, so merge bar is very high. Thank you.

It is funny - Colin forgot to share the document (https://docs.google.com/document/d/1oyzXBl4Z-PLh36br9l9T2yhsHiPnZVmY3XF-B7wI2DI/edit# ) before leaving on holiday, so I cannot access it either.

Answers in-line:
* Is this M69 regression? Is it critical?
Without the fix, requests for access tokens that are issued from the clients of the IdentityService will be cancelled. After Colin's investigation, there vast majority of the clients of the IdentityService do retry. The only one that does not retry is the InvalidationService. It is impossible for us to evaluate what could go wrong in this scenario. 
* Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M69?
Yes, this has been on canary for the last 4 days. It has good unit test coverage. We believe it should be safe to merge to M69 (we also have 4 more weeks before branch).
* Any other important details to justify the merge.
At this point, this bug was discovered by reading the code, so we do not have a concrete example with a real-world issue. However, knowing the code, we advise on merging it back to avoid any breakages or useless retries when requesting access tokens.

Approving merge to M69 branch 3497 based on comment #9. Please merge ASAP so we can take it in for this week M69 beta release. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/910861876bd479527f28b35eb04a6b3212f7f25f

commit 910861876bd479527f28b35eb04a6b3212f7f25f
Author: Mihai Sardarescu <msarda@chromium.org>
Date: Mon Aug 06 16:06:34 2018

IdentityManager: Fix timing of token update observer callbacks

This CL fixes a timing issue with the implementation of
IdentityManager::Observer::OnRefreshTokenUpdatedForAccount():

- It is fired from IdentityManager::WillFireOnRefreshTokenAvailable()
- which is fired from
ProfileOAuth2TokenService::OnRefreshTokenAvailable()
- just *before* ProfileOAuth2TokenService calls
CancelRequestsForAccount().

The upshot is that any access token requests made from
IdentityManager::Observer::OnRefreshTokenUpdatedForAccount()
implementations will be immediately cancelled. Yikes! On desktop, it
also means that access token requests that are made from
ProfileOAuth2TokenService::Observer::OnRefreshTokenAvailable() clients
after any such cancellations will also be cancelled, as
MutableProfileOAuth2TokenServiceDelegate has a policy of immediately
canceling access token requests for some amount of time after it has
seen a cancelled access token request (this is part of its backoff
policy).

The fix is the following:
- Move the firing of the IdentityManager::Observer callbacks to its
  (new) implementation of the corresponding
  OAuth2TokenService::Observer callbacks.

Note that the updating of IdentityManager's internal state must still
be done in IdentityManager::WillFireOnRefreshToken{Available, Revoked};
this ensures that it updates its state before any observers of
ProfileOAuth2TokenService are notified of the state change, and thus
that any such observers see a consistent state between IdentityManager
and ProfileOAuth2TokenService. To decouple the internal update from
the firing of the observer callbacks, some state is now saved from
the internal updates firing to forward in the observer callbacks.

This change is unittested via extensions to the
PrimaryAccountAccessTokenFetcher unittests that ensure that the fetcher
does not have a request cancelled that is made in its implementation of
IdentityManager::Observer::OnRefreshTokenUpdatedForAccount(). Before
the production fix, the extended unittests would have failed, as the
fetchers were seeing a cancelled request and then retrying it (which
succeeded).

This CL is a cherrypick of
https://chromium-review.googlesource.com/c/chromium/src/+/1152731 to
M69. Changes from the version on trunk:
- Minor conflict resolved in identity_manager.h due to IdentityManager
  observing GaiaCookieManagerService on trunk but not on 69
- The bulk of
  https://chromium-review.googlesource.com/c/chromium/src/+/1149867
  applied to identity_manager.cc to facilitate using |iterator| (this
  also resolved conflicts that arose in the cherrypick).

Bug:  869376 
Change-Id: I488600d724fb56f94490b0bf6323eeb32141ee26
Reviewed-on: https://chromium-review.googlesource.com/1152731
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#579813}
Reviewed-on: https://chromium-review.googlesource.com/1163718
Cr-Commit-Position: refs/branch-heads/3497@{#421}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/910861876bd479527f28b35eb04a6b3212f7f25f/services/identity/public/cpp/identity_manager.cc
[modify] https://crrev.com/910861876bd479527f28b35eb04a6b3212f7f25f/services/identity/public/cpp/identity_manager.h
[modify] https://crrev.com/910861876bd479527f28b35eb04a6b3212f7f25f/services/identity/public/cpp/identity_manager_unittest.cc
[modify] https://crrev.com/910861876bd479527f28b35eb04a6b3212f7f25f/services/identity/public/cpp/primary_account_access_token_fetcher.h
[modify] https://crrev.com/910861876bd479527f28b35eb04a6b3212f7f25f/services/identity/public/cpp/primary_account_access_token_fetcher_unittest.cc

Whoops, shared the doc. Thanks for the cherrypick, Mihai!