Proactively make CryptAuth syncs when retrying multidevice setup |
|||||||
Issue descriptionSee this thread for context: https://groups.google.com/a/google.com/forum/#!topic/better-together-dev/F4Y-QPPwCr8 When in the pending verification state, we shouldn't rely entirely on GCM tickles to force the Chromebook to sync. On every retry, we should also sync devices (maybe after a small delay) just to see if we missed the tickle.
,
Oct 5
,
Oct 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/adedcd1dee83500692045e89f1e2e1360497111f commit adedcd1dee83500692045e89f1e2e1360497111f Author: Kyle Horimoto <khorimoto@google.com> Date: Tue Oct 23 17:19:57 2018 [CrOS MultiDevice] Proactively sync device metadata after API calls. Any successful call to the SetSoftwareFeatureState() API function should result in a change to the "enabled_features" field of the ExternalDeviceInfo message returned by CryptAuth. Indeed, once SetSoftwareFeatureState() completes, CryptAuth issues a GCM tickle to all devices on the same GAIA account, telling each of these devices that new data is available to sync. Unfortunately, GCM is flaky; sometimes it takes a long time for the tickle to arrive to devices, and sometimes the tickle does not arrive at all. This resulted in bugs where users would attempt to change a feature's state (e.g., when setting a phone as the multi-device host), the network call would succeed, but no GCM tickle would arrive, resulting in clients being stuck waiting for new device metadata to arrive. This CL fixes that issue by proactively requesting a sync as soon as the SetSoftwareFeatureState() completes. Bug: 892349 Change-Id: I7d7bd610a4a997c398e0aecd1450b050fa1e9429 Reviewed-on: https://chromium-review.googlesource.com/c/1285692 Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#601998} [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_regular.cc [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/chrome/test/base/in_process_browser_test.cc [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/chromeos/services/device_sync/device_sync_impl.cc [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/chromeos/services/device_sync/device_sync_impl.h [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/chromeos/services/device_sync/device_sync_service.cc [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/chromeos/services/device_sync/device_sync_service_unittest.cc [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/chromeos/services/device_sync/public/cpp/device_sync_client_impl_unittest.cc [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/chromeos/services/device_sync/public/mojom/device_sync.mojom [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/components/cryptauth/cryptauth_device_manager_impl.cc [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/components/cryptauth/sync_scheduler.cc [modify] https://crrev.com/adedcd1dee83500692045e89f1e2e1360497111f/components/cryptauth/sync_scheduler.h
,
Oct 23
,
Oct 23
+kbleicher@ FYI
,
Oct 24
This change impacts a number of areas. Can you add context regarding testing on ToT/M72 or otherwise prior to a merge? I'd like to understand the merge risk. Thanks.
,
Oct 24
Verified manually by going through multi-device setup flow in OOBE as well as post-OOBE modes. Also manually verified forgetting device, then setting it up again.
,
Oct 24
Approving merge to M71 Chrome OS.
,
Oct 24
,
Oct 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/26a790756b4d1a11ae66ee7e1850a76462f0f2bf commit 26a790756b4d1a11ae66ee7e1850a76462f0f2bf Author: Kyle Horimoto <khorimoto@google.com> Date: Wed Oct 24 20:34:05 2018 [CrOS MultiDevice] Proactively sync device metadata after API calls. Any successful call to the SetSoftwareFeatureState() API function should result in a change to the "enabled_features" field of the ExternalDeviceInfo message returned by CryptAuth. Indeed, once SetSoftwareFeatureState() completes, CryptAuth issues a GCM tickle to all devices on the same GAIA account, telling each of these devices that new data is available to sync. Unfortunately, GCM is flaky; sometimes it takes a long time for the tickle to arrive to devices, and sometimes the tickle does not arrive at all. This resulted in bugs where users would attempt to change a feature's state (e.g., when setting a phone as the multi-device host), the network call would succeed, but no GCM tickle would arrive, resulting in clients being stuck waiting for new device metadata to arrive. This CL fixes that issue by proactively requesting a sync as soon as the SetSoftwareFeatureState() completes. Bug: 892349 Change-Id: I7d7bd610a4a997c398e0aecd1450b050fa1e9429 Reviewed-on: https://chromium-review.googlesource.com/c/1285692 Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#601998}(cherry picked from commit adedcd1dee83500692045e89f1e2e1360497111f) Reviewed-on: https://chromium-review.googlesource.com/c/1298397 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#304} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_regular.cc [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/chrome/test/base/in_process_browser_test.cc [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/chromeos/services/device_sync/device_sync_impl.cc [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/chromeos/services/device_sync/device_sync_impl.h [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/chromeos/services/device_sync/device_sync_service.cc [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/chromeos/services/device_sync/device_sync_service_unittest.cc [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/chromeos/services/device_sync/public/cpp/device_sync_client_impl_unittest.cc [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/chromeos/services/device_sync/public/mojom/device_sync.mojom [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/components/cryptauth/cryptauth_device_manager_impl.cc [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/components/cryptauth/sync_scheduler.cc [modify] https://crrev.com/26a790756b4d1a11ae66ee7e1850a76462f0f2bf/components/cryptauth/sync_scheduler.h
,
Oct 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/26a790756b4d1a11ae66ee7e1850a76462f0f2bf Commit: 26a790756b4d1a11ae66ee7e1850a76462f0f2bf Author: khorimoto@google.com Commiter: khorimoto@chromium.org Date: 2018-10-24 20:34:05 +0000 UTC [CrOS MultiDevice] Proactively sync device metadata after API calls. Any successful call to the SetSoftwareFeatureState() API function should result in a change to the "enabled_features" field of the ExternalDeviceInfo message returned by CryptAuth. Indeed, once SetSoftwareFeatureState() completes, CryptAuth issues a GCM tickle to all devices on the same GAIA account, telling each of these devices that new data is available to sync. Unfortunately, GCM is flaky; sometimes it takes a long time for the tickle to arrive to devices, and sometimes the tickle does not arrive at all. This resulted in bugs where users would attempt to change a feature's state (e.g., when setting a phone as the multi-device host), the network call would succeed, but no GCM tickle would arrive, resulting in clients being stuck waiting for new device metadata to arrive. This CL fixes that issue by proactively requesting a sync as soon as the SetSoftwareFeatureState() completes. Bug: 892349 Change-Id: I7d7bd610a4a997c398e0aecd1450b050fa1e9429 Reviewed-on: https://chromium-review.googlesource.com/c/1285692 Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#601998}(cherry picked from commit adedcd1dee83500692045e89f1e2e1360497111f) Reviewed-on: https://chromium-review.googlesource.com/c/1298397 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#304} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by khorimoto@chromium.org
, Oct 4Status: Started (was: Untriaged)