Sync loop in RecentTabsMediator |
|||
Issue descriptionWe've got a sync loop in RecentTabsMediator that is causing a sync update to be sent about once a second. The code path is: 1) On startup, we create an instance of RecentTabs to go in the tab grid. While initializing, we call [RecentTabsMediator reloadSessions]. 2) reloadSessions calls reloadSessionData, which calls ProfileSyncService::TriggerRefresh(). 3) TriggerRefresh() triggers a sync refresh, and when the answer comes back, the model observer bridge invokes reloadSessions, taking us back to #1. Loop forever. I noticed this because scrolling in Collections was janky, stuttering about once a second. It's much easier to see if you visit about:sync-internals and look at the debug logs. M68 and earlier are unaffected. I believe this is because they did not use the USS implementation of sessions, which was enabled by default in https://chromium-review.googlesource.com/1086945 on June 18 in M69. The previous implementation triggered exactly only sync update, but terminated the cycle without triggering a second refresh. The new implementation was running as a 50/50 experiment in dev and canary. I believe this is why not everyone saw this bug, and also why I was unable to reproduce in a local build this morning. It also explains why Sergio was able to reproduce in the afternoon. To hit this bug, you need to: 1) Be running M69 dev channel, or trunk compiled after June 18. 2) Be in the 50% experiment, if on dev channel prior to the June 20 release. 3) Have UIRefresh enabled. 4) Be signed in to sync.
,
Jun 19 2018
,
Jun 19 2018
Sorry for the delay, I can pick this up since I introduced the issue.
,
Jun 19 2018
rohitraio@: I'm trying to get an iPhone to repro the issue, but in case you can do it meanwhile, would you mind pasting chrome://sync-internals? I'd like to rule out that sync engine throttling is relevant to explain the diff.
,
Jun 19 2018
After taking a look at the screenshots you shared with me offline, I suspect the following: The first call to TriggerRefresh() fetches remote updates, and there's none. SessionSyncBridge::ApplySyncChanges() receives an empty list of updates, but nevertheless broadcasts changes in https://cs.chromium.org/chromium/src/components/sync_sessions/session_sync_bridge.cc?l=266&rcl=de403170ea9cabbe26afd1917c286b80b1b7d688 I think it makes sense to avoid such notification when the update list is empty. The reason why the pre-USS implementation could differ is that GenericChangeProcessor::CommitChangesFromSyncModel() returns early for the empty-updates case. I'll write a quick patch to prevent this, but it might still make sense to update the iOS code too.
,
Jun 19 2018
The iOS Recent Tabs code will be fixed by https://chromium-review.googlesource.com/c/chromium/src/+/1106001.
,
Jun 19 2018
Sync counterpart in https://chromium-review.googlesource.com/c/chromium/src/+/1106143 Either of the two patches should fix the observed problem, but I'd go ahead and fix both.
,
Jun 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e0e4ea992d21f5abf688c505c8f9751f62ccd207 commit e0e4ea992d21f5abf688c505c8f9751f62ccd207 Author: Mikel Astiz <mastiz@chromium.org> Date: Tue Jun 19 14:56:17 2018 Fix potential sync loops due to broadcasting empty updates We thought there was no reason to avoid broadcasting notifications when an empty list of remote updates is received, but it turns out that at least one client (iOS RecentTabsMediator) relied on the fact that such events would be ignored (not broadcasted via SyncServiceObserver::OnForeignSessionUpdated()). To avoid behavioral changes with the pre-USS implementation, which dropped such empty updates in an earlier stage (I believe in GenericChangeProcessor::CommitChangesFromSyncModel()), we adopt similar logic for USS (SessionSyncBridge). Bug: 854049 Change-Id: I6d3b6123b7fb15bb0e5bfeccdc0a6d9170427fe4 Reviewed-on: https://chromium-review.googlesource.com/1106143 Reviewed-by: Rohit Rao <rohitrao@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#568444} [modify] https://crrev.com/e0e4ea992d21f5abf688c505c8f9751f62ccd207/components/sync_sessions/session_sync_bridge.cc [modify] https://crrev.com/e0e4ea992d21f5abf688c505c8f9751f62ccd207/components/sync_sessions/session_sync_bridge_unittest.cc
,
Jun 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d5a28e18c3d64bcdb9273a3dc553611b76e256cb commit d5a28e18c3d64bcdb9273a3dc553611b76e256cb Author: Rohit Rao <rohitrao@chromium.org> Date: Tue Jun 19 16:13:36 2018 [ios] Fixes an infinite sync loop in RecentTabsMediator. |reloadSessions| previously had code to trigger another sync refresh. The old sync sessions implementation would detect that nothing had changed and break the update cycle, but a new implementation notifies observers unconditionally. This was leading to an infinite loop of sync updates. BUG= 854049 Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I85dbf2810f5ff6e009812a33ad643fb54cfdc8c4 Reviewed-on: https://chromium-review.googlesource.com/1106001 Reviewed-by: Sergio Collazos <sczs@chromium.org> Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Rohit Rao <rohitrao@chromium.org> Cr-Commit-Position: refs/heads/master@{#568476} [modify] https://crrev.com/d5a28e18c3d64bcdb9273a3dc553611b76e256cb/ios/chrome/browser/ui/ntp/recent_tabs/legacy_recent_tabs_table_coordinator.mm [modify] https://crrev.com/d5a28e18c3d64bcdb9273a3dc553611b76e256cb/ios/chrome/browser/ui/recent_tabs/recent_tabs_coordinator.mm [modify] https://crrev.com/d5a28e18c3d64bcdb9273a3dc553611b76e256cb/ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.h [modify] https://crrev.com/d5a28e18c3d64bcdb9273a3dc553611b76e256cb/ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.mm
,
Jun 19 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by rohitrao@chromium.org
, Jun 19 2018