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

Issue metadata

Status: Fixed
Owner:
Closed: Nov 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Task



Sign in to add a comment
link

Issue 867901: Improve ProfileSyncService's "PlatformSyncAllowedProvider" handling

Reported by treib@chromium.org, Jul 26 2018 Project Member

Issue description

PlatformSyncAllowedProvider is a callback that returns the state of Android's "Master Sync" toggle. There are several things wrong with this system:

- The callback is passed in after PSS is first initialized, so at that point PSS might have already started up. At this point, we don't even check the value of the callback.
- PSS doesn't know when its value changes, so the Android code anyway has to call RequestStart/Stop as appropriate.

So having a callback and all the necessary plumbing seems useless; might as well just have a SetSyncAllowedByPlatform(bool allowed).

Related:   bug 566134 ,  bug 568771 
 

Comment 1 by treib@chromium.org, Jul 26 2018

One confusing thing caused by all this is that DISABLE_REASON_PLATFORM_OVERRIDE and DISABLE_REASON_USER_CHOICE get quite tangled: When Android's MasterSync gets turned off, we call RequestStop, which also sets IsSyncRequested to false (i.e. DISABLE_REASON_USER_CHOICE).
It all seems to work out correctly somehow, since the UI is very careful about when to call RequestStart. E.g. first turning Sync off in Chrome settings (->USER_CHOICE), then turning MasterSync off and on again does *not* cause a call to RequestStart.

Comment 2 by treib@chromium.org, Jul 26 2018

Re #1: This works because there are actually *two* Android-level Sync flags: "MasterSync" and "ChromeSync", and the Android code tries to keep the state of "ChromeSync" consistent with the SyncRequested flag [1]. That seems problematic because the "ChromeSync" flag is set both from SyncRequested and from some OS-level concept [2].

[1] https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java?rcl=2f3418b8ea95005302e1d8f916d3adf87d01ae47&l=182
[2] https://cs.chromium.org/chromium/src/components/sync/android/java/src/org/chromium/components/sync/AndroidSyncSettings.java?rcl=1dbc3a455c5f0420e2ba571ba9ee5819a172fc60&l=316

Comment 3 by treib@chromium.org, Oct 9

Description: Show this description

Comment 4 by bugdroid1@chromium.org, Oct 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0d63432c51741e6dc0bf48f0b84e4afb63f887f

commit f0d63432c51741e6dc0bf48f0b84e4afb63f887f
Author: Marc Treib <treib@chromium.org>
Date: Fri Oct 12 13:42:49 2018

ProfileSyncService cleanup: Remove platform_sync_allowed_provider_

ProfileSyncService::platform_sync_allowed_provider_ was a callback that
returned whether Android's "Master Sync" toggle is enabled, and so Sync
shouldn't start up. However, PSS didn't know when the value changed, so
the callback was kinda useless, and PSS could easily get into an
inconsistent state (where there is a disable reason, but Sync is still
running). Things were working out in practice only because the Android
UI code also called RequestStart/Stop as necessary.
So, since PSS needs change notifications anyway, let's make that
explicit: This CL replaces the callback by a bool, which can be set
explicitly.

Bug:  867901 
Change-Id: I5b7e9f4657d1f565d9643984eb52ec1df26b5c90
Reviewed-on: https://chromium-review.googlesource.com/c/1269870
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599195}
[modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
[modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/chrome/android/java/src/org/chromium/chrome/browser/sync/SyncController.java
[modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/chrome/browser/sync/profile_sync_service_android.cc
[modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/chrome/browser/sync/profile_sync_service_android.h
[modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/f0d63432c51741e6dc0bf48f0b84e4afb63f887f/components/browser_sync/profile_sync_service.h

Comment 5 by treib@chromium.org, Oct 31

Cc: -treib@chromium.org
Owner: treib@chromium.org
Status: Started (was: Available)

Comment 6 by bugdroid1@chromium.org, Nov 6

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1012fccb7dd1741f016d405e8f614f31e07119b

commit d1012fccb7dd1741f016d405e8f614f31e07119b
Author: Marc Treib <treib@chromium.org>
Date: Tue Nov 06 17:10:30 2018

Stop sync when AllowedByPlatform is set to false

This currently has no practical effect (the only call site also calls
RequestStop anyway), but it prevents ProfileSyncService from getting
into an inconsistent state.

Bug:  867901 
Change-Id: Iaaa72405eac6fb81ee739d2cbeea6ddf52960996
Reviewed-on: https://chromium-review.googlesource.com/c/1319716
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605716}
[modify] https://crrev.com/d1012fccb7dd1741f016d405e8f614f31e07119b/components/browser_sync/profile_sync_service.cc

Comment 7 by treib@chromium.org, Nov 6

Status: Fixed (was: Started)
Let's call this close enough. There's still the issue of the two disable reasons getting tangled, but that's for another day...

Comment 8 by treib@chromium.org, Jan 11

Filed bug 921025 for untangling the two toggles.

Sign in to add a comment