New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 913618 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: 2019-02-15
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

DeviceSyncService.SetSoftwareFeatureState fails for locally set features

Project Member Reported by hansberry@chromium.org, Dec 10

Issue description

Labels: -Pri-2 Pri-1
Bumping priority because we should really get a better idea of what's going on here. If it *is* just a timing issue, how can we measure and verify that?

Ryan, note that "Enabling fails only for Better Together Suite" because that's the only feature that's enabled from the Chromebook side. All other features are only enabled from Android.
Jeremy and I just dug into this a little more -- he found this line in CryptAuthDeviceManager [1] which prevents clients from getting fresh data from CryptAuth when syncing after calling ToggleEasyUnlock for BetterTogether. We should be able to fix this issue by adding another check here for INVOCATION_REASON_FEATURE_TOGGLED (and make sure that is passed along correctly after we enable/disable BetterTogether).

1) https://cs.chromium.org/chromium/src/chromeos/services/device_sync/cryptauth_device_manager_impl.cc?rcl=9f156827c1ca92f4ac92d651c6388602ce3bd20e&l=790
Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 15

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

commit 7d3e24d74083aab6a8adb0d21f754cc0ae3dc8c3
Author: Kyle Horimoto <khorimoto@chromium.org>
Date: Tue Jan 15 02:21:06 2019

[CrOS MultiDevice] Require fresh reads for syncs in some cases.

Before this CL, stale reads were allowed for GetMyDevicesRequests in
most cases. This CL updates that logic such that fresh reads are
required when we expect that data was very recently changed (e.g., when
a feature has just been toggled).

Bug: 913618
Change-Id: I9c515b7ce7da56f790fdede7443bfc7cc9e5ee73
Reviewed-on: https://chromium-review.googlesource.com/c/1407962
Reviewed-by: Jeremy Klein <jlklein@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Andrii Shyshkalov <tandrii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622682}
[modify] https://crrev.com/7d3e24d74083aab6a8adb0d21f754cc0ae3dc8c3/chromeos/services/device_sync/cryptauth_device_manager_impl.cc
[modify] https://crrev.com/7d3e24d74083aab6a8adb0d21f754cc0ae3dc8c3/chromeos/services/device_sync/cryptauth_device_manager_impl_unittest.cc

Labels: M-72 Merge-Request-72
Requesting merge to M-72. Users in the wild have been complaining that they go through the multi-device setup flow only to see that the phone they just sent up is stuck in the "verifying" state. We believe that this is due to the fact that "stale reads" were allowed with this operation, meaning that the user could enable the feature, then get stale metadata back about the feature which says that it is disabled, even though it is not.

The fix in comment #5 disallows stale reads for this use case. It is a safe merge (i.e., it does not contain any complex logic), but that being said, it is a speculative fix since we don't have a reliable repro of the bug.

We'd like to get this fix out in the wild as soon as possible so that we can gauge whether or not it has actually fixed the problem.
Project Member

Comment 7 by sheriffbot@chromium.org, Jan 15

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: We are only 13 days from stable.
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-72 Merge-Rejected-72
If this is a speculative fix without a repro, it sounds like you should put this fix on the dev channel and look for results there.  We're in the middle of beta at this point and stable is quickly approaching.
NextAction: 2019-02-15
Makes sense - no problem. I'm setting a NextAction date for 2/15, when we should have some dev/beta results. Thanks!

Sign in to add a comment