Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401083003:
[Sync] Adding integration tests for USS encryption and fixing a worker bug. (Closed)
Description[Sync] Adding integration tests for USS encryption and fixing a worker bug.
The worker bug was that we stored undecryptable updates in memory, while continuing to store progress marker changes to disk. To make dealing with this issue more difficult, OnCryptographerUpdated() is called twice while incorporating an update Nigori node, the first containing an old cryptographer that does not have new pending keys.
Do address these issues, updates are no longer propagated to the processor on cryptographer change or when the cryptographer has pending keys. Adding EncryptionAcceptedApplyUpdates() to allow forcing propagation after, for example, a passphrase is entered successfully.
BUG=647875
Committed: https://crrev.com/5afeb69d7607d1d31b98b16d1cfcfc98dd2b0f0b
Cr-Commit-Position: refs/heads/master@{#426576}
Patch Set 1 #Patch Set 2 : Rewrote some comments. #Patch Set 3 : Updated another comment. #Patch Set 4 : Removing "Encryptoin keys" capitalization fix. #
Total comments: 28
Patch Set 5 : Updates for Max. #Patch Set 6 : Reworked logic in hopes of being more durable and fast failing. #Patch Set 7 : Fixing spacing in sync_encryption_handler.h #
Total comments: 40
Patch Set 8 : Updates for Max. #
Total comments: 8
Patch Set 9 : Updated for rkaplow@'s comments. #Messages
Total messages: 39 (26 generated)
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Sync] Adding integration tests for USS encryption and fixing a worker bug. BUG=647875 ========== to ========== [Sync] Adding integration tests for USS encryption and fixing a worker bug. The worker bug was that we stored undecryptable updates in memory, while continuing to store progress marker changes to disk. My approach to solving this was to block all updates in the worker while we know there are outstanding encryption problems. This turned out to be surprisingly hard, we can't simply trust the cryptographer to know what was up. I punted on trying to fully reason about why ModelTypeWorker::UpdateCryptographer gets called, and tried to make the code more defensive against the scenarios that I could create, and more fragile against scenarios I hope don't happen. BUG=647875 ==========
skym@chromium.org changed reviewers: + maxbogue@chromium.org, pavely@chromium.org
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Max and Pavel, PTAL, there's definitely some less than ideal stuff in this CL, let me know if you have suggestions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right): https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:33: const char* kPassphrase = "12345"; That's the stupidest combination I've ever heard in my life! That's the kind of thing an idiot would have on his luggage! https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:343: ASSERT_TRUE(PassphraseAcceptedChecker(GetSyncService(0)).Wait()); The passphrase can be not accepted? https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:361: ASSERT_TRUE(DataChecker(model0, kKey1, kValue2).Wait()); Why do you check kKey1 and kKey3 twice but never kKey2? Maybe just check all four on each client? https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:380: // Unfourtunately, the behavior here is kind of undefind and we're relying on "Unfortunately", "undefined" https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:381: // race conditoins to hit the logic we're trying to test. What typically "conditions" Can you make it clearer which things are racing? Also, does this test succeed either way? https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:383: // going to restore/migrate nigori to its initial/default value. It will also So the client 1 nigori is going from what to what? No encryption to KEYSTORE? But not CUSTOM? https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:385: // entity. It can correctly use the default nigori encryption, and picks up Why can it use default encryption if it should be using passphrase, and why is that correct? https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:388: // nigori, and bizarrely it will never get it during this GetUpdates cycle. It Seems like something that's broken... https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... File components/sync/engine_impl/model_type_worker.cc (right): https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.cc:336: // Reset the flag, if anything undecryptable is encountered below this flag "Reset the flag. If anything undecryptable is encountered below, then this flag.." https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.cc:379: // never going to recieve the new key, and |decrypted_everything_| is "receive" https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.cc:382: // undecryptable entity contiuously blocks progress. This would cause "continuously" https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.cc:383: // the data type to silently stop working, which is definitely less than Should we add a UMA counter to track this case...? https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... File components/sync/engine_impl/model_type_worker.h (right): https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.h:113: // encyrption issues. Must wait for keys to be updated. "encryption" https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.h:183: bool decrypted_everything_; I keep reading this as "decrypt_everything_". Could you make it "everything_decrypted_" instead so it's harder to misread?
https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right): https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:33: const char* kPassphrase = "12345"; On 2016/10/11 15:56:38, maxbogue wrote: > That's the stupidest combination I've ever heard in my life! That's the kind of > thing an idiot would have on his luggage! :) https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:343: ASSERT_TRUE(PassphraseAcceptedChecker(GetSyncService(0)).Wait()); On 2016/10/11 15:56:38, maxbogue wrote: > The passphrase can be not accepted? If it's async, it can before accepted, and after accepted. As for being rejected, not explicitly but there might be a reason we never reach the correct state to consider it accepted. See https://cs.chromium.org/chromium/src/chrome/browser/sync/test/integration/syn... You could probably get a passphrase to not take if you already had one. The client UI makes this impossible to do afaik. Actually might be good to have an integ test around two clients simultaneously adding conflicting passphrases, but I worry that the fake server's implementation might not be good enough to do the right thing. Will investigate. https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:361: ASSERT_TRUE(DataChecker(model0, kKey1, kValue2).Wait()); On 2016/10/11 15:56:39, maxbogue wrote: > Why do you check kKey1 and kKey3 twice but never kKey2? Maybe just check all > four on each client? Whoops, model1 should have checked key2, not key3 (line 358). Fixed. Because we're verifying that the changes go where they're supposed to! We could do all 8 checks but I worry that'd make it less obvious what we're testing exactly. https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:380: // Unfourtunately, the behavior here is kind of undefind and we're relying on On 2016/10/11 15:56:38, maxbogue wrote: > "Unfortunately", "undefined" Done. https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:381: // race conditoins to hit the logic we're trying to test. What typically On 2016/10/11 15:56:39, maxbogue wrote: > "conditions" > > Can you make it clearer which things are racing? Also, does this test succeed > either way? Done. And I believe so, although it consistently executes on my machine. If you put in a blocking checker between the passphrase and the WriteItem, you basically get the TwoClientUssSyncTest.Encryption test case. https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:383: // going to restore/migrate nigori to its initial/default value. It will also On 2016/10/11 15:56:38, maxbogue wrote: > So the client 1 nigori is going from what to what? No encryption to KEYSTORE? > But not CUSTOM? I'm not sure actually. I was just printing out key names. Adding some logging really fast to SyncEncryptionHandlerImpl::ApplyNigoriUpdateImpl I see both PassphraseType::IMPLICIT_PASSPHRASE and PassphraseType::KEYSTORE_PASSPHRASE fly by. https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:385: // entity. It can correctly use the default nigori encryption, and picks up On 2016/10/11 15:56:38, maxbogue wrote: > Why can it use default encryption if it should be using passphrase, and why is > that correct? If it's just temporary, hasn't really mattered before. Though maybe we waste bandwidth encrypting things and then re-encrypting them in quick succession. https://codereview.chromium.org/2401083003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:388: // nigori, and bizarrely it will never get it during this GetUpdates cycle. It On 2016/10/11 15:56:38, maxbogue wrote: > Seems like something that's broken... It does seem odd. https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... File components/sync/engine_impl/model_type_worker.cc (right): https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.cc:336: // Reset the flag, if anything undecryptable is encountered below this flag On 2016/10/11 15:56:39, maxbogue wrote: > "Reset the flag. If anything undecryptable is encountered below, then this > flag.." Done. https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.cc:379: // never going to recieve the new key, and |decrypted_everything_| is On 2016/10/11 15:56:39, maxbogue wrote: > "receive" Done. https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.cc:382: // undecryptable entity contiuously blocks progress. This would cause On 2016/10/11 15:56:39, maxbogue wrote: > "continuously" Done. https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.cc:383: // the data type to silently stop working, which is definitely less than On 2016/10/11 15:56:39, maxbogue wrote: > Should we add a UMA counter to track this case...? It would be great to have an UMA counter that helped us understand how often we got stuck in the can't decrypt everything state. But it's really hard to know you're in that, and instead you'll just get a lot of noise from normal usage. https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... File components/sync/engine_impl/model_type_worker.h (right): https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.h:113: // encyrption issues. Must wait for keys to be updated. On 2016/10/11 15:56:39, maxbogue wrote: > "encryption" Spelling it hard :( https://codereview.chromium.org/2401083003/diff/60001/components/sync/engine_... components/sync/engine_impl/model_type_worker.h:183: bool decrypted_everything_; On 2016/10/11 15:56:39, maxbogue wrote: > I keep reading this as "decrypt_everything_". Could you make it > "everything_decrypted_" instead so it's harder to misread? Done.
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker.cc (right): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.cc:214: DCHECK(!has_encryted_updates_); Should this be a CHECK? https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker.h (right): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:196: bool has_encryted_updates_; incorrectly spelled.
lgtm; almost all comments are just nits and spelling. https://codereview.chromium.org/2401083003/diff/120001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right): https://codereview.chromium.org/2401083003/diff/120001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:279: // Model 2 should not be affected. Maybe say "The second model should not be affected."? Or just "Model 1".. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker.cc (right): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.cc:38: has_encryted_updates_(false), I'd prefer just doing = false in the declaration above, personally. I like to keep constructors as simple as possible. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.cc:212: // encrytped data. It is possible that non-immediately consistent updates do encrypted https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.cc:214: DCHECK(!has_encryted_updates_); On 2016/10/19 19:22:16, skym wrote: > Should this be a CHECK? I would add a histogram/counter thingy before changing it to a check probably, since someone that hit a CHECK here would just hit it over and over wouldn't they? https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.cc:311: return true; You could just return IsTypeInitialized() && !BlockForEncryption(). I think that might actually be clearer, with the comments rephrased to match: "We can only commit if we've received the initial update response and aren't blocked by missing encryption keys." https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker.h (right): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:91: // An alternative way way to drive sending data to the processor, that should s/way way/way https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:117: // encryption issues. Must wait for keys to be updated. issues and must wait https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:131: // Returns whether the encryption key name is different between The way this is phrased does not make it immediately clear that this function has a side effect. I would switch the order so it's more like "updates blah if necessary and returns whether an update occurred" https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:137: // that has encrytped data. Also updates |has_encryted_updates_| to reflect encrypted https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:195: // If there are outstanding encrypted updates in |entities_|. s/If/Whether https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker_unittest.cc (left): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:973: processor()->HasUpdateResponse(kHash1); This just... wasn't checked against before? o_O https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker_unittest.cc (right): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:718: // Now the information is allowed ot reach the proc. to* processor* https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:882: // Now perform first sync and make sure the ekn makes it. EKN* for consistency https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:911: // ApplyUpdates() will cause everything to finally be delievered. delivered* https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:924: // Prepeare to commit an item. Prepare* https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:930: TriggerPartialUpdateFromServer(10, kTag1, kValue1); Why Partial here? Shouldn't it not matter, since it won't be applied anyways? https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:1045: // Prepeare to commit an item. Prepare* https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:1051: TriggerPartialUpdateFromServer(10, kTag1, kValue1); same question re partial
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Updates for Max, adding rkaplow@ for histograms.xml change. https://codereview.chromium.org/2401083003/diff/120001/chrome/browser/sync/te... File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right): https://codereview.chromium.org/2401083003/diff/120001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:279: // Model 2 should not be affected. On 2016/10/19 22:15:43, maxbogue wrote: > Maybe say "The second model should not be affected."? Or just "Model 1".. Done. https://codereview.chromium.org/2401083003/diff/120001/chrome/browser/sync/te... chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:324: // Write an item on model 0 to trigger a GetUpdates in model 2. Fixed here as well. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker.cc (right): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.cc:38: has_encryted_updates_(false), On 2016/10/19 22:15:43, maxbogue wrote: > I'd prefer just doing = false in the declaration above, personally. I like to > keep constructors as simple as possible. Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.cc:212: // encrytped data. It is possible that non-immediately consistent updates do On 2016/10/19 22:15:43, maxbogue wrote: > encrypted Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.cc:214: DCHECK(!has_encryted_updates_); On 2016/10/19 22:15:43, maxbogue wrote: > On 2016/10/19 19:22:16, skym wrote: > > Should this be a CHECK? > > I would add a histogram/counter thingy before changing it to a check probably, > since someone that hit a CHECK here would just hit it over and over wouldn't > they? That is true. Adding a histogram. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.cc:311: return true; On 2016/10/19 22:15:43, maxbogue wrote: > You could just return IsTypeInitialized() && !BlockForEncryption(). I think that > might actually be clearer, with the comments rephrased to match: "We can only > commit if we've received the initial update response and aren't blocked by > missing encryption keys." Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker.h (right): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:91: // An alternative way way to drive sending data to the processor, that should On 2016/10/19 22:15:43, maxbogue wrote: > s/way way/way Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:117: // encryption issues. Must wait for keys to be updated. On 2016/10/19 22:15:43, maxbogue wrote: > issues and must wait Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:131: // Returns whether the encryption key name is different between On 2016/10/19 22:15:43, maxbogue wrote: > The way this is phrased does not make it immediately clear that this function > has a side effect. I would switch the order so it's more like "updates blah if > necessary and returns whether an update occurred" Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:137: // that has encrytped data. Also updates |has_encryted_updates_| to reflect On 2016/10/19 22:15:43, maxbogue wrote: > encrypted Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:195: // If there are outstanding encrypted updates in |entities_|. On 2016/10/19 22:15:43, maxbogue wrote: > s/If/Whether Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker.h:196: bool has_encryted_updates_; On 2016/10/19 19:22:16, skym wrote: > incorrectly spelled. Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker_unittest.cc (left): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:973: processor()->HasUpdateResponse(kHash1); On 2016/10/19 22:15:44, maxbogue wrote: > This just... wasn't checked against before? o_O Yup! Was surprised when I realized that as well. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... File components/sync/engine_impl/model_type_worker_unittest.cc (right): https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:718: // Now the information is allowed ot reach the proc. On 2016/10/19 22:15:43, maxbogue wrote: > to* processor* Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:882: // Now perform first sync and make sure the ekn makes it. On 2016/10/19 22:15:44, maxbogue wrote: > EKN* for consistency Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:911: // ApplyUpdates() will cause everything to finally be delievered. On 2016/10/19 22:15:44, maxbogue wrote: > delivered* Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:924: // Prepeare to commit an item. On 2016/10/19 22:15:44, maxbogue wrote: > Prepare* This wasn't even me! Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:930: TriggerPartialUpdateFromServer(10, kTag1, kValue1); On 2016/10/19 22:15:44, maxbogue wrote: > Why Partial here? Shouldn't it not matter, since it won't be applied anyways? Because we don't want to hit the DCHECK in ApplyPendingUpdates(). Until we add the pending key, it will be applied. Updated comment a bit. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:1045: // Prepeare to commit an item. On 2016/10/19 22:15:44, maxbogue wrote: > Prepare* Done. https://codereview.chromium.org/2401083003/diff/120001/components/sync/engine... components/sync/engine_impl/model_type_worker_unittest.cc:1051: TriggerPartialUpdateFromServer(10, kTag1, kValue1); On 2016/10/19 22:15:44, maxbogue wrote: > same question re partial Same response!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
skym@chromium.org changed reviewers: + rkaplow@chromium.org
Adding rkaplow@ for real this time for histograms.xml change.
lgtm https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63577: + enum="BooleanHasEncryptedUpdates"> nit, instead of a custom Boolean.. I would just use Boolean https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63581: + encrypted updates, but the cryptographer has no pending keys. This state can you reword? This first sentance isn't making sense to me
https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63581: + encrypted updates, but the cryptographer has no pending keys. This state On 2016/10/20 17:31:10, rkaplow wrote: > can you reword? This first sentance isn't making sense to me I suggest "Whether there are still encrypted updates that the cryptographer can't decrypt during ApplyUpdates in the ModelTypeWorker (USS only).". https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63582: + should be extermely rare, due to a bug or inconsistent GetUpdates that extremely*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63577: + enum="BooleanHasEncryptedUpdates"> On 2016/10/20 17:31:10, rkaplow wrote: > nit, instead of a custom Boolean.. I would just use Boolean Done. https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63581: + encrypted updates, but the cryptographer has no pending keys. This state On 2016/10/20 17:31:10, rkaplow wrote: > can you reword? This first sentance isn't making sense to me Done. https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63581: + encrypted updates, but the cryptographer has no pending keys. This state On 2016/10/20 18:12:46, maxbogue wrote: > On 2016/10/20 17:31:10, rkaplow wrote: > > can you reword? This first sentance isn't making sense to me > > I suggest "Whether there are still encrypted updates that the cryptographer > can't decrypt during ApplyUpdates in the ModelTypeWorker (USS only).". Done. https://codereview.chromium.org/2401083003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:63582: + should be extermely rare, due to a bug or inconsistent GetUpdates that On 2016/10/20 18:12:46, maxbogue wrote: > extremely* Done.
Description was changed from ========== [Sync] Adding integration tests for USS encryption and fixing a worker bug. The worker bug was that we stored undecryptable updates in memory, while continuing to store progress marker changes to disk. My approach to solving this was to block all updates in the worker while we know there are outstanding encryption problems. This turned out to be surprisingly hard, we can't simply trust the cryptographer to know what was up. I punted on trying to fully reason about why ModelTypeWorker::UpdateCryptographer gets called, and tried to make the code more defensive against the scenarios that I could create, and more fragile against scenarios I hope don't happen. BUG=647875 ========== to ========== [Sync] Adding integration tests for USS encryption and fixing a worker bug. The worker bug was that we stored undecryptable updates in memory, while continuing to store progress marker changes to disk. To make dealing with this issue more difficult, OnCryptographerUpdated() is called twice while incorporating an update Nigori node, the first containing an old cryptographer that does not have new pending keys. Do address these issues, updates are no longer propagated to the processor on cryptographer change or when the cryptographer has pending keys. Adding EncryptionAcceptedApplyUpdates() to allow forcing propagation after, for example, a passphrase is entered successfully. BUG=647875 ==========
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maxbogue@chromium.org, rkaplow@chromium.org Link to the patchset: https://codereview.chromium.org/2401083003/#ps160001 (title: "Updated for rkaplow@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Sync] Adding integration tests for USS encryption and fixing a worker bug. The worker bug was that we stored undecryptable updates in memory, while continuing to store progress marker changes to disk. To make dealing with this issue more difficult, OnCryptographerUpdated() is called twice while incorporating an update Nigori node, the first containing an old cryptographer that does not have new pending keys. Do address these issues, updates are no longer propagated to the processor on cryptographer change or when the cryptographer has pending keys. Adding EncryptionAcceptedApplyUpdates() to allow forcing propagation after, for example, a passphrase is entered successfully. BUG=647875 ========== to ========== [Sync] Adding integration tests for USS encryption and fixing a worker bug. The worker bug was that we stored undecryptable updates in memory, while continuing to store progress marker changes to disk. To make dealing with this issue more difficult, OnCryptographerUpdated() is called twice while incorporating an update Nigori node, the first containing an old cryptographer that does not have new pending keys. Do address these issues, updates are no longer propagated to the processor on cryptographer change or when the cryptographer has pending keys. Adding EncryptionAcceptedApplyUpdates() to allow forcing propagation after, for example, a passphrase is entered successfully. BUG=647875 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Adding integration tests for USS encryption and fixing a worker bug. The worker bug was that we stored undecryptable updates in memory, while continuing to store progress marker changes to disk. To make dealing with this issue more difficult, OnCryptographerUpdated() is called twice while incorporating an update Nigori node, the first containing an old cryptographer that does not have new pending keys. Do address these issues, updates are no longer propagated to the processor on cryptographer change or when the cryptographer has pending keys. Adding EncryptionAcceptedApplyUpdates() to allow forcing propagation after, for example, a passphrase is entered successfully. BUG=647875 ========== to ========== [Sync] Adding integration tests for USS encryption and fixing a worker bug. The worker bug was that we stored undecryptable updates in memory, while continuing to store progress marker changes to disk. To make dealing with this issue more difficult, OnCryptographerUpdated() is called twice while incorporating an update Nigori node, the first containing an old cryptographer that does not have new pending keys. Do address these issues, updates are no longer propagated to the processor on cryptographer change or when the cryptographer has pending keys. Adding EncryptionAcceptedApplyUpdates() to allow forcing propagation after, for example, a passphrase is entered successfully. BUG=647875 Committed: https://crrev.com/5afeb69d7607d1d31b98b16d1cfcfc98dd2b0f0b Cr-Commit-Position: refs/heads/master@{#426576} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5afeb69d7607d1d31b98b16d1cfcfc98dd2b0f0b Cr-Commit-Position: refs/heads/master@{#426576} |
