Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(247)

Issue 10918119: Reland: Adjust ChromeToMobile error logging and mitigation. (Closed)

Created:
8 years, 3 months ago by msw
Modified:
8 years, 3 months ago
Reviewers:
nyquist, sky, Raghu Simha
CC:
chromium-reviews
Visibility:
Public.

Description

Reland: Adjust ChromeToMobile error logging and mitigation. A continuation of http://codereview.chromium.org/10913089 The original http://crrev.com/155317 exacerbated http://crbug.com/146826 Thus, it was reverted in http://crrev.com/155364 This CL additionally manages URLFetcher lifetimes with a ScopedVector. Original CL Description follows: ================================================ Adjust ChromeToMobile error logging and mitigation. Increase kMaxRetries for URLFetcher to 5 (common elsewhere). Decrease kDelayHours for URLFetcher to 1hr (w/exp backoff). Clear |access_token_| with new kGaiaOAuth2LoginAccessToken. Clear |access_token_| *and devices* on OnGetTokenFailure(). Handle invalid login refresh tokens as OnGetTokenFailure(). Clear token, devices, retry on 403 (auth) search responses. Add and log some new BAD_* UMA Metric enum values. (407 related to Issue 146685 and perhaps Issue 137267) LOG (Chrome, not UMA) send response data on failure. Corresponding src/tools/histograms/histograms.xml change: https://chromereviews.googleplex.com/4782033 BUG=102709, 120941, 146685, 146826 TEST=Less Chrome To Mobile failures, additional insightful logging.

Patch Set 1 #

Patch Set 2 : Manage URLFetcher lifetimes with a ScopedVector. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+81 lines, -25 lines) Patch
M chrome/browser/chrome_to_mobile_service.h View 1 3 chunks +14 lines, -1 line 1 comment Download
M chrome/browser/chrome_to_mobile_service.cc View 1 12 chunks +67 lines, -24 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
msw
Scott, Tommy, Raghu, please take a look; thanks!
8 years, 3 months ago (2012-09-07 15:19:20 UTC) #1
msw
I split the leak fix out to: http://codereview.chromium.org/10914147/ If that sticks, perhaps I'll be able ...
8 years, 3 months ago (2012-09-07 15:51:04 UTC) #2
sky
LGTM
8 years, 3 months ago (2012-09-07 16:19:58 UTC) #3
Raghu Simha
URLFetcher stuff LGTM. http://codereview.chromium.org/10918119/diff/2001/chrome/browser/chrome_to_mobile_service.h File chrome/browser/chrome_to_mobile_service.h (right): http://codereview.chromium.org/10918119/diff/2001/chrome/browser/chrome_to_mobile_service.h#newcode17 chrome/browser/chrome_to_mobile_service.h:17: #include "base/memory/scoped_vector.h" nit: This goes above ...
8 years, 3 months ago (2012-09-07 17:15:55 UTC) #4
msw
8 years, 3 months ago (2012-09-07 19:34:15 UTC) #5
Closing (abandoning this CL), sorry for confusion/churn.
The leak fix landed as http://crrev.com/155417
The original CL was re-landed as http://crrev.com/155449
(I was able to revert the revert of the original CL)

Powered by Google App Engine
This is Rietveld 408576698