Issue metadata
Sign in to add a comment
|
Regression: Unnecessary glimpse of "Sync is disabled by your administrator" text is seen when we open advanced sync settings to enter passphrase |
||||||||||||||||||||||
Issue descriptionChrome Version: 73.0.3639.0/11398.0.0 dev channel Daisy,Kip,Reks OS:chrome OS What steps will reproduce the problem? (1)Sign into user with an account that asks for Sync passphrase (2)Click on the sync passphrase error message and observe the advanced sync settings page while it opens(Please refer video) Actual:Unnecessary glimpse of "Sync is disabled by your administrator" text is seen Expected: No such unnecessary glimpse of "Sync is disabled by your administrator" text should be seen when we open advanced sync settings to enter passphrase This is a Regression issue as same works fine in 72.0.3626.15/ 11316.18.0 dev
,
Dec 14
The string seems to be IDS_SIGNED_IN_WITH_SYNC_DISABLED (there's an almost-duplicate, but only this one has the trailing "."). That gets returns here [1] if either the SyncService is null or it has DISABLE_REASON_ENTERPRISE_POLICY. That disable reason seems unlikely to occur "randomly", so maybe we somehow have a null SyncService here? It's certainly possible that some of my changes somehow influenced this, though I don't immediately see how. [1] https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?rcl=96147a54c6fcc445846b2a7163fcc1145613f1b2&l=41
,
Dec 17
It seems like the relevant place is here [2], called from the WebUI when the page gets loaded. The service is grabbed by
syncer::SyncService* service =
ProfileSyncServiceFactory::GetSyncServiceForBrowserContext(profile_);
The PeopleHandler listens to various notifications from the service such as StateChanged() / OnPrimaryAccountSet() / OnPrimaryAccountCleared(), which then fixes the issue (it goes through the same code path, though, again requesting the service from the factory).
It seems like the first call to the factory must return nullptr. Tried to look into the factory code but could not spot anything weird. Marc, does it ring a bell?
[2] https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/people_handler.cc?l=812
,
Dec 17
A potential work around would be to return a different string if SyncService is nullptr. Not sure if this would break any other use case. This would not solve the glimpse but at least the text would not be misleading. A proper fix is of course to get rid of the glipse...
,
Dec 17
The factory should only return nullptr if ProfileSyncService::IsSyncAllowedByFlag() is false [3]. If that were the case, then it wouldn't go away. [3] https://cs.chromium.org/chromium/src/chrome/browser/sync/profile_sync_service_factory.cc?rcl=26f2a38310463295809c84c57c64a8fe98f7afb8&l=121
,
Dec 18
Can we get a bisect for this?
,
Dec 18
C #6> Tool bisect is not available at our end. Hence providing manual bisect info: Good build:72.0.3626.15/ 11316.18.0 Bad build :73.0.3639.0/11398.0.0 Thanks!
,
Dec 18
There's almost 3700 CLs between those builds... I guess we'll have to run a manual bisect, or just debug directly.
,
Dec 18
,
Jan 8
I'll take a stab at this. Some initial thoughts: - This comment ([1]) in sync_ui_util.cc seems inaccurate because |service| may be null, which would display an error message. - For the only relevant calling site, which is sync_ui_util::GetStatusLabels(), the sync service is grabbed in PeopleHandler::GetSyncStatusDictionary() by using ProfileSyncServiceFactory::GetSyncServiceForProfile(profile_) which shouldn't return null as mentioned by treib@. [1] https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?l=258&rcl=3cf9cd8e07ce48b301c6d86e5dead8376ecf765b
,
Jan 8
I try to repro and came to the following conclusion: most code pointers referenced in above comments are irrelevant because they don't even get exercised. I think the string shows up because it's the default value in people_page/sync_page.html [1]. I will investigate what could cause this. [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people_page/sync_page.html?l=118&rcl=3e71d144f9264edb23ed6662de1a64adc7670948
,
Jan 8
I'm starting a manual bisect.
,
Jan 8
Bisecting points to https://chromium-review.googlesource.com/c/1324915 by dpapad@: "Settings WebUI: Remove card and page/subpage animations." It's unclear to me whether the issue was there beforehand, but was impossible to notice due to animations. dpapad@: any ideas about how to go about this? Here's the full bisect log: git bisect start # bad: [4050a80ff0d1eada91aed7ee236d8ce5c266a417] Publish DEPS for 73.0.3639.0 git bisect bad 4050a80ff0d1eada91aed7ee236d8ce5c266a417 # good: [fd33170141d5c61177a2fe5a456498d823b0237f] Publish DEPS for 72.0.3626.0 git bisect good fd33170141d5c61177a2fe5a456498d823b0237f # good: [d897fb137fbaaa9355c0c93124cc048824eb1e65] Add nigeltao@chromium.org to app_service/OWNERS git bisect good d897fb137fbaaa9355c0c93124cc048824eb1e65 # good: [6562ea1bc24eddee0a22b8d8d64614e69b3792c2] Update comment after CL:1364970 git bisect good 6562ea1bc24eddee0a22b8d8d64614e69b3792c2 # bad: [7c69b5099532dcbf5269c3016d20bd3c43c607a4] [Snowflake] Move presubmit checks to check XMLs on multiple directories git bisect bad 7c69b5099532dcbf5269c3016d20bd3c43c607a4 # good: [a58093bf1ff2b9320d6083dd27556045e8ef69d3] Make XR browser tests use executable-relative paths git bisect good a58093bf1ff2b9320d6083dd27556045e8ef69d3 # good: [fa8fd965900a1ae66630aa9b36f1e837178f32e7] v8binding: Invoke on{before,after}print event listeners even when paused git bisect good fa8fd965900a1ae66630aa9b36f1e837178f32e7 # bad: [7187f86e3cfd06220ef7f36ee0efc809988cd45c] Don't prefix absolute paths in gn_run_binary.py. git bisect bad 7187f86e3cfd06220ef7f36ee0efc809988cd45c # bad: [c86d3822692fdb802047c283009c82f7e35cecc5] Closure compile multi_metadata_provider_unittest git bisect bad c86d3822692fdb802047c283009c82f7e35cecc5 # good: [2e9864817aee821cd2698b6b87610d4525a51059] Roll src/third_party/skia 27d5e8123477..1a592c2a3dee (1 commits) git bisect good 2e9864817aee821cd2698b6b87610d4525a51059 # bad: [fea7c851b80582534e5c4318996525bd42e50c30] Settings WebUI: Fix overscroll behavior corner cases. git bisect bad fea7c851b80582534e5c4318996525bd42e50c30 # good: [0668cb24e7364f5fc98d2af2ad58832fc8e58c9e] Temporary suppression of WebGL Multiview test. git bisect good 0668cb24e7364f5fc98d2af2ad58832fc8e58c9e # good: [39d4a75a1ef2eb572a779f36c6f1f0a11be86d75] FilesApp crostini: make crostini-files flag visible to users git bisect good 39d4a75a1ef2eb572a779f36c6f1f0a11be86d75 # bad: [cae9c16c2ce08055ef583f3de6ac259927e665dc] Roll src-internal a80aac83cf1d..d91c806161eb (1 commits) git bisect bad cae9c16c2ce08055ef583f3de6ac259927e665dc
,
Jan 11
> It's unclear to me whether the issue was there beforehand That was the case. The animations removal just reveals this flicker. I'll look for a way to remove the flicker, but I don't think this should be a P1.
,
Jan 11
,
Jan 12
Hid the glimpse of "Sync is disabled by your admin...". There is still a flicker of "Please wait..." while the data loads but that looks like it's intentional, so I've kept it.
,
Jan 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/05e587c19f490abee0c7b1a740b40827ffb0f268 commit 05e587c19f490abee0c7b1a740b40827ffb0f268 Author: John Lee <johntlee@chromium.org> Date: Wed Jan 16 04:27:55 2019 Settings WebUI: Hide "sync disabled" warning until data has been loaded. Bug: 915165 Change-Id: I02e911d45559a70d244f0106e940ad0986a95b87 Reviewed-on: https://chromium-review.googlesource.com/c/1407912 Commit-Queue: John Lee <johntlee@chromium.org> Reviewed-by: Hector Carmona <hcarmona@chromium.org> Cr-Commit-Position: refs/heads/master@{#623102} [modify] https://crrev.com/05e587c19f490abee0c7b1a740b40827ffb0f268/chrome/browser/resources/settings/people_page/sync_page.html [modify] https://crrev.com/05e587c19f490abee0c7b1a740b40827ffb0f268/chrome/browser/resources/settings/people_page/sync_page.js
,
Jan 16
(6 days ago)
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by jkrcal@chromium.org
, Dec 14Status: Available (was: Untriaged)