Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2323043003:
arc: Add a global 5 minute timeout for ARC opt-in (Closed)
|
Created:
4 years, 3 months ago by Luis Héctor Chávez Modified:
4 years, 3 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Add a global 5 minute timeout for ARC opt-in
This should be a safety net for *all* boot failures we have (or might
have). This change replaces the flag |waiting_for_reply_| with a
base::OneShotTimeout that will log the situation and report a timeout
error to the user.
BUG=645233
TEST=Intentionally make ARC not send the reply, see the UI, ARC is still
running.
Committed: https://crrev.com/3d3317a6e0dde5de7effb4406c4b60bb7aa91bb5
Cr-Commit-Position: refs/heads/master@{#417770}
Patch Set 1 #Patch Set 2 : Fixed a dumb build failure #Patch Set 3 : Add logging and also remove data on timeout. #
Total comments: 4
Patch Set 4 : Added a new enum value for timeout #
Total comments: 4
Patch Set 5 : Removed obsolete comment, useless function. Added histogram string. #
Total comments: 4
Patch Set 6 : nits #
Messages
Total messages: 42 (26 generated)
The CQ bit was checked by lhchavez@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lhchavez@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lhchavez@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...
lhchavez@chromium.org changed reviewers: + khmel@chromium.org, yusukes@chromium.org
PTAL This also manages to fix the issue where Android is fully set up but Chrome is not aware of that.
https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:696: arc_sign_in_timer_.Start(FROM_HERE, kArcSignInTimeout, This happens only for initial sign-in. I would recommend to set timeout for all starts. For example, Arc may fail to load not first but second time. In this case setAuthCode... is not called. Probably you may want to set this in StartArc.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:696: arc_sign_in_timer_.Start(FROM_HERE, kArcSignInTimeout, On 2016/09/08 23:18:07, khmel wrote: > This happens only for initial sign-in. I would recommend to set timeout for all > starts. For example, Arc may fail to load not first but second time. In this > case setAuthCode... is not called. Probably you may want to set this in > StartArc. If we want to add a timeout for all starts, this is not the right level to add it to. It'd be better to put it in ArcBridgeBootstrap/ArcSession. And that code will change soon, so it's not the right time to add it either. The goal of this change is to avoid infinite loops in the opt-in ui.
On 2016/09/08 23:26:50, Luis Héctor Chávez wrote: > https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_auth_service.cc (right): > > https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_auth_service.cc:696: > arc_sign_in_timer_.Start(FROM_HERE, kArcSignInTimeout, > On 2016/09/08 23:18:07, khmel wrote: > > This happens only for initial sign-in. I would recommend to set timeout for > all > > starts. For example, Arc may fail to load not first but second time. In this > > case setAuthCode... is not called. Probably you may want to set this in > > StartArc. > > If we want to add a timeout for all starts, this is not the right level to add > it to. It'd be better to put it in ArcBridgeBootstrap/ArcSession. And that code > will change soon, so it's not the right time to add it either. > > The goal of this change is to avoid infinite loops in the opt-in ui. For me infinite loops for second login is the same level user confusion (even more) than first login. In this case user may click app launcher never-end animation will be shown in shelf. I am not going to stop this review and if you prefer to go only for first launch then lgtm
On 2016/09/08 23:29:46, khmel wrote: > On 2016/09/08 23:26:50, Luis Héctor Chávez wrote: > > > https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... > > File chrome/browser/chromeos/arc/arc_auth_service.cc (right): > > > > > https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_auth_service.cc:696: > > arc_sign_in_timer_.Start(FROM_HERE, kArcSignInTimeout, > > On 2016/09/08 23:18:07, khmel wrote: > > > This happens only for initial sign-in. I would recommend to set timeout for > > all > > > starts. For example, Arc may fail to load not first but second time. In this > > > case setAuthCode... is not called. Probably you may want to set this in > > > StartArc. > > > > If we want to add a timeout for all starts, this is not the right level to add > > it to. It'd be better to put it in ArcBridgeBootstrap/ArcSession. And that > code > > will change soon, so it's not the right time to add it either. > > > > The goal of this change is to avoid infinite loops in the opt-in ui. > > For me infinite loops for second login is the same level user confusion (even > more) than first login. In this case user may click app launcher never-end > animation will be shown in shelf. I am not going to stop this review and if you > prefer to go only for first launch then lgtm fair enough, we should definitely think how to surface/handle those as well.
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Drive-by. https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:704: OnSignInFailedInternal(ProvisioningResult::MOJO_CALL_TIMEOUT); Could you define another enum for timeout? I think we'd like to categorize this case into another one for future investigation.
The CQ bit was checked by lhchavez@chromium.org to run a CQ dry run
https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2323043003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:704: OnSignInFailedInternal(ProvisioningResult::MOJO_CALL_TIMEOUT); On 2016/09/09 01:15:13, hidehiko wrote: > Could you define another enum for timeout? > I think we'd like to categorize this case into another one for future > investigation. Good idea, done.
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/2323043003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2323043003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:239: arc_sign_in_timer_.Stop(); nit/optional: this looks redundant, because this is the first thing run in OnSignInFailedInternal? https://codereview.chromium.org/2323043003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2323043003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:79: OVERALL_SIGN_IN_TIMEOUT = 17, Could you add an entry to tools/metrics/histograms/histograms.xml
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lhchavez@chromium.org changed reviewers: + holte@chromium.org
holte PTAL tools/metrics/histograms/histograms.xml https://codereview.chromium.org/2323043003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2323043003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:239: arc_sign_in_timer_.Stop(); On 2016/09/09 02:31:48, hidehiko wrote: > nit/optional: this looks redundant, because this is the first thing run in > OnSignInFailedInternal? Done. I also deleted the comment below now that I'm at it. https://codereview.chromium.org/2323043003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2323043003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:79: OVERALL_SIGN_IN_TIMEOUT = 17, On 2016/09/09 02:31:48, hidehiko wrote: > Could you add an entry to tools/metrics/histograms/histograms.xml Ah couldn't find this string in that file so I thought I didn't have to update it :P Done.
lgtm w/ comments: https://codereview.chromium.org/2323043003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2323043003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:699: VLOG(1) << "Timed out waiting for first sign in."; VLOG(1) won't be included in the feedback report, correct? What about using LOG(ERROR) here (or in OnSignInFailedInternal) to make it easier to read feedback reports? https://codereview.chromium.org/2323043003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2323043003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:78: // Overall sign in timeout. nit: This comment does not add any info at all, but I didn't come up with a better one. Copying the comment in arc_auth_service.cc (line 397-398) is one idea, but it's probably too verbose and/or specific.
https://codereview.chromium.org/2323043003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2323043003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:699: VLOG(1) << "Timed out waiting for first sign in."; On 2016/09/09 03:28:57, Yusuke Sato wrote: > VLOG(1) won't be included in the feedback report, correct? What about using > LOG(ERROR) here (or in OnSignInFailedInternal) to make it easier to read > feedback reports? we set vmodule=arc_*=1, so this does get in feedback reports. having said that, this is a good candidate for LOG(ERROR). https://codereview.chromium.org/2323043003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.h (right): https://codereview.chromium.org/2323043003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.h:78: // Overall sign in timeout. On 2016/09/09 03:28:57, Yusuke Sato wrote: > nit: This comment does not add any info at all, but I didn't come up with a > better one. Copying the comment in arc_auth_service.cc (line 397-398) is one > idea, but it's probably too verbose and/or specific. Found a way to improve it slightly without being too specific.
lgtm.
The CQ bit was checked by lhchavez@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms lgtm
The CQ bit was checked by lhchavez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@chromium.org, yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2323043003/#ps100001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from
==========
arc: Add a global 5 minute timeout for ARC opt-in
This should be a safety net for *all* boot failures we have (or might
have). This change replaces the flag |waiting_for_reply_| with a
base::OneShotTimeout that will log the situation and report a timeout
error to the user.
BUG=645233
TEST=Intentionally make ARC not send the reply, see the UI, ARC is still
running.
==========
to
==========
arc: Add a global 5 minute timeout for ARC opt-in
This should be a safety net for *all* boot failures we have (or might
have). This change replaces the flag |waiting_for_reply_| with a
base::OneShotTimeout that will log the situation and report a timeout
error to the user.
BUG=645233
TEST=Intentionally make ARC not send the reply, see the UI, ARC is still
running.
Committed: https://crrev.com/3d3317a6e0dde5de7effb4406c4b60bb7aa91bb5
Cr-Commit-Position: refs/heads/master@{#417770}
==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3d3317a6e0dde5de7effb4406c4b60bb7aa91bb5 Cr-Commit-Position: refs/heads/master@{#417770} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
