Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2704403002:
Avoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out. (Closed)
|
Created:
3 years, 10 months ago by msarda Modified:
3 years, 9 months ago Reviewers:
Roger Tawa OOO till Jul 10th CC:
chromium-reviews, marq+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out.
When testing on Gaia prod sandbox, checking the GAIA connection fails (URL
"https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"),
fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires
and then the user token is fetched. The retry logic however colides with the 5 seconds
timer and leads to a DCHECK being fired.
After a lot of investigation, it looks like fetching the ExternalCCResult is a
convenience and failing this fetch does not actually affect merging the accounts
to the Gaia cookies. On top of that, UMA data shows that the failures for this fetch
are due to the timer of 5 seconds firing (see
https://uma.googleplex.com/histograms/?endDate=20170301&dayCount=7&histograms=Signin.Reconciler.AllExternalCcResultCompleted%2CSignin.Reconciler.ExternalCcResultTime.NotCompleted&fixupData=true&showMax=true&filters=channel%2Ceq%2C4%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial )
This CL removed the retry logic for fetching ExternalCCResult.
BUG=680622
Review-Url: https://codereview.chromium.org/2704403002
Cr-Commit-Position: refs/heads/master@{#456093}
Committed: https://chromium.googlesource.com/chromium/src/+/fe8a1e6ad75ca7bca3a6145116e6658968cd0ef6
Patch Set 1 : Prepare for review #
Total comments: 2
Patch Set 2 : Do not start the externalt CC result fetcher if it already finished. #
Total comments: 6
Patch Set 3 : Clean-up the transient state before scheduling a new ExternalCcResultFetcher #
Total comments: 4
Patch Set 4 : Address code review #Patch Set 5 : Remove retry logic from the ExternalCcResultFetcher #
Total comments: 2
Messages
Total messages: 32 (19 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Avoid starting a merge session when a uber token is not available BUG= ========== to ========== Avoid starting a merge session when a uber token is not available. When testing on Gaia prod sandbox, checking the GAIA connection fails (URL "https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"), fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires and Chrome attempts to start a merge user session even though it does not have a uber token. This leads to the following DCHECK(!uber_token_.empty()) firing in the GaiaCookieManagerService::StartFetchingMergeSession. This CL avoid starting a merge session when the timer fires if the uber token is missing. BUG=NONE ==========
msarda@chromium.org changed reviewers: + rogerta@chromium.org
Please take a look.
https://codereview.chromium.org/2704403002/diff/20001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2704403002/diff/20001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:288: helper_->StartFetchingMergeSession(); I wonder if you need to explicitly call OnMergeSessionFailure() in this case to make sure pending requests are still handled. Does that make sense?
Patchset #2 (id:40001) has been deleted
Description was changed from ========== Avoid starting a merge session when a uber token is not available. When testing on Gaia prod sandbox, checking the GAIA connection fails (URL "https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"), fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires and Chrome attempts to start a merge user session even though it does not have a uber token. This leads to the following DCHECK(!uber_token_.empty()) firing in the GaiaCookieManagerService::StartFetchingMergeSession. This CL avoid starting a merge session when the timer fires if the uber token is missing. BUG=NONE ========== to ========== Avoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out. When testing on Gaia prod sandbox, checking the GAIA connection fails (URL "https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"), fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires and then the user token is fetched. Later on, the get connection completes with a transient error (SERVICE_UNAVAILABLE) and then the ExternalCCResultFetcher starts a new request and schedules a new timer. When the timer fires, it attempts to start a second merge session that leads to the DCHECK(!uber_token_.empty()) firing in the GaiaCookieManagerService::StartFetchingMergeSession. This CL avoid re-starting a ExternalCCResultFetcher if a previous one has completed (either via a valid response or as the timer timed out). BUG=680622 ==========
Roger: May I ask you to take a second look (please ignore changes in the ios/* folder as I have added them by mistake and did not have change to clean up). I basically want to check that my understanding of this code is correct. If you think this is correct, then i'll try to see how I can add a unit test tomorrow. https://codereview.chromium.org/2704403002/diff/20001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2704403002/diff/20001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:288: helper_->StartFetchingMergeSession(); On 2017/02/21 16:15:16, Roger Tawa wrote: > I wonder if you need to explicitly call OnMergeSessionFailure() in this case to > make sure pending requests are still handled. Does that make sense? After you reply, I've debugged this more carefully. I have updated the CL description and went for a different fix.
Hi Mihai, I'm trying to understand the original problem. You say that if /GetCheckConnectionInfo fails with 404, the timer fires. I would not expect this. Looking more deeply into the code, I can see two scenario that might break this assumption. If the call to /GetCheckConnectionInfo fails with a transient error, it will be retried after a delay. During this delay, GaiaCookieManagerService::ExternalCcResultFetcher::IsRunning() will return false. So a second attempt would be made. Is it possible that the 404 is being treated as a transient error()? You do mention it completes with one. I wonder if the right fix for this bug is to change GaiaCookieManagerService::ExternalCcResultFetcher::IsRunning() to be: return helper_->gaia_auth_fetcher_ || fetchers_.size() > 0u || timer_.IsRunning(); https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (left): https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:286: // will need to be passed to Start() and Run() here. I think this comment is still valid. Is there a reason to remove it? https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:140: This method is only called at one place, line 568 below, and |external_cc_result_fetched_| is already checked there before calling. So not sure this extra check does much. Maybe this function should have a DCHECK instead: DCHECK(!helper_->external_cc_result_fetched_); https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:211: If the timer already fired, CleanupTransientState() would have been called, which resets |helper_->gaia_auth_fetcher_|. So this should not be called. Are you seeing that it is called?
Patchset #3 (id:80001) has been deleted
I've added a comment below the list of calls that lead to this error. I've updated again the code to fix it. Note that with this change, I actually wait all the retry attempts are done before the last timeout starts. This means that merge session is only started very late. I do not really understand what this fetcher does. It looks like even if it fails, the next merge session is successful. https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (left): https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:286: // will need to be passed to Start() and Run() here. On 2017/02/23 19:00:00, Roger Tawa wrote: > I think this comment is still valid. Is there a reason to remove it? Fixed. https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:140: On 2017/02/23 19:00:00, Roger Tawa wrote: > This method is only called at one place, line 568 below, and > |external_cc_result_fetched_| is already checked there before calling. So not > sure this extra check does much. Maybe this function should have a DCHECK > instead: > > DCHECK(!helper_->external_cc_result_fetched_); I've added this DCHECK and it fires. Here is the list of calls: [0224/124358.755662:ERROR:gaia_cookie_manager_service.cc(135)] ExternalCcResultFetcher::Start [0224/124358.755784:ERROR:gaia_cookie_manager_service.cc(287)] ExternalCcResultFetcher::CleanupTransientState [0224/124358.884159:WARNING:gaia_auth_fetcher.cc(721)] ClientLogin failed with [0224/124358.884292:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response from Google Accounts servers. [0224/124358.884437:ERROR:gaia_cookie_manager_service.cc(209)] ExternalCcResultFetcher::OnGetCheckConnectionInfoError [0224/124358.884529:ERROR:gaia_cookie_manager_service.cc(218)] SCHEDULE ExternalCcResultFetcher::Start in 0 s [0224/124359.882536:ERROR:gaia_cookie_manager_service.cc(135)] ExternalCcResultFetcher::Start [0224/124359.882688:ERROR:gaia_cookie_manager_service.cc(287)] ExternalCcResultFetcher::CleanupTransientState [0224/124400.009480:WARNING:gaia_auth_fetcher.cc(721)] ClientLogin failed with [0224/124400.009629:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response from Google Accounts servers. [0224/124400.009730:ERROR:gaia_cookie_manager_service.cc(209)] ExternalCcResultFetcher::OnGetCheckConnectionInfoError [0224/124400.009837:ERROR:gaia_cookie_manager_service.cc(218)] SCHEDULE ExternalCcResultFetcher::Start in 0 s [0224/124402.482121:ERROR:gaia_cookie_manager_service.cc(135)] ExternalCcResultFetcher::Start [0224/124402.482263:ERROR:gaia_cookie_manager_service.cc(287)] ExternalCcResultFetcher::CleanupTransientState [0224/124402.620963:WARNING:gaia_auth_fetcher.cc(721)] ClientLogin failed with [0224/124402.621105:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response from Google Accounts servers. [0224/124402.621207:ERROR:gaia_cookie_manager_service.cc(209)] ExternalCcResultFetcher::OnGetCheckConnectionInfoError [0224/124402.621311:ERROR:gaia_cookie_manager_service.cc(218)] SCHEDULE ExternalCcResultFetcher::Start in 0 s [0224/124407.482516:ERROR:gaia_cookie_manager_service.cc(279)] ExternalCcResultFetcher::Timeout [0224/124407.482904:ERROR:gaia_cookie_manager_service.cc(287)] ExternalCcResultFetcher::CleanupTransientState [0224/124410.337061:FATAL:gaia_cookie_manager_service.cc(134)] ExternalCcResultFetcher::Start [0224/124410.337061:FATAL:gaia_cookie_manager_service.cc(134)] Check failed: !helper_->external_cc_result_fetched_ What happens is that when we get an OnGetCheckConnectionInfoError, we schedule a retry, but we did not stop the timer (there is no call to CleanupTransientState on the retry branch in OnGetCheckConnectionInfoError. In my case, the timer fires between the moment the ExternalCcResultFetcher::Start is scheduled and the time the moment the Start method is actually called. This leads to the DCHECK being hit. https://codereview.chromium.org/2704403002/diff/60001/components/signin/core/... components/signin/core/browser/gaia_cookie_manager_service.cc:211: On 2017/02/23 19:00:00, Roger Tawa wrote: > If the timer already fired, CleanupTransientState() would have been called, > which resets |helper_->gaia_auth_fetcher_|. So this should not be called. Are > you seeing that it is called? See comment above.
Description was changed from ========== Avoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out. When testing on Gaia prod sandbox, checking the GAIA connection fails (URL "https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"), fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires and then the user token is fetched. Later on, the get connection completes with a transient error (SERVICE_UNAVAILABLE) and then the ExternalCCResultFetcher starts a new request and schedules a new timer. When the timer fires, it attempts to start a second merge session that leads to the DCHECK(!uber_token_.empty()) firing in the GaiaCookieManagerService::StartFetchingMergeSession. This CL avoid re-starting a ExternalCCResultFetcher if a previous one has completed (either via a valid response or as the timer timed out). BUG=680622 ========== to ========== Avoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out. When testing on Gaia prod sandbox, checking the GAIA connection fails (URL "https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"), fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires and then the user token is fetched. Later on, the get connection completes with a transient error (SERVICE_UNAVAILABLE) and then the ExternalCCResultFetcher starts a new request and schedules a new timer. When the second timer fires, it attempts to start a second merge session that leads to the DCHECK(!uber_token_.empty()) firing in the GaiaCookieManagerService::StartFetchingMergeSession. This CL clears the ExternalCCResultFetcher transient state before scheduling a new start. BUG=680622 ==========
Hi Mihai, I think I understand now what is going on. Please see below and let me know what you think. Thanks. https://codereview.chromium.org/2704403002/diff/100001/components/signin/core... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2704403002/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:200: I would leave the call to CleanupTransientState() where it was. I think one required change to fix 680622 is this: GaiaCookieManagerService::ExternalCcResultFetcher::IsRunning() { return helper_->gaia_auth_fetcher_ || fetchers_.size() > 0u || timer_.IsRunning(); } I think I now understand your call log. The member |external_cc_result_fetched_| is set to false in the ctor and can only set it to true after that. It is never reset back to false. In your log we see: [0224/124407.482516:ERROR:gaia_cookie_manager_service.cc(279)] ExternalCcResultFetcher::Timeout which unconditionally sets |external_cc_result_fetched_| to true. Yet 3 seconds later see see: [0224/124410.337061:FATAL:gaia_cookie_manager_service.cc(134)] ExternalCcResultFetcher::Start There are only two places in the code that could call Start(). One is line 560 below. However when |external_cc_result_fetched_| is true this call will not be made. So this is not the cause of the call. The other place is line 206, in the case of a retry. If a retry is pending when the timeout expires, |gaia_auth_fetcher_timer_| is not reset. This can happen if the retry delay is longer than the timeout. This seems to make sense here, since the DCHECK was hit 3 seconds after the expiry of a 5 second timer. Or 8 seconds, which could makes sense for the 3rd retry of an exponential retry timer with the parameters set here. So I think another fix is also required, see comment below. https://codereview.chromium.org/2704403002/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:268: helper_->gaia_auth_fetcher_.reset(); Also stop |gaia_auth_fetcher_timer_|.
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2704403002/diff/100001/components/signin/core... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2704403002/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:200: On 2017/02/24 17:20:15, Roger Tawa wrote: > I would leave the call to CleanupTransientState() where it was. > > I think one required change to fix 680622 is this: > > GaiaCookieManagerService::ExternalCcResultFetcher::IsRunning() { > return helper_->gaia_auth_fetcher_ || fetchers_.size() > 0u || > timer_.IsRunning(); > } > > I think I now understand your call log. The member > |external_cc_result_fetched_| is set to false in the ctor and can only set it to > true after that. It is never reset back to false. > > In your log we see: > > [0224/124407.482516:ERROR:gaia_cookie_manager_service.cc(279)] > ExternalCcResultFetcher::Timeout > > which unconditionally sets |external_cc_result_fetched_| to true. Yet 3 seconds > later see see: > > [0224/124410.337061:FATAL:gaia_cookie_manager_service.cc(134)] > ExternalCcResultFetcher::Start > > There are only two places in the code that could call Start(). One is line 560 > below. However when |external_cc_result_fetched_| is true this call will not be > made. So this is not the cause of the call. > > The other place is line 206, in the case of a retry. If a retry is pending when > the timeout expires, |gaia_auth_fetcher_timer_| is not reset. This can happen > if the retry delay is longer than the timeout. This seems to make sense here, > since the DCHECK was hit 3 seconds after the expiry of a 5 second timer. Or 8 > seconds, which could makes sense for the 3rd retry of an exponential retry timer > with the parameters set here. > > So I think another fix is also required, see comment below. I guess it all depends on what we want the behavior to be: * with my patch, we keep on retrying for 15 minutes (basically reaching the end of the backoff retry logic) * with your proposed solution, it stops retrying after 5 minutes. Here is the log I get with your proposed patch: [5065:5065:0303/150940.553079:VERBOSE1:gaia_cookie_manager_service.cc(353)] GaiaCookieManagerService::AddAccountToCookie: 116550294018676870905 [5065:5065:0303/150940.553395:VERBOSE1:gaia_cookie_manager_service.cc(730)] GaiaCookieManagerService::StartFetchingUbertoken account_id=116550294018676870905 [5065:5065:0303/150942.414714:VERBOSE1:gaia_cookie_manager_service.cc(560)] GaiaCookieManagerService::OnUbertokenSuccess account=116550294018676870905 [5065:5065:0303/150942.582054:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response from Google Accounts servers. [5065:5065:0303/150942.582100:VERBOSE1:gaia_cookie_manager_service.cc(200)] GaiaCookieManagerService::ExternalCcResultFetcher::OnGetCheckConnectionInfoError [5065:5065:0303/150942.582152:VERBOSE1:gaia_cookie_manager_service.cc(205)] Schedule retry in 0ms [5065:5065:0303/150942.750395:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response from Google Accounts servers. [5065:5065:0303/150942.750453:VERBOSE1:gaia_cookie_manager_service.cc(200)] GaiaCookieManagerService::ExternalCcResultFetcher::OnGetCheckConnectionInfoError [5065:5065:0303/150942.750515:VERBOSE1:gaia_cookie_manager_service.cc(205)] Schedule retry in 735ms [5065:5065:0303/150943.735907:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response from Google Accounts servers. [5065:5065:0303/150943.735979:VERBOSE1:gaia_cookie_manager_service.cc(200)] GaiaCookieManagerService::ExternalCcResultFetcher::OnGetCheckConnectionInfoError [5065:5065:0303/150943.736043:VERBOSE1:gaia_cookie_manager_service.cc(205)] Schedule retry in 1756ms [5065:5065:0303/150945.659336:WARNING:gaia_auth_fetcher.cc(742)] Incomprehensible response from Google Accounts servers. [5065:5065:0303/150945.659422:VERBOSE1:gaia_cookie_manager_service.cc(200)] GaiaCookieManagerService::ExternalCcResultFetcher::OnGetCheckConnectionInfoError [5065:5065:0303/150945.659511:VERBOSE1:gaia_cookie_manager_service.cc(205)] Schedule retry in 6281ms [5065:5065:0303/150950.496680:VERBOSE1:gaia_cookie_manager_service.cc(266)] GaiaCookieManagerService::ExternalCcResultFetcher::Timeout [5065:5065:0303/150951.260146:VERBOSE1:gaia_cookie_manager_service.cc(588)] MergeSession successful account=116550294018676870905 Basically, on a call to OnGetCheckConnectionInfoError, and in case we retry, |timer_| is not reset right away. So if helper_->fetcher_backoff_.GetTimeUntilRelease() is longer than 5 seconds, then |timer_| will fire and the timeout method will be called. I think we need to call CleanupTransientState before scheduling a next time. I agree with all the rest of you proposal. https://codereview.chromium.org/2704403002/diff/100001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:268: helper_->gaia_auth_fetcher_.reset(); On 2017/02/24 17:20:15, Roger Tawa wrote: > Also stop |gaia_auth_fetcher_timer_|. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Avoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out. When testing on Gaia prod sandbox, checking the GAIA connection fails (URL "https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"), fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires and then the user token is fetched. Later on, the get connection completes with a transient error (SERVICE_UNAVAILABLE) and then the ExternalCCResultFetcher starts a new request and schedules a new timer. When the second timer fires, it attempts to start a second merge session that leads to the DCHECK(!uber_token_.empty()) firing in the GaiaCookieManagerService::StartFetchingMergeSession. This CL clears the ExternalCCResultFetcher transient state before scheduling a new start. BUG=680622 ========== to ========== Avoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out. When testing on Gaia prod sandbox, checking the GAIA connection fails (URL "https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"), fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires and then the user token is fetched. The retry logic however colides with the 5 seconds timer and leads to a DCHECK being fired. After a lot of investigation, it looks like fetching the ExternalCCResult is a convenience and failing this fetch does not actually affect merging the accounts to the Gaia cookies. On top of that, UMA data shows that the failures for this fetch are due to the timer of 5 seconds firing (see https://uma.googleplex.com/histograms/?endDate=20170301&dayCount=7&histograms... ) This CL removed the retry logic for fetching ExternalCCResult. BUG=680622 ==========
The CQ bit was checked by msarda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Roger: Please take another look. Note that there was no point in adding unit tests for the retry case as this CL actually removes the retry logic (as we discussed off-line last Friday).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Note that perf is done, may I ask you to take another look?
lgtm Thanks for being patient with me :-) Do you plan to leave VLOGs in before committing? Or maybe change them to DVLOGs? All options fine with me. https://codereview.chromium.org/2704403002/diff/140001/components/signin/core... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2704403002/diff/140001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:156: timer_.IsRunning(); Since there are no longer retries, this change is no longer strictly needed. But seems fine to keep it in anyway.
I prefer keeping the VLOG (the entire file uses VLOG). I do not see any hard in that for now. https://codereview.chromium.org/2704403002/diff/140001/components/signin/core... File components/signin/core/browser/gaia_cookie_manager_service.cc (right): https://codereview.chromium.org/2704403002/diff/140001/components/signin/core... components/signin/core/browser/gaia_cookie_manager_service.cc:156: timer_.IsRunning(); On 2017/03/10 16:48:51, Roger Tawa wrote: > Since there are no longer retries, this change is no longer strictly needed. > But seems fine to keep it in anyway. This fetcher is running as long as the timer did not fire (once it fires, it is stopped, right?) So I prefer keeping this check for completeness.
The CQ bit was checked by msarda@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489164830393760,
"parent_rev": "5759cafbf041c1fdc2b26416b4644e52f7df6dfc", "commit_rev":
"fe8a1e6ad75ca7bca3a6145116e6658968cd0ef6"}
Message was sent while issue was closed.
Description was changed from ========== Avoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out. When testing on Gaia prod sandbox, checking the GAIA connection fails (URL "https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"), fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires and then the user token is fetched. The retry logic however colides with the 5 seconds timer and leads to a DCHECK being fired. After a lot of investigation, it looks like fetching the ExternalCCResult is a convenience and failing this fetch does not actually affect merging the accounts to the Gaia cookies. On top of that, UMA data shows that the failures for this fetch are due to the timer of 5 seconds firing (see https://uma.googleplex.com/histograms/?endDate=20170301&dayCount=7&histograms... ) This CL removed the retry logic for fetching ExternalCCResult. BUG=680622 ========== to ========== Avoid re-starting a ExternalCCResultFetcher if a previous has finished or timed out. When testing on Gaia prod sandbox, checking the GAIA connection fails (URL "https://accounts.sandbox.google.com/GetCheckConnectionInfo?source=ChromiumBrowser"), fails "Error 404 (Not Found)!!1". In this case, the ExternalCcResultFetcher timer fires and then the user token is fetched. The retry logic however colides with the 5 seconds timer and leads to a DCHECK being fired. After a lot of investigation, it looks like fetching the ExternalCCResult is a convenience and failing this fetch does not actually affect merging the accounts to the Gaia cookies. On top of that, UMA data shows that the failures for this fetch are due to the timer of 5 seconds firing (see https://uma.googleplex.com/histograms/?endDate=20170301&dayCount=7&histograms... ) This CL removed the retry logic for fetching ExternalCCResult. BUG=680622 Review-Url: https://codereview.chromium.org/2704403002 Cr-Commit-Position: refs/heads/master@{#456093} Committed: https://chromium.googlesource.com/chromium/src/+/fe8a1e6ad75ca7bca3a6145116e6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/fe8a1e6ad75ca7bca3a6145116e6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
