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

Issue 915165 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug-Regression



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

Project Member Reported by rkalavakuntla@chromium.org, Dec 14

Issue description

Chrome 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




 
Actual.png
527 KB View Download
Actual.mp4
6.8 MB View Download
Expected.mp4
2.7 MB View Download
Cc: treib@chromium.org jkrcal@chromium.org
Status: Available (was: Untriaged)
Marc, sorry to redirect all bugs to you: 
could it be related to your changes in ProfileSyncService?
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
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

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...
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
Labels: Needs-Bisect
Can we get a bisect for this?
Labels: -Needs-Bisect
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!
There's almost 3700 CLs between those builds... I guess we'll have to run a manual bisect, or just debug directly.
Labels: Sync-Triaged
Labels: FixitSync
Owner: mastiz@chromium.org
Status: Started (was: Available)
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
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
I'm starting a manual bisect.
Cc: mastiz@chromium.org
Owner: dpa...@chromium.org
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

Labels: -Pri-1 Pri-2
Status: Assigned (was: Started)
> 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.
Owner: johntlee@chromium.org
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.
sync.mp4
119 KB View Download
Project Member

Comment 17 by bugdroid1@chromium.org, 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

Comment 18 by johntlee@chromium.org, Jan 16 (6 days ago)

Status: Fixed (was: Assigned)

Sign in to add a comment