|
|
Created:
4 years, 10 months ago by asargent_no_longer_on_chrome Modified:
4 years, 10 months ago Reviewers:
Nicolas Zea CC:
chromium-reviews, tim+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org, Marc Treib Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange sync conflict resolution for extensions
For extensions and apps, if the server has seen a delete action
(corresponding to an uninstall) and the local state has a not-yet-synced
change, we would prefer to resolve this conflict by keeping the server
state and uninstalling locally. This helps to prevent uninstalled
extensions "coming back to life".
BUG=517910
Committed: https://crrev.com/d40211da2bf6d6b3a6d56e6f9bd26506249339b9
Cr-Commit-Position: refs/heads/master@{#376603}
Patch Set 1 #Patch Set 2 : added comments and unit test #
Total comments: 17
Patch Set 3 : unit test changes, added integration test #
Total comments: 4
Patch Set 4 : moved entry type check lower down in ProcessSimpleConflict #
Messages
Total messages: 15 (5 generated)
asargent@chromium.org changed reviewers: + zea@chromium.org
https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3778: extension.PutIsUnsynced(true); Note: a bunch of the code I use in this test to create/update the entry and nudge the sync to happen is largely cribbed from SyncerBookmarksTest::Create / SyncerBookmarksTest::Update and SyncerTest::SyncShareNudge, but those all assume the ModelType is BOOKMARKS. Let me know if you think it would be worth trying to generalize that logic into helper methods that let you parameterize the ModelType.
https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_re... File sync/engine/conflict_resolver.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_re... sync/engine/conflict_resolver.cc:118: // where the non_unique_name or parent don't match. Another exception is nit: may as well put this into its own e) bullet https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_re... sync/engine/conflict_resolver.cc:207: } else if (entry_deleted || !name_matches || !parent_matches) { Have you manually tested that this works? I think that when we tombstone an item, we also clear out the name, which might mean this logic gets triggered, and we undelete the tombstone anyways. It might be good to add an integration test for this to ensure that doesn't happen. You can probably add it to single_client_extensions_sync_test.cc, and borrow some of the logic from SingleClientBookmarkSyncTest::DownloadModifiedBookmark (in particular, that test calls into the fake server with GetFakeServer()->ModifyEntitySpecifics(..). Basically, if you modify the specifics on the server first to delete it, then modify the item locally, then wait, you should be able to force a conflict, and verify that the items remains undeleted by waiting until it's undeleted at the server too. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3782: ResetSession(); You should just be able to call SyncShareNudge() here and below (the fact that it nudges for Bookmarks doesn't matter). https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3804: mock_server_->AddUpdateTombstone(entry.GetId(), EXTENSIONS); You can just save the id above when you create the item. That way you don't need to open a transaction and look up the entry again. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3807: // Create a local update nit: add a period. Also maybe call out that we're purposefully creating the item before we do the sync to force a conflict. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3815: extension.PutSpecifics(specifics); you don't need to actually change the specifics at this layer. By setting IsUnsynced to true, you'll force the conflict. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3818: if (extension.GetSyncing()) I don't think this condition or the PutDirtySync are necessary.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Thanks for the review! Please take another look. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_re... File sync/engine/conflict_resolver.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_re... sync/engine/conflict_resolver.cc:118: // where the non_unique_name or parent don't match. Another exception is On 2016/02/11 23:41:50, Nicolas Zea wrote: > nit: may as well put this into its own e) bullet Done. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/conflict_re... sync/engine/conflict_resolver.cc:207: } else if (entry_deleted || !name_matches || !parent_matches) { On 2016/02/11 23:41:50, Nicolas Zea wrote: > Have you manually tested that this works? I think that when we tombstone an > item, we also clear out the name, which might mean this logic gets triggered, > and we undelete the tombstone anyways. I have manually tested that the patch works for the following scenario: 1) Get two separate profiles setup syncing to the same account, with one installed extension that is disabled 2) Restart profile B with command-line flags for to use a proxy on a non-listening local port, to simulate "no network connection" 3) On profile A (has network access), uninstall the extension 4) On profile B (still with no network access), enable the extension 5) Restart profile B without the command-line flag, so it has network access again and can contact the sync server. Without my CL, what happens at the end of these steps is that the conflict logic in this file ends up resurrecting the extension at profile A. With my CL, the extension gets uninstalled at B as desired. > It might be good to add an integration test for this to ensure that doesn't > happen. You can probably add it to single_client_extensions_sync_test.cc, and > borrow some of the logic from > SingleClientBookmarkSyncTest::DownloadModifiedBookmark (in particular, that test > calls into the fake server with GetFakeServer()->ModifyEntitySpecifics(..). > Basically, if you modify the specifics on the server first to delete it, then > modify the item locally, then wait, you should be able to force a conflict, and > verify that the items remains undeleted by waiting until it's undeleted at the > server too. Done. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3782: ResetSession(); On 2016/02/11 23:41:50, Nicolas Zea wrote: > You should just be able to call SyncShareNudge() here and below (the fact that > it nudges for Bookmarks doesn't matter). Done. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3804: mock_server_->AddUpdateTombstone(entry.GetId(), EXTENSIONS); On 2016/02/11 23:41:50, Nicolas Zea wrote: > You can just save the id above when you create the item. That way you don't need > to open a transaction and look up the entry again. Done. BTW, I found that this only worked if I saved the id I got from within the first ReadTransaction after nudging the server (value was of the form "smock_server:10000"), but if I saved the id from within the first WriteTransaction but before nudging the server, it didn't work (value was of the form "cAC89AF2E-F949-47EC-8E89-9ABF59481D56"). Hopefully the way I'm doing it now was what you meant in your comment. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3807: // Create a local update On 2016/02/11 23:41:50, Nicolas Zea wrote: > nit: add a period. Also maybe call out that we're purposefully creating the item > before we do the sync to force a conflict. Done. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3815: extension.PutSpecifics(specifics); On 2016/02/11 23:41:50, Nicolas Zea wrote: > you don't need to actually change the specifics at this layer. By setting > IsUnsynced to true, you'll force the conflict. Hmm, I found a subtle difference in the test behavior when I didn't modify the specifics - it's the difference between specifics_match being true vs. false in ConflictResolver::ProcessSimpleConflict. When the specifics do match (ie don't change any specifics here in the test), then down in the final ReadTransaction at the end of the test, I found that entry.GetIsDel() returns false instead of true. Does this matter? I was worried it might mean the item wasn't actually deleted locally. In both cases in ConflictResolver::ProcessSimpleConflict, we end up calling conflict_util::IgnoreConflict - the only difference between the two cases is whether we also call status->increment_num_server_overwrites() and counters->num_server_overwrites++ or not. When I first ran into this issue while writing the test, I figured that changing the enable/disable state matches my manual test case, and also what I suspect *might* be the source of this happening in the wild. Let me know what you think I should do here: a) Skip updating the specifics, and remove the check of entry.GetIsDel() below b) Leave it as is c) Something else - perhaps this has uncovered a real problem that still isn't fixed? https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3818: if (extension.GetSyncing()) On 2016/02/11 23:41:50, Nicolas Zea wrote: > I don't think this condition or the PutDirtySync are necessary. Done.
LG after the conditional move. https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3815: extension.PutSpecifics(specifics); On 2016/02/18 00:53:45, Antony Sargent wrote: > On 2016/02/11 23:41:50, Nicolas Zea wrote: > > you don't need to actually change the specifics at this layer. By setting > > IsUnsynced to true, you'll force the conflict. > > Hmm, I found a subtle difference in the test behavior when I didn't modify the > specifics - it's the difference between specifics_match being true vs. false in > ConflictResolver::ProcessSimpleConflict. When the specifics do match (ie don't > change any specifics here in the test), then down in the final ReadTransaction > at the end of the test, I found that entry.GetIsDel() returns false instead of > true. Does this matter? I was worried it might mean the item wasn't actually > deleted locally. You're right, entry.GetIsDel should be true if the item was properly deleted after the conflict resolved. > > In both cases in ConflictResolver::ProcessSimpleConflict, we end up calling > conflict_util::IgnoreConflict - the only difference between the two cases is > whether we also call status->increment_num_server_overwrites() and > counters->num_server_overwrites++ or not. Ah, that's I think because of the condition you added actually. Previously we would assume that if the local isn't deleted, then the server wouldn't be deleted either (due to the original condition), and hence checking specifics match would be enough. With the new condition for extensions/apps that isn't true. This makes me think we should move the condition (see my other comment). > > When I first ran into this issue while writing the test, I figured that changing > the enable/disable state matches my manual test case, and also what I suspect > *might* be the source of this happening in the wild. > > Let me know what you think I should do here: > > a) Skip updating the specifics, and remove the check of entry.GetIsDel() below > > b) Leave it as is > > c) Something else - perhaps this has uncovered a real problem that still isn't > fixed? > https://codereview.chromium.org/1664643003/diff/80001/sync/engine/conflict_re... File sync/engine/conflict_resolver.cc (right): https://codereview.chromium.org/1664643003/diff/80001/sync/engine/conflict_re... sync/engine/conflict_resolver.cc:122: if (!entry.GetServerIsDel() || (type == EXTENSIONS || type == APPS)) { The more I think about it, the more I think it would be safer to not have this condition here, and instead special case the extension/apps case down below in the else. The rationale is that the logic within the body of this condition is based on the assumption that the entry is not deleted by the server. I think this is why you ran into that subtle case with the specifics matching not triggering the right logic. https://codereview.chromium.org/1664643003/diff/80001/sync/engine/conflict_re... sync/engine/conflict_resolver.cc:245: // The entry is deleted on the server but still exists locally. Basically, this is where I would add a new condition. Something like: if (type == Extensions or Apps) { Copy the code for ignoring local changes here } else { Normal undeletion code here. }
https://codereview.chromium.org/1664643003/diff/80001/sync/engine/conflict_re... File sync/engine/conflict_resolver.cc (right): https://codereview.chromium.org/1664643003/diff/80001/sync/engine/conflict_re... sync/engine/conflict_resolver.cc:245: // The entry is deleted on the server but still exists locally. On 2016/02/18 19:28:55, Nicolas Zea wrote: > Basically, this is where I would add a new condition. Something like: > if (type == Extensions or Apps) { > Copy the code for ignoring local changes here > } else { > Normal undeletion code here. > } Actually this logic should probably happen before the entry.GetIsDir check too. if (is extension or app) { .. ignore local changes } else { if (is dir) { ... } .. undelete by overwriting server changes }
PTAL https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... File sync/engine/syncer_unittest.cc (right): https://codereview.chromium.org/1664643003/diff/20001/sync/engine/syncer_unit... sync/engine/syncer_unittest.cc:3815: extension.PutSpecifics(specifics); With the modification you suggested in conflict_resolver.cc, I've verified that this test now succeeds regardless of whether the specifics are actually changed or not. For now I've left the test as-is (still modifying the specifics), but let me know if you'd rather I changed it to not do so. https://codereview.chromium.org/1664643003/diff/80001/sync/engine/conflict_re... File sync/engine/conflict_resolver.cc (right): https://codereview.chromium.org/1664643003/diff/80001/sync/engine/conflict_re... sync/engine/conflict_resolver.cc:245: // The entry is deleted on the server but still exists locally. On 2016/02/18 19:30:31, Nicolas Zea wrote: > On 2016/02/18 19:28:55, Nicolas Zea wrote: > > Basically, this is where I would add a new condition. Something like: > > if (type == Extensions or Apps) { > > Copy the code for ignoring local changes here > > } else { > > Normal undeletion code here. > > } > > Actually this logic should probably happen before the entry.GetIsDir check too. > > if (is extension or app) { > .. ignore local changes > } else { > if (is dir) { > ... > } > .. undelete by overwriting server changes > } That seems to work great. Done.
LGTM, thanks!
The CQ bit was checked by asargent@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1664643003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1664643003/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Change sync conflict resolution for extensions For extensions and apps, if the server has seen a delete action (corresponding to an uninstall) and the local state has a not-yet-synced change, we would prefer to resolve this conflict by keeping the server state and uninstalling locally. This helps to prevent uninstalled extensions "coming back to life". BUG=517910 ========== to ========== Change sync conflict resolution for extensions For extensions and apps, if the server has seen a delete action (corresponding to an uninstall) and the local state has a not-yet-synced change, we would prefer to resolve this conflict by keeping the server state and uninstalling locally. This helps to prevent uninstalled extensions "coming back to life". BUG=517910 Committed: https://crrev.com/d40211da2bf6d6b3a6d56e6f9bd26506249339b9 Cr-Commit-Position: refs/heads/master@{#376603} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d40211da2bf6d6b3a6d56e6f9bd26506249339b9 Cr-Commit-Position: refs/heads/master@{#376603} |