Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
957753002:
Install default apps if sync is disabled. (Closed)
|
Created:
5 years, 10 months ago by Ivan Podogov Modified:
5 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInstall default apps if sync is disabled.
BUG=424075
TEST=manual
Committed: https://crrev.com/f66fae52d362f07dcd71ec0fef111c3e1653ad84
Cr-Commit-Position: refs/heads/master@{#318894}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Check both states. #
Total comments: 7
Patch Set 3 : Robustify a bit. #
Total comments: 5
Patch Set 4 : Don't use debug defines. #
Total comments: 2
Patch Set 5 : Check if sync is enabled locally. #
Total comments: 2
Patch Set 6 : Check for sync autostart. #
Messages
Total messages: 24 (5 generated)
ginkage@chromium.org changed reviewers: + asargent@chromium.org, dpolukhin@chromium.org
PTAL
lgtm https://codereview.chromium.org/957753002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/external_pref_loader.cc:145: ProfileSyncService::SyncStatusSummary::NOT_ENABLED) { I would also do the same in case of unrecoverable error.
ginkage@chromium.org changed reviewers: + atwilson@chromium.org
https://codereview.chromium.org/957753002/diff/1/chrome/browser/extensions/ex... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/1/chrome/browser/extensions/ex... chrome/browser/extensions/external_pref_loader.cc:145: ProfileSyncService::SyncStatusSummary::NOT_ENABLED) { On 2015/02/25 13:39:11, Dmitry Polukhin wrote: > I would also do the same in case of unrecoverable error. Done.
atwilson@chromium.org changed reviewers: + zea@chromium.org
Nicolas should take a look to make sure the right behavior is happening re: checking for fatal sync errors. https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:124: ProfileSyncServiceFactory::GetForProfile(profile_); You can't depend on ProfileSyncService existing, as it could be disabled by a cmd-line flag or via GPO. I think this code will cause a crash if you run with --disable-sync. Maybe try calling profile->IsSyncAccessible() first, and if not, treat this like sync is disabled. https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:147: status == ProfileSyncService::SyncStatusSummary::UNRECOVERABLE_ERROR) { Not sure if perhaps you should be checking for UNKNOWN_ERROR or some of the other error states? Perhaps ping nzea?
https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:124: ProfileSyncServiceFactory::GetForProfile(profile_); On 2015/02/25 14:47:31, Andrew T Wilson wrote: > You can't depend on ProfileSyncService existing, as it could be disabled by a > cmd-line flag or via GPO. I think this code will cause a crash if you run with > --disable-sync. > > Maybe try calling profile->IsSyncAccessible() first, and if not, treat this like > sync is disabled. Funny thing is, IsSyncAccessible() call would also crash in that case. I've adjusted patch a bit, so that it would behave better, but then again, on test accounts I have I get some crashes even on an unpatched master when using --disable-sync. https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:147: status == ProfileSyncService::SyncStatusSummary::UNRECOVERABLE_ERROR) { On 2015/02/25 14:47:31, Andrew T Wilson wrote: > Not sure if perhaps you should be checking for UNKNOWN_ERROR or some of the > other error states? Perhaps ping nzea? UNKNOWN_ERROR corresponds to some state that even ProfileSyncService authors are not sure it is possible to be in... I'm not sure what course of action would be the best in that case.
https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:124: ProfileSyncServiceFactory::GetForProfile(profile_); On 2015/02/25 16:34:14, Ivan Podogov wrote: > On 2015/02/25 14:47:31, Andrew T Wilson wrote: > > You can't depend on ProfileSyncService existing, as it could be disabled by a > > cmd-line flag or via GPO. I think this code will cause a crash if you run with > > --disable-sync. > > > > Maybe try calling profile->IsSyncAccessible() first, and if not, treat this > like > > sync is disabled. > > Funny thing is, IsSyncAccessible() call would also crash in that case. I've > adjusted patch a bit, so that it would behave better, but then again, on test > accounts I have I get some crashes even on an unpatched master when using > --disable-sync. If you find those, please file bugs. These regress sometimes and we need to fix them. I don't think IsSyncAccessible() should ever crash - what's the crash you are seeing when calling that? https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:147: status == ProfileSyncService::SyncStatusSummary::UNRECOVERABLE_ERROR) { On 2015/02/25 16:34:14, Ivan Podogov wrote: > On 2015/02/25 14:47:31, Andrew T Wilson wrote: > > Not sure if perhaps you should be checking for UNKNOWN_ERROR or some of the > > other error states? Perhaps ping nzea? > > UNKNOWN_ERROR corresponds to some state that even ProfileSyncService authors are > not sure it is possible to be in... I'm not sure what course of action would be > the best in that case. What I want to make sure is we aren't left in a state where we never load default apps. What happens if you treat it like an unrecoverable error (what's the worst case scenario?)
https://codereview.chromium.org/957753002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/40001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:148: if (status == ProfileSyncService::SyncStatusSummary::NOT_ENABLED || Do you have logic elsewhere to do the load when sync finishes? Or is the idea to not do the load at all if sync is running? Either way, this enum is primarily for debugging UI, not so much knowing sync internal state. IsSyncEnabledAndLoggedIn() && HasSyncSetupCompleted() would be better, and it also not transient based on the state of the backend (which can have its startup delayed by up to 11 seconds).
CL updated once again. https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/20001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:124: ProfileSyncServiceFactory::GetForProfile(profile_); On 2015/02/25 17:47:22, Andrew T Wilson wrote: > On 2015/02/25 16:34:14, Ivan Podogov wrote: > > On 2015/02/25 14:47:31, Andrew T Wilson wrote: > > > You can't depend on ProfileSyncService existing, as it could be disabled by > a > > > cmd-line flag or via GPO. I think this code will cause a crash if you run > with > > > --disable-sync. > > > > > > Maybe try calling profile->IsSyncAccessible() first, and if not, treat this > > like > > > sync is disabled. > > > > Funny thing is, IsSyncAccessible() call would also crash in that case. I've > > adjusted patch a bit, so that it would behave better, but then again, on test > > accounts I have I get some crashes even on an unpatched master when using > > --disable-sync. > > If you find those, please file bugs. These regress sometimes and we need to fix > them. > > I don't think IsSyncAccessible() should ever crash - what's the crash you are > seeing when calling that? My bad, I mistook somethig: profile was nullptr, and it was not a crash, but a mere DCHECK fail with SIGTRAP, that happens independently of --disable-sync... Brought the IsSyncAccessible back, it should be enough to keep service available in all cases. https://codereview.chromium.org/957753002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/40001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:148: if (status == ProfileSyncService::SyncStatusSummary::NOT_ENABLED || On 2015/02/25 18:26:14, Nicolas Zea wrote: > Do you have logic elsewhere to do the load when sync finishes? Or is the idea to > not do the load at all if sync is running? > > Either way, this enum is primarily for debugging UI, not so much knowing sync > internal state. IsSyncEnabledAndLoggedIn() && HasSyncSetupCompleted() would be > better, and it also not transient based on the state of the backend (which can > have its startup delayed by up to 11 seconds). The main idea is: if sync is enabled (and we don't know that until the sync service connects to the server, thus IsSyncEnabled won't do the trick, but IsSyncEnabledAndLoggedIn does), then we must wait until priority sync is done (that case is handled in OnIsSyncingChanged function above, and HasSyncSetupCompleted is simply not sufficient in this case, we must wait a bit more than that), otherwise we will catch the "sync disabled" server response here. When the sync is disabled on the device itself, we will never reach this function, so that is covered as well.
extensions lgtm once zea@ has no remaining concerns
https://codereview.chromium.org/957753002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/40001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:125: if (service) I don't think this will catch the case where sync is disabled at startup time, i.e. if the user has never enabled sync. This is where I think you should be checking IsSyncEnabledAndLoggedIn && HasSyncSetupCompleted, before registering any observers. If those two are not true, sync will not be running, at least at startup, and will only trigger observer if it's enabled (which is not guaranteed to happen at all). https://codereview.chromium.org/957753002/diff/40001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:148: if (status == ProfileSyncService::SyncStatusSummary::NOT_ENABLED || On 2015/02/26 12:50:15, Ivan Podogov wrote: > On 2015/02/25 18:26:14, Nicolas Zea wrote: > > Do you have logic elsewhere to do the load when sync finishes? Or is the idea > to > > not do the load at all if sync is running? > > > > Either way, this enum is primarily for debugging UI, not so much knowing sync > > internal state. IsSyncEnabledAndLoggedIn() && HasSyncSetupCompleted() would be > > better, and it also not transient based on the state of the backend (which can > > have its startup delayed by up to 11 seconds). > > The main idea is: if sync is enabled (and we don't know that until the sync > service connects to the server, thus IsSyncEnabled won't do the trick, but > IsSyncEnabledAndLoggedIn does), then we must wait until priority sync is done > (that case is handled in OnIsSyncingChanged function above, and > HasSyncSetupCompleted is simply not sufficient in this case, we must wait a bit > more than that), otherwise we will catch the "sync disabled" server response > here. When the sync is disabled on the device itself, we will never reach this > function, so that is covered as well. I'm a bit confused by this; I think we might be thinking of "enabled" in two different ways. You do know if sync is currently enabled without hitting the server, in the sense that you know whether sync is going to attempt to start up and sync. (see my comment above) That said, sync can, effectively at any time, hit a "permanent error" which will subsequently disable sync. Registering to listen to those events is fine. IsSyncEnabledAndLoggedIn will return false once we've hit a permanent error. Does that make things clearer?
https://codereview.chromium.org/957753002/diff/40001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/40001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:148: if (status == ProfileSyncService::SyncStatusSummary::NOT_ENABLED || On 2015/02/26 21:46:47, Nicolas Zea wrote: > On 2015/02/26 12:50:15, Ivan Podogov wrote: > > On 2015/02/25 18:26:14, Nicolas Zea wrote: > > > Do you have logic elsewhere to do the load when sync finishes? Or is the > idea > > to > > > not do the load at all if sync is running? > > > > > > Either way, this enum is primarily for debugging UI, not so much knowing > sync > > > internal state. IsSyncEnabledAndLoggedIn() && HasSyncSetupCompleted() would > be > > > better, and it also not transient based on the state of the backend (which > can > > > have its startup delayed by up to 11 seconds). > > > > The main idea is: if sync is enabled (and we don't know that until the sync > > service connects to the server, thus IsSyncEnabled won't do the trick, but > > IsSyncEnabledAndLoggedIn does), then we must wait until priority sync is done > > (that case is handled in OnIsSyncingChanged function above, and > > HasSyncSetupCompleted is simply not sufficient in this case, we must wait a > bit > > more than that), otherwise we will catch the "sync disabled" server response > > here. When the sync is disabled on the device itself, we will never reach this > > function, so that is covered as well. > > I'm a bit confused by this; I think we might be thinking of "enabled" in two > different ways. You do know if sync is currently enabled without hitting the > server, in the sense that you know whether sync is going to attempt to start up > and sync. (see my comment above) > > That said, sync can, effectively at any time, hit a "permanent error" which will > subsequently disable sync. Registering to listen to those events is fine. > IsSyncEnabledAndLoggedIn will return false once we've hit a permanent error. > > Does that make things clearer? Um... These comments are for the old patchset, take a look at the last one (from 9 hours ago), please.
Oops, missed that. That said, I still don't think your code is properly handling the fact that sync will never notify observers if it never starts up (this is completely independent of whether IsSyncAccessible returns true or not). https://codereview.chromium.org/957753002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/60001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:127: service->AddObserver(this); You're still signing up for events that may never happen if sync is not actually going to run. See previous comment here.
https://codereview.chromium.org/957753002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/60001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:127: service->AddObserver(this); On 2015/02/26 22:03:47, Nicolas Zea wrote: > You're still signing up for events that may never happen if sync is not actually > going to run. See previous comment here. You wrote: "This is where I think you should be checking IsSyncEnabledAndLoggedIn && HasSyncSetupCompleted, before registering any observers." Problem is, HasSyncSetupCompleted is false in this line, even when sync is enabled: this function is called a little bit too early. Although, IsSyncEnabledAndLoggedIn is true, so we bight still check for that. Will that be fine (see the next patchset)?
https://codereview.chromium.org/957753002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/80001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:127: if (service->IsSyncEnabledAndLoggedIn()) Is this code only running in ChromeOS? if so, this should I believe be fine. If not though, I think you need to check: service->IsSyncEnabledAndLoggedIn() && (HasSyncSetupCompleted() || browser_defaults::kSyncAutoStarts) The issue that I was forgetting earlier is that on ChromeOS we check browser_defaults::kSyncAutoStarts and automatically set SyncSetupCompleted based on that, but on other platforms the user can explicitly disable sync (or just never enable it).
https://codereview.chromium.org/957753002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/external_pref_loader.cc (right): https://codereview.chromium.org/957753002/diff/80001/chrome/browser/extension... chrome/browser/extensions/external_pref_loader.cc:127: if (service->IsSyncEnabledAndLoggedIn()) On 2015/02/27 19:25:00, Nicolas Zea wrote: > Is this code only running in ChromeOS? if so, this should I believe be fine. > > If not though, I think you need to check: > service->IsSyncEnabledAndLoggedIn() && (HasSyncSetupCompleted() || > browser_defaults::kSyncAutoStarts) > > The issue that I was forgetting earlier is that on ChromeOS we check > browser_defaults::kSyncAutoStarts and automatically set SyncSetupCompleted based > on that, but on other platforms the user can explicitly disable sync (or just > never enable it). I'm not really sure of where this code may run, but since it is not in a ChromeOS-specific path anyway, I suppose it is better to be a little more careful... New patchset ready.
LGTM % one suggestion. This logic is complex and subtle enough that it might merit a full integration test with a proper sync engine running to ensure it doesn't regress. That can be a bit of work, so not going to hold up this patch for it. Feel free to chat with pvalenzuela@ about how to get a test going for this.
The CQ bit was checked by ginkage@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpolukhin@chromium.org, asargent@chromium.org Link to the patchset: https://codereview.chromium.org/957753002/#ps100001 (title: "Check for sync autostart.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/957753002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/f66fae52d362f07dcd71ec0fef111c3e1653ad84 Cr-Commit-Position: refs/heads/master@{#318894} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
