Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1240573012:
Extension syncing: Introduce a NeedsSync pref (Closed)
|
Created:
5 years, 5 months ago by Marc Treib Modified:
5 years, 5 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ext_sync_uninstall Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtension syncing: Introduce a NeedsSync pref
that indicates the extension has local changes that still need to be synced.
It's set when something changes before sync is ready, and cleared once the extension state has been synced.
This should handle all conflicts between sync and local state reasonably well, and as a bonus allows us to get rid of the (weird and not-really-working) PendingEnables class.
BUG=509990
Committed: https://crrev.com/c349453205678fda3d6a8a7f1a90a5cec56b8216
Cr-Commit-Position: refs/heads/master@{#339651}
Patch Set 1 #
Total comments: 21
Patch Set 2 : rebase #Patch Set 3 : review #Patch Set 4 : more cleanup; fix test #
Total comments: 16
Patch Set 5 : review2 #Patch Set 6 : fix sync_integration_tests build #Patch Set 7 : ...and browser_tests #
Total comments: 11
Patch Set 8 : review #Patch Set 9 : rebase (and comment fixes) #Patch Set 10 : fix #Patch Set 11 : (b); hackfix sync_integration_tests #
Total comments: 4
Messages
Total messages: 26 (7 generated)
treib@chromium.org changed reviewers: + kalman@chromium.org
First shot at the new pref, PTAL! (This CL is based on https://codereview.chromium.org/1236363002, but basically unrelated.)
Some non-trivial comments. It might be nice to land that further cleanup I mentioned in the other patch first, since it's partly orthogonal. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:84: result.reserve(data.size()); You should be able to do this in the constructor; syncer::SyncDataList result(data.size(), ExtensionSyncData()); (or maybe the arguments are reversed). https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:148: ExtensionPrefs::Get(profile_)->SetNeedsSync(extension.id(), false); I think this only needs to go at the point when the extensions that need syncing are pushed out, not here? https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:149: } else if (extension_service_->is_ready() && !flare_.is_null()) { It seems like these 2 cases should be separated. 1. if the bundle is syncing, push the sync change. If not, mark it as needing syncing in prefs. 2. if the sync flare hasn't been run yet, and the extension system is ready, run the flare. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:236: if (ExtensionPrefs::Get(profile_)->NeedsSync(id)) I don't think this is the right place for this. Instead we should be filtering changes to push back out to sync based on whether the pref is set... sort of a reversal of the logic. Basically up on line 173 the call "bundle->PushSyncDataList" should only contain the extensions that have changed. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:379: bundle->IsSyncing() && Should this be a (D)CHECK? https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/s... File chrome/browser/extensions/sync_bundle.h (right): https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/s... chrome/browser/extensions/sync_bundle.h:54: void PushSyncChange(const std::string& extension_id, I see that some of my comments in the other CL were done in here anyway. Nice :-) https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/s... chrome/browser/extensions/sync_bundle.h:91: std::map<std::string, ExtensionSyncData> pending_sync_data_; Could you add comments for this and |synced_extensions_|? I'm particularly interested in |pending_sync_data_| because there is a lot of code dealing with that, and it seems to come down to returning false from ApplyExtensionSyncDataHelper (as we've established before) and I don't understand why we want to hold onto those changes. Would really simplify things to delete it. Next time, I suppose. https://codereview.chromium.org/1240573012/diff/1/extensions/browser/extensio... File extensions/browser/extension_prefs.cc (right): https://codereview.chromium.org/1240573012/diff/1/extensions/browser/extensio... extensions/browser/extension_prefs.cc:1864: return needs_sync; Looks like there is a helper to simplify this, "ReadPrefAsBooleanAndReturn".
https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:84: result.reserve(data.size()); On 2015/07/15 20:27:57, kalman wrote: > You should be able to do this in the constructor; syncer::SyncDataList > result(data.size(), ExtensionSyncData()); (or maybe the arguments are reversed). Actually no, that would do something different: It'd create |data.size()| empty ExtensionSyncDatas (which wouldn't work because the default constructor is private). reserve() OTOH just allocates memory, but doesn't construct the elements. Calling it at all isn't strictly necessary, it's just a small optimization to avoid some re-allocations. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:148: ExtensionPrefs::Get(profile_)->SetNeedsSync(extension.id(), false); On 2015/07/15 20:27:57, kalman wrote: > I think this only needs to go at the point when the extensions that need syncing > are pushed out, not here? I think you're right. By the time we get here, the pref should already have been cleared. I've put in a DCHECK instead. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:149: } else if (extension_service_->is_ready() && !flare_.is_null()) { On 2015/07/15 20:27:57, kalman wrote: > It seems like these 2 cases should be separated. > > 1. if the bundle is syncing, push the sync change. If not, mark it as needing > syncing in prefs. Done. > 2. if the sync flare hasn't been run yet, and the extension system is ready, run > the flare. I think this should still go in the "else - if the bundle is syncing already, there's no point in running the flare. Also, the extension_service_->is_ready() check seems redundant - we're being called by the extension service here, so it must be ready, right? https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:236: if (ExtensionPrefs::Get(profile_)->NeedsSync(id)) On 2015/07/15 20:27:57, kalman wrote: > I don't think this is the right place for this. Instead we should be filtering > changes to push back out to sync based on whether the pref is set... sort of a > reversal of the logic. Basically up on line 173 the call > "bundle->PushSyncDataList" should only contain the extensions that have changed. That's two different things. What MergeDataAndStartSyncing does is: 1. Apply the initial sync data. 2. Push out all the local state. You're proposing that in 2. we should only push what's actually changed (which makes sense, but is just an optimization). But what this code here does is during 1., filter out those sync changes for which we have more recent local changes. That's actually required to make all this work. I do agree that here isn't really the right place for it though. I've moved the check into SyncBundle::MergeDataAndStartSyncing. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:379: bundle->IsSyncing() && On 2015/07/15 20:27:57, kalman wrote: > Should this be a (D)CHECK? Not sure - can GetAllSyncData (from syncer::SyncableService) be called before MergeDataAndStartSyncing? Anyway, I don't see the point in the IsSyncing check. Maybe we should just remove it? https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/s... File chrome/browser/extensions/sync_bundle.h (right): https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/s... chrome/browser/extensions/sync_bundle.h:91: std::map<std::string, ExtensionSyncData> pending_sync_data_; On 2015/07/15 20:27:57, kalman wrote: > Could you add comments for this and |synced_extensions_|? > > I'm particularly interested in |pending_sync_data_| because there is a lot of > code dealing with that, and it seems to come down to returning false from > ApplyExtensionSyncDataHelper (as we've established before) and I don't > understand why we want to hold onto those changes. Would really simplify things > to delete it. > > Next time, I suppose. Added comments to explain. I don't think we can get rid of them though :( https://codereview.chromium.org/1240573012/diff/1/extensions/browser/extensio... File extensions/browser/extension_prefs.cc (right): https://codereview.chromium.org/1240573012/diff/1/extensions/browser/extensio... extensions/browser/extension_prefs.cc:1864: return needs_sync; On 2015/07/15 20:27:57, kalman wrote: > Looks like there is a helper to simplify this, "ReadPrefAsBooleanAndReturn". Indeed! Thanks, done.
https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:5928: ASSERT_FALSE(service()->IsExtensionEnabled(good0)); Turns out I broke this test: Installing the extension before Sync is up will set NeedsSync. So the sync change to disable that comes into MergeDataAndStartSyncing will be ignored, which (I think) is fine, Sync shouldn't even know about this extension yet. To fix is simply to send a "proper" sync change after MergeDataAndStartSyncing. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:123: // See crbug.com/256795. I think we could handle this by setting NeedsSync on the uninstalled extension. (Do extension prefs survive the uninstallation?) Then in MergeDataAndStartSyncing, we can interpret "NeedsSync is set but the extension isn't installed" as "we need to send a deletion to sync". [BTW, I think this is what caused the problems with the KMOO extension.]
btw the thing about the uninstalled extension tombstone makes sense in a follow-up. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:149: } else if (extension_service_->is_ready() && !flare_.is_null()) { On 2015/07/16 09:22:34, Marc Treib wrote: > On 2015/07/15 20:27:57, kalman wrote: > > It seems like these 2 cases should be separated. > > > > 1. if the bundle is syncing, push the sync change. If not, mark it as needing > > syncing in prefs. > > Done. > > > 2. if the sync flare hasn't been run yet, and the extension system is ready, > run > > the flare. > > I think this should still go in the "else - if the bundle is syncing already, > there's no point in running the flare. > Also, the extension_service_->is_ready() check seems redundant - we're being > called by the extension service here, so it must be ready, right? Ok good point. I still think it shouldn't be in an else because it's not logically related, and that can be a source of bugs. As for the ready thing, I never really understood that, but it's not a matter of correlating between the existence of an extensionservice and whether it's ready. Though generally speaking we probably check ready() where we don't need to, and don't check it when we should. Eh. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:236: if (ExtensionPrefs::Get(profile_)->NeedsSync(id)) > 2. Push out all the local state. > You're proposing that in 2. we should only push what's actually changed (which > makes sense, but is just an optimization). In a way it's an optimisation, but only because code which pushes out "changes" for everything is grossly inefficient and the wrong contract from a sync interface perspective. The point of sync is that you only say what changed. Sure, sync will say "hey wait a minute you said this thing changed but it didn't, I'm going to ignore it" but it's still *wrong*. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:379: bundle->IsSyncing() && On 2015/07/16 09:22:34, Marc Treib wrote: > On 2015/07/15 20:27:57, kalman wrote: > > Should this be a (D)CHECK? > > Not sure - can GetAllSyncData (from syncer::SyncableService) be called before > MergeDataAndStartSyncing? > Anyway, I don't see the point in the IsSyncing check. Maybe we should just > remove it? Removing SGTM. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:123: // See crbug.com/256795. On 2015/07/16 13:20:43, Marc Treib wrote: > I think we could handle this by setting NeedsSync on the uninstalled extension. > (Do extension prefs survive the uninstallation?) No, but IIRC there is a precedent for tombstone extension pref entries. I can't remember what, worth looking into[*], but I agree that having a tombstone extension pref entry with just the needs_sync bit set to true is close to the right thing. Another option to be to maintain a separate list, but I think a tombstone is simpler. > Then in MergeDataAndStartSyncing, we can interpret "NeedsSync is set but the > extension isn't installed" as "we need to send a deletion to sync". > [BTW, I think this is what caused the problems with the KMOO extension.] Yes that sounds likely. Ugh, good catch. [*] we used to do this for blacklists but the tombstone entries actually caused some funky bugs where they would get picked up as disabled, and then if you ever tried to re-enable them the prefs for that extension would get corrupted and you could never re-install that extension. Another place might be for external extensions that get uninstalled - there needs to be a record that you uninstalled it otherwise it's just going to get re-installed next chrome startup. A similar problem really. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:361: bool only_with_needs_sync) const { Sorry for nit but could we reverse this boolean? Make true mean to include all sync data, false to mean only include things that have changed. Something about that makes more sense to me. Maybe it's that true > false and all sync data > only needs sync. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.h (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.h:67: // Applies the change specified passed in by either ExtensionSyncData to the This applies a change *from the sync server* right, not some locally generated chane? Could you clarify that? https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/sync_bundle.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/sync_bundle.cc:35: sync_service_->ApplySyncData(*extension_sync_data); It's odd to me that we make this check here, but the corresponding code which gives it context is in ExtensionSyncService::MergeDataAndStartSyncing which calls this method. In fact can we just get rid of MergeDataAndStartSyncing here? Move it into ExtensionSyncService::MergeDataAndStartSyncing? https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/sync_bundle.cc:96: AddSyncedExtension(extension_sync_data->id()); Seems like the wrong place to be adding/removing the extensions. Should be the job of just observing the ExtensionRegistry. In fact could you replace AddSyncExtension/RemoveSyncedExtension with querying the ExtensionRegistry then checking that util::ShouldSync method? Not something for right now...
https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:149: } else if (extension_service_->is_ready() && !flare_.is_null()) { On 2015/07/16 18:25:46, kalman wrote: > On 2015/07/16 09:22:34, Marc Treib wrote: > > On 2015/07/15 20:27:57, kalman wrote: > > > It seems like these 2 cases should be separated. > > > > > > 1. if the bundle is syncing, push the sync change. If not, mark it as > needing > > > syncing in prefs. > > > > Done. > > > > > 2. if the sync flare hasn't been run yet, and the extension system is ready, > > run > > > the flare. > > > > I think this should still go in the "else - if the bundle is syncing already, > > there's no point in running the flare. > > Also, the extension_service_->is_ready() check seems redundant - we're being > > called by the extension service here, so it must be ready, right? > > Ok good point. I still think it shouldn't be in an else because it's not > logically related, and that can be a source of bugs. As for the ready thing, I > never really understood that, but it's not a matter of correlating between the > existence of an extensionservice and whether it's ready. Hm, turns out there's a test (ExtensionServiceTest.DeferredSyncStartupOnInstall) that makes sure the flare isn't run after sync has started. So maybe it actually matters? I've left it in the "!IsSyncing" branch for now... > Though generally speaking we probably check ready() where we don't need to, and > don't check it when we should. Eh. https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:236: if (ExtensionPrefs::Get(profile_)->NeedsSync(id)) On 2015/07/16 18:25:46, kalman wrote: > > > 2. Push out all the local state. > > You're proposing that in 2. we should only push what's actually changed (which > > makes sense, but is just an optimization). > > In a way it's an optimisation, but only because code which pushes out "changes" > for everything is grossly inefficient and the wrong contract from a sync > interface perspective. The point of sync is that you only say what changed. > Sure, sync will say "hey wait a minute you said this thing changed but it > didn't, I'm going to ignore it" but it's still *wrong*. Still "just" an optimization ;P Anyway, done in PS4 already :) https://codereview.chromium.org/1240573012/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_sync_service.cc:379: bundle->IsSyncing() && On 2015/07/16 18:25:46, kalman wrote: > On 2015/07/16 09:22:34, Marc Treib wrote: > > On 2015/07/15 20:27:57, kalman wrote: > > > Should this be a (D)CHECK? > > > > Not sure - can GetAllSyncData (from syncer::SyncableService) be called before > > MergeDataAndStartSyncing? > > Anyway, I don't see the point in the IsSyncing check. Maybe we should just > > remove it? > > Removing SGTM. Done. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:123: // See crbug.com/256795. On 2015/07/16 18:25:46, kalman wrote: > On 2015/07/16 13:20:43, Marc Treib wrote: > > I think we could handle this by setting NeedsSync on the uninstalled > extension. > > (Do extension prefs survive the uninstallation?) > > No, but IIRC there is a precedent for tombstone extension pref entries. I can't > remember what, worth looking into[*], but I agree that having a tombstone > extension pref entry with just the needs_sync bit set to true is close to the > right thing. > > Another option to be to maintain a separate list, but I think a tombstone is > simpler. > > > Then in MergeDataAndStartSyncing, we can interpret "NeedsSync is set but the > > extension isn't installed" as "we need to send a deletion to sync". > > [BTW, I think this is what caused the problems with the KMOO extension.] > > Yes that sounds likely. Ugh, good catch. > > > [*] we used to do this for blacklists but the tombstone entries actually caused > some funky bugs where they would get picked up as disabled, and then if you ever > tried to re-enable them the prefs for that extension would get corrupted and you > could never re-install that extension. Another place might be for external > extensions that get uninstalled - there needs to be a record that you > uninstalled it otherwise it's just going to get re-installed next chrome > startup. A similar problem really. I looked into ExtensionPrefs::IsExternalExtensionUninstalled, which is really very similar to what we need here - basically a tombstone. As far as I can tell, that one is just stored as an extension pref. So how does it survive the uninstallation? If ExtensionPrefs are cleared on uninstallation, is this one just set after the uninstall has happened, and so never gets cleared? That seems kinda hacky and fragile, but still, the same approach might work here... https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:361: bool only_with_needs_sync) const { On 2015/07/16 18:25:47, kalman wrote: > Sorry for nit but could we reverse this boolean? Make true mean to include all > sync data, false to mean only include things that have changed. Something about > that makes more sense to me. Maybe it's that true > false and all sync data > > only needs sync. Sure, done. Agreed that this makes it easier to read, somehow. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.h (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.h:67: // Applies the change specified passed in by either ExtensionSyncData to the On 2015/07/16 18:25:47, kalman wrote: > This applies a change *from the sync server* right, not some locally generated > chane? Could you clarify that? Yes, "apply" = server -> local, "push" = local -> server. Comment improved, hopefully. I also removed the "to be tried again" part, since that doesn't happen. (Also, with some of the SyncBundle changes, this doesn't need to be public anymore. It's not supposed to be part of the public interface, it was just called by the SyncBundle.) https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/sync_bundle.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/sync_bundle.cc:35: sync_service_->ApplySyncData(*extension_sync_data); On 2015/07/16 18:25:47, kalman wrote: > It's odd to me that we make this check here, but the corresponding code which > gives it context is in ExtensionSyncService::MergeDataAndStartSyncing which > calls this method. > > In fact can we just get rid of MergeDataAndStartSyncing here? Move it into > ExtensionSyncService::MergeDataAndStartSyncing? Not quite - this also calls AddSyncedExtension for all the items in initial_sync_data, which still needs to happen. I've moved the actual applying into ExtensionSyncService though and renamed this to Init. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/sync_bundle.cc:96: AddSyncedExtension(extension_sync_data->id()); On 2015/07/16 18:25:47, kalman wrote: > Seems like the wrong place to be adding/removing the extensions. Should be the > job of just observing the ExtensionRegistry. > > In fact could you replace AddSyncExtension/RemoveSyncedExtension with querying > the ExtensionRegistry then checking that util::ShouldSync method? Almost, but not quite :( There's some special code in ExtensionService so that uninstalls with UNINSTALL_REASON_REINSTALL aren't synced. > Not something for right now... Yup, I'll look into it. I'd really like to get rid of the almost-pointless synced_extensions_ list.
lgtm, thanks for sticking with this. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:123: // See crbug.com/256795. > I looked into ExtensionPrefs::IsExternalExtensionUninstalled, which is really > very similar to what we need here - basically a tombstone. As far as I can tell, > that one is just stored as an extension pref. So how does it survive the > uninstallation? If ExtensionPrefs are cleared on uninstallation, is this one > just set after the uninstall has happened, and so never gets cleared? That seems > kinda hacky and fragile, but still, the same approach might work here... Ok maybe a separate pref would be better after all, and if the tombstone for external extensions adds complexity we could use it there. Like, "extensions": { "needs_sync_deletion": ["dafhsldjkfhaljksdlf", "kfhdlajksdf"], ... } we can discuss this later. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/sync_bundle.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/sync_bundle.cc:35: sync_service_->ApplySyncData(*extension_sync_data); > Not quite - this also calls AddSyncedExtension for all the items in > initial_sync_data, which still needs to happen. I've moved the actual applying > into ExtensionSyncService though and renamed this to Init. Ah right, thanks. "StartSyncing" would be even better I think. https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:162: bundle->Init(sync_processor.Pass()); (See "Init" --> "StartSyncing" comment - it's more descriptive and still fixes the "MergeData"/interface-same-naming confusion) https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:169: if (extension_sync_data.get()) { (incidentally you don't need the .get() calls for scoped_ptr<> null checks, but it's not necessary to change it - whichever you prefer) https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:170: bundle->ApplySyncData(*extension_sync_data); Why isn't this guarded behind the ExtensionNeedsSync(...) check? In fact if you're not applying these sync changes (this->ApplySyncData) should you be updating that (as we established, silly) synced_extenison_list_ structure in SyncBundle at all? (i.e. should bundle->ApplySyncData call move into this->ApplySyncData) It would be even nice to change bundle->ApplySyncData into those 2 add/remove methods because bundle->ApplySyncData doesn't actually do anything with the sync data, which is misleading. However as established the best solution would be to remove that synced_extensions_ concept entirely, so you might as well hold off any further cleanup here until then. I am really puzzled by the whole thing (in fact I don't quite understand your response to the ExtensionRegistry comment but it's not important right now). https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:183: ExtensionPrefs::Get(profile_)->SetNeedsSync(data.id(), false); I find it a little bit weird that querying this bit gets its own method (ExtensionNeedsSync) while setting it doesn't. I'd have gone with just querying the prefs directly. Not that important though. https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:382: FillSyncDataList( Could you add: // TODO(trieb): Should we be including blacklisted/blocked extensions here? // I.e. just calling registry->GeneratedInstalledExtensionsSet()? // It would be more consistent, but the danger is that the black/blocklist // hasn't been updated on all clients by the time sync has kicked in - // so it's safest not to. Take care to add any other extension lists here // in the future if they are added. https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.h (right): https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.h:93: // to the pending list. "... They will be applied later when..?"
https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:123: // See crbug.com/256795. On 2015/07/17 15:28:50, kalman wrote: > > I looked into ExtensionPrefs::IsExternalExtensionUninstalled, which is really > > very similar to what we need here - basically a tombstone. As far as I can > tell, > > that one is just stored as an extension pref. So how does it survive the > > uninstallation? If ExtensionPrefs are cleared on uninstallation, is this one > > just set after the uninstall has happened, and so never gets cleared? That > seems > > kinda hacky and fragile, but still, the same approach might work here... > > Ok maybe a separate pref would be better after all, and if the tombstone for > external extensions adds complexity we could use it there. Like, > > "extensions": { > "needs_sync_deletion": ["dafhsldjkfhaljksdlf", "kfhdlajksdf"], > ... > } > > we can discuss this later. Acknowledged. https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... File chrome/browser/extensions/sync_bundle.cc (right): https://codereview.chromium.org/1240573012/diff/50001/chrome/browser/extensio... chrome/browser/extensions/sync_bundle.cc:35: sync_service_->ApplySyncData(*extension_sync_data); On 2015/07/17 15:28:50, kalman wrote: > > Not quite - this also calls AddSyncedExtension for all the items in > > initial_sync_data, which still needs to happen. I've moved the actual applying > > into ExtensionSyncService though and renamed this to Init. > > Ah right, thanks. "StartSyncing" would be even better I think. Done. https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:169: if (extension_sync_data.get()) { On 2015/07/17 15:28:50, kalman wrote: > (incidentally you don't need the .get() calls for scoped_ptr<> null checks, but > it's not necessary to change it - whichever you prefer) Done. https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:170: bundle->ApplySyncData(*extension_sync_data); On 2015/07/17 15:28:50, kalman wrote: > Why isn't this guarded behind the ExtensionNeedsSync(...) check? > > In fact if you're not applying these sync changes (this->ApplySyncData) should > you be updating that (as we established, silly) synced_extenison_list_ structure > in SyncBundle at all? > > (i.e. should bundle->ApplySyncData call move into this->ApplySyncData) Yeah, that makes sense. Done. > It would be even nice to change bundle->ApplySyncData into those 2 add/remove > methods because bundle->ApplySyncData doesn't actually do anything with the sync > data, which is misleading. I didn't want to make the SyncBundle::Add/RemoveSyncedExtension methods public. Let's try to keep that crap as local as possible :) SyncBundle::ApplySyncData isn't really a good name anymore, but oh well. Hopefully it'll soon be gone anyway. > However as established the best solution would be to remove that > synced_extensions_ concept entirely, so you might as well hold off any further > cleanup here until then. I am really puzzled by the whole thing (in fact I don't > quite understand your response to the ExtensionRegistry comment but it's not > important right now). There's a case where an extension is uninstalled (and removed from the ExtensionRegistry), but that change is *not* propagated to Sync. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:183: ExtensionPrefs::Get(profile_)->SetNeedsSync(data.id(), false); On 2015/07/17 15:28:50, kalman wrote: > I find it a little bit weird that querying this bit gets its own method > (ExtensionNeedsSync) while setting it doesn't. I'd have gone with just querying > the prefs directly. Not that important though. Ah, that's from a previous patch set where SyncBundle needed that info. Now it's really unnecessary; I've removed it. https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:382: FillSyncDataList( On 2015/07/17 15:28:50, kalman wrote: > Could you add: > > // TODO(trieb): Should we be including blacklisted/blocked extensions here? > // I.e. just calling registry->GeneratedInstalledExtensionsSet()? > // It would be more consistent, but the danger is that the black/blocklist > // hasn't been updated on all clients by the time sync has kicked in - > // so it's safest not to. Take care to add any other extension lists here > // in the future if they are added. Sure, done. I've also added your name in the TODO(..) though ;P https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.h (right): https://codereview.chromium.org/1240573012/diff/110001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.h:93: // to the pending list. On 2015/07/17 15:28:50, kalman wrote: > "... They will be applied later when..?" Nope, currently it's not applied later, ever. I'm hoping to change that in the next CL. (At the moment, all "pending" means is "send this cached data back to Sync, instead of the actual local state". It's supposed to handle the case where we get an update from Sync saying "version 3" but we have version 2 installed. It'll take us a while to update, and we don't want Sync flip-flopping meanwhile. The implementation is quite broken though, big surprise there.)
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1240573012/#ps170001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1240573012/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #10 (id:170001) has been deleted
Argh, of course there's more complications... (1) Extensions can be installed before the ExtensionSyncService exists. Then ExtensionService just won't call SyncExtensionChangeIfNeeded, the NeedsSync pref never gets set, and the extension doesn't get synced. I see two ways to fix this: a) Set the pref outside of the ExtensionSyncService, whenever we call SyncExtensionChangeIfNeeded. (Probably make a helper function to do both at once.) b) Instead of having an extension_sync_service_ member in ExtensionService, just call ExtensionSyncService::Get(...) which will create it if it doesn't exist yet. Any preferences? a) is ugly and inconvenient, b) will cause the ExtensionSyncService to be created earlier. (Maybe right now, it's not created at all if Sync is disabled? I haven't checked.) (2) This might or might not be just an issue with the test setup; haven't figured it out yet: In the sync integration tests, we get MergeDataAndStartSyncing, then StopSyncing, then MergeDataAndStartSyncing again. (This does not happen when running Chrome normally, or at least not always.) In the first MergeDataAndStartSyncing, we'll clear the NeedsSync pref and send out changes, but apparently these are discarded somehow?! I'll investigate more tomorrow, brain's fried now...
On 2015/07/20 16:19:25, Marc Treib wrote: > Argh, of course there's more complications... > > (1) Extensions can be installed before the ExtensionSyncService exists. Then > ExtensionService just won't call SyncExtensionChangeIfNeeded, the NeedsSync pref > never gets set, and the extension doesn't get synced. I see two ways to fix > this: > a) Set the pref outside of the ExtensionSyncService, whenever we call > SyncExtensionChangeIfNeeded. (Probably make a helper function to do both at > once.) > b) Instead of having an extension_sync_service_ member in ExtensionService, just > call ExtensionSyncService::Get(...) which will create it if it doesn't exist > yet. > Any preferences? a) is ugly and inconvenient, b) will cause the > ExtensionSyncService to be created earlier. (Maybe right now, it's not created > at all if Sync is disabled? I haven't checked.) > > (2) This might or might not be just an issue with the test setup; haven't > figured it out yet: In the sync integration tests, we get > MergeDataAndStartSyncing, then StopSyncing, then MergeDataAndStartSyncing again. > (This does not happen when running Chrome normally, or at least not always.) In > the first MergeDataAndStartSyncing, we'll clear the NeedsSync pref and send out > changes, but apparently these are discarded somehow?! I'll investigate more > tomorrow, brain's fried now... I prefer (b), there shouldn't be any harm in constructing an object earlier. (and btw I'd just put it as a member of ExtensionSystem rather than making it a KeyedServer or anything). > There's a case where an extension is uninstalled (and removed from the > ExtensionRegistry), but that change is *not* propagated to Sync. Btw I think this is solvable, you could detect using REINSTALL somehow, or a similar approach: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
On 2015/07/20 16:36:56, kalman wrote: > On 2015/07/20 16:19:25, Marc Treib wrote: > > Argh, of course there's more complications... > > > > (1) Extensions can be installed before the ExtensionSyncService exists. Then > > ExtensionService just won't call SyncExtensionChangeIfNeeded, the NeedsSync > pref > > never gets set, and the extension doesn't get synced. I see two ways to fix > > this: > > a) Set the pref outside of the ExtensionSyncService, whenever we call > > SyncExtensionChangeIfNeeded. (Probably make a helper function to do both at > > once.) > > b) Instead of having an extension_sync_service_ member in ExtensionService, > just > > call ExtensionSyncService::Get(...) which will create it if it doesn't exist > > yet. > > Any preferences? a) is ugly and inconvenient, b) will cause the > > ExtensionSyncService to be created earlier. (Maybe right now, it's not created > > at all if Sync is disabled? I haven't checked.) > > > > (2) This might or might not be just an issue with the test setup; haven't > > figured it out yet: In the sync integration tests, we get > > MergeDataAndStartSyncing, then StopSyncing, then MergeDataAndStartSyncing > again. > > (This does not happen when running Chrome normally, or at least not always.) > In > > the first MergeDataAndStartSyncing, we'll clear the NeedsSync pref and send > out > > changes, but apparently these are discarded somehow?! I'll investigate more > > tomorrow, brain's fried now... > > I prefer (b), there shouldn't be any harm in constructing an object earlier. Done. > (and btw I'd just put it as a member of ExtensionSystem rather than making it a > KeyedServer or anything). ExtensionSyncService is already a KeyedService. Maybe it shouldn't be (ExtensionService isn't), but that's another CL... > > There's a case where an extension is uninstalled (and removed from the > > ExtensionRegistry), but that change is *not* propagated to Sync. > > Btw I think this is solvable, you could detect using REINSTALL somehow, or a > similar approach: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... Sure, it'll be solvable somehow. I'm just saying it's not quite as simple as it maybe should be.
https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_unittest.cc (left): https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... chrome/browser/extensions/extension_service_unittest.cc:1151: void InitializeExtensionSyncService() { Not necessary anymore, now that the ExtensionSyncService is created by the ExtensionService when it's needed. (This was actually creating a second ExtensionSyncService and breaking things.) https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:180: std::vector<ExtensionSyncData> data_list = GetLocalSyncDataList(type, true); I still haven't figured out the actual problem here (or whether it's an actual problem or just an issue with the test setup), so I've reverted to "send everything on sync startup" for now. Are you okay with submitting like this for now? The "send only stuff that actually changed" part is really unrelated to the rest, and I'd like to get the rest landed...
https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_unittest.cc (left): https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... chrome/browser/extensions/extension_service_unittest.cc:1151: void InitializeExtensionSyncService() { On 2015/07/21 10:54:00, Marc Treib wrote: > Not necessary anymore, now that the ExtensionSyncService is created by the > ExtensionService when it's needed. > (This was actually creating a second ExtensionSyncService and breaking things.) wow... https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:180: std::vector<ExtensionSyncData> data_list = GetLocalSyncDataList(type, true); On 2015/07/21 10:54:00, Marc Treib wrote: > I still haven't figured out the actual problem here (or whether it's an actual > problem or just an issue with the test setup), so I've reverted to "send > everything on sync startup" for now. > Are you okay with submitting like this for now? The "send only stuff that > actually changed" part is really unrelated to the rest, and I'd like to get the > rest landed... I'll leave it to your judgement. If you think it's ok, go for it.
On 2015/07/21 13:43:30, kalman wrote: > https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... > File chrome/browser/extensions/extension_service_unittest.cc (left): > > https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... > chrome/browser/extensions/extension_service_unittest.cc:1151: void > InitializeExtensionSyncService() { > On 2015/07/21 10:54:00, Marc Treib wrote: > > Not necessary anymore, now that the ExtensionSyncService is created by the > > ExtensionService when it's needed. > > (This was actually creating a second ExtensionSyncService and breaking > things.) > > wow... Ah, I meant after my "implicitly create ExtensionSyncService when you need it" change. Before this CL, it was more or less fine (I think). I was just trying to explain why I needed to touch tests. > https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... > File chrome/browser/extensions/extension_sync_service.cc (right): > > https://codereview.chromium.org/1240573012/diff/210001/chrome/browser/extensi... > chrome/browser/extensions/extension_sync_service.cc:180: > std::vector<ExtensionSyncData> data_list = GetLocalSyncDataList(type, true); > On 2015/07/21 10:54:00, Marc Treib wrote: > > I still haven't figured out the actual problem here (or whether it's an actual > > problem or just an issue with the test setup), so I've reverted to "send > > everything on sync startup" for now. > > Are you okay with submitting like this for now? The "send only stuff that > > actually changed" part is really unrelated to the rest, and I'd like to get > the > > rest landed... > > I'll leave it to your judgement. If you think it's ok, go for it. Alright, thanks! It's not a regression, we're just back to the old behavior of pushing all extensions on sync startup, even those that haven't changed. So I'll land this now, and look at it again at some later point.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1240573012/#ps210001 (title: "(b); hackfix sync_integration_tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1240573012/210001
Message was sent while issue was closed.
Committed patchset #11 (id:210001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c349453205678fda3d6a8a7f1a90a5cec56b8216 Cr-Commit-Position: refs/heads/master@{#339651} |
