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

Issue 873902 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows , Chrome
Pri: 2
Type: Bug
Flaky-Test: TwoClientPreferencesSyncTest.SingleClientEnabledEncryptionBothChanged_E2ETest



Sign in to add a comment

flaky TwoClientPreferencesSyncTest.SingleClientEnabledEncryptionBothChanged_E2ETest on CrOS

Project Member Reported by shimazu@chromium.org, Aug 14

Issue description

TwoClientPreferencesSyncTest.SingleClientEnabledEncryptionBothChanged_E2ETest is flaky on chromeos bots.

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=sync_integration_tests&tests=TwoClientPreferencesSyncTest.SingleClientEnabledEncryptionBothChanged_E2ETest

I'll disable it tentatively.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/952dfd231ca7a8287934bcfb96daa8f02d8dd24a

commit 952dfd231ca7a8287934bcfb96daa8f02d8dd24a
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Tue Aug 14 02:14:30 2018

Disable a test in TwoClientPreferencesSyncTest on CrOS

TwoClientPreferencesSyncTest.SingleClientEnabledEncryptionBothChanged is flaky
on ChromeOS bots. This disables it.

TBR=vitaliii@chromium.org
NOTRY=true

Bug:  873902 
Change-Id: I72b3692746942c7c8e4cb58e06bc9b7817402f85
Reviewed-on: https://chromium-review.googlesource.com/1173201
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582798}
[modify] https://crrev.com/952dfd231ca7a8287934bcfb96daa8f02d8dd24a/chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc

Components: Services>Sync
Owner: melandory@chromium.org
melandory@, could you triage please?
Owner: ----
Removing myself from owner for the bug, but it's probably good candidate for fixit
Labels: sync-fixit-2018q4
Labels: OS-Chrome OS-Windows
Status: Available (was: Assigned)
Also flaky on Windows, though not quite as badly.
Labels: FixitSync
Owner: mastiz@chromium.org
Status: Assigned (was: Available)
I took a quick look at a recent failure on Windows, which may or may not be related to the original ChromeOS issues that motivated this bug.

From https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8930309984534468480/+/steps/sync_integration_tests/0/stdout:

There is a DCHECK failure:
[816:4336:1109/112014.155:FATAL:model_type_worker.cc(401)] Check failed: entries_pending_decryption_.empty(). 

Backtrace:
	base::debug::StackTrace::StackTrace [0x6EE6D2A6+102]
	base::debug::StackTrace::StackTrace [0x6EE6C27B+27]
	logging::LogMessage::~LogMessage [0x6EED0B37+151]
	syncer::ModelTypeWorker::ApplyPendingUpdates [0x08698975+549]
	syncer::ModelTypeWorker::ApplyUpdates [0x0869B376+422]
	syncer::NormalGetUpdatesDelegate::ApplyUpdates [0x07906DCA+282]
	syncer::NormalGetUpdatesDelegate::ApplyUpdates [0x07906CE7+55]
	syncer::GetUpdatesProcessor::ApplyUpdates [0x0790E2CE+126]
	syncer::Syncer::DownloadAndApplyUpdates [0x0664D3D1+929]
	syncer::Syncer::NormalSyncShare [0x0664CE38+520]
	syncer::SyncSchedulerImpl::DoNudgeSyncCycleJob [0x06653D54+836]
	syncer::SyncSchedulerImpl::TrySyncCycleJobImpl [0x06656F1D+1357]
Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/84b396debc80028ff30630365840caa9f901e38f

commit 84b396debc80028ff30630365840caa9f901e38f
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Nov 19 11:51:43 2018

Avoid DCHECK failure during ModelTypeWorker decryption

It's unclear why the DCHECK removed in this patch is guaranteed to hold
true, and in fact some tests run into it. Instead, let's defer
application of updates until all pending ones can be decrypted.

According to an old (previously deprecated) UMA metric,
Sync.WorkerApplyHasEncryptedUpdates, the condition doesn't meet about
once per million times. If this only affects the small subset of users
with custom passphrase enabled, which is likely, then it's non-
negligible.

This may (in rare cases) differ from the cryptographer being ready,
specially during sync cyles where NIGORI updates are not requested.

Bug:  873902 
Change-Id: Ic70efdad3b72e9387bac50e782a51da09e532857
Reviewed-on: https://chromium-review.googlesource.com/c/1335933
Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609243}
[modify] https://crrev.com/84b396debc80028ff30630365840caa9f901e38f/components/sync/engine_impl/model_type_worker.cc
[modify] https://crrev.com/84b396debc80028ff30630365840caa9f901e38f/components/sync/engine_impl/model_type_worker_unittest.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/480bd04fd4799540d5ae6e069c9dbc37c2451200

commit 480bd04fd4799540d5ae6e069c9dbc37c2451200
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Nov 19 13:27:42 2018

Remove corrupt data from sync worker's encrypted update queue

If decrypting an update fails despite CanDecrypt() returning true, it's
safe to assume the data is corrupted. Hence, let's drop such update and
prevent the worker's update queue from being blocked indefinitely.

Bug:  873902 
Change-Id: I01bc8271d2e1daa961f3136d8b5a21d4ecde1ced
Reviewed-on: https://chromium-review.googlesource.com/c/1341531
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609267}
[modify] https://crrev.com/480bd04fd4799540d5ae6e069c9dbc37c2451200/components/sync/engine_impl/model_type_worker.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/05048d0fa6c68d58bc4f30a0c2c5953f3e8178df

commit 05048d0fa6c68d58bc4f30a0c2c5953f3e8178df
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Nov 20 09:47:23 2018

Reenable a test in TwoClientPreferencesSyncTest on CrOS

Recent changes (linked to the bug) are believed to have deflaked the
test, so let's reenable it on all platforms.

Bug:  873902 
Change-Id: I9804fa4d83c3b1afb9b71356d134736b63b11298
Reviewed-on: https://chromium-review.googlesource.com/c/1343083
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609649}
[modify] https://crrev.com/05048d0fa6c68d58bc4f30a0c2c5953f3e8178df/chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc

Project Member

Comment 12 by Findit, Nov 20

Labels: Test-Flaky Test-Findit-Analyzed

Flaky test: USS/TwoClientPreferencesSyncTest.SingleClientEnabledEncryptionBothChanged_E2ETest/0
Sample failed build due to flakiness: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/9019
Test output log: https://chromium-swarm.appspot.com/task?id=4149f3cf2e770510
Culprit (70.0% confidence): r606633
Analysis: https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWVy6gELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9vdCKzAWNocm9taXVtLmNocm9taXVtb3MvbGludXgtY2hyb21lb3MtZGJnLzkwMTkvc3luY19pbnRlZ3JhdGlvbl90ZXN0cy9WVk5UTDFSM2IwTnNhV1Z1ZEZCeVpXWmxjbVZ1WTJWelUzbHVZMVJsYzNRdVUybHVaMnhsUTJ4cFpXNTBSVzVoWW14bFpFVnVZM0o1Y0hScGIyNUNiM1JvUTJoaGJtZGxaRjlGTWtWVVpYTjBMekE9DAsSE01hc3RlckZsYWtlQW5hbHlzaXMYAQw

Please revert the culprit, or disable the test and find the appropriate owner.

If the culprit above is wrong, please file a bug using this link:
https://bugs.chromium.org/p/chromium/issues/entry?status=Unconfirmed&labels=Pri-1,Test-Findit-Wrong&components=Tools%3ETest%3EFindit%3EFlakiness&summary=%5BFindit%5D%20Flake%20Analyzer%20-%20Wrong%20result%20for%20USS/TwoClientPreferencesSyncTest.SingleClientEnabledEncryptionBothChanged_E2ETest/0&comment=Link%20to%20Analysis%3A%20https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWVy6gELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9vdCKzAWNocm9taXVtLmNocm9taXVtb3MvbGludXgtY2hyb21lb3MtZGJnLzkwMTkvc3luY19pbnRlZ3JhdGlvbl90ZXN0cy9WVk5UTDFSM2IwTnNhV1Z1ZEZCeVpXWmxjbVZ1WTJWelUzbHVZMVJsYzNRdVUybHVaMnhsUTJ4cFpXNTBSVzVoWW14bFpFVnVZM0o1Y0hScGIyNUNiM1JvUTJoaGJtZGxaRjlGTWtWVVpYTjBMekE9DAsSE01hc3RlckZsYWtlQW5hbHlzaXMYAQw

Automatically posted by the findit-for-me app (https://goo.gl/Ot9f7N).
Project Member

Comment 13 by Findit, Nov 21


Flaky test: USS/TwoClientPreferencesSyncTest.SingleClientEnabledEncryptionBothChanged_E2ETest/1
Sample failed build due to flakiness: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/9029
Test output log: https://chromium-swarm.appspot.com/task?id=414c06e82ad5da10
Culprit (70.0% confidence): r606633
Analysis: https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWVy6gELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9vdCKzAWNocm9taXVtLmNocm9taXVtb3MvbGludXgtY2hyb21lb3MtZGJnLzkwMjkvc3luY19pbnRlZ3JhdGlvbl90ZXN0cy9WVk5UTDFSM2IwTnNhV1Z1ZEZCeVpXWmxjbVZ1WTJWelUzbHVZMVJsYzNRdVUybHVaMnhsUTJ4cFpXNTBSVzVoWW14bFpFVnVZM0o1Y0hScGIyNUNiM1JvUTJoaGJtZGxaRjlGTWtWVVpYTjBMekU9DAsSE01hc3RlckZsYWtlQW5hbHlzaXMYAQw

Please revert the culprit, or disable the test and find the appropriate owner.

If the culprit above is wrong, please file a bug using this link:
https://bugs.chromium.org/p/chromium/issues/entry?status=Unconfirmed&labels=Pri-1,Test-Findit-Wrong&components=Tools%3ETest%3EFindit%3EFlakiness&summary=%5BFindit%5D%20Flake%20Analyzer%20-%20Wrong%20result%20for%20USS/TwoClientPreferencesSyncTest.SingleClientEnabledEncryptionBothChanged_E2ETest/1&comment=Link%20to%20Analysis%3A%20https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWVy6gELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9vdCKzAWNocm9taXVtLmNocm9taXVtb3MvbGludXgtY2hyb21lb3MtZGJnLzkwMjkvc3luY19pbnRlZ3JhdGlvbl90ZXN0cy9WVk5UTDFSM2IwTnNhV1Z1ZEZCeVpXWmxjbVZ1WTJWelUzbHVZMVJsYzNRdVUybHVaMnhsUTJ4cFpXNTBSVzVoWW14bFpFVnVZM0o1Y0hScGIyNUNiM1JvUTJoaGJtZGxaRjlGTWtWVVpYTjBMekU9DAsSE01hc3RlckZsYWtlQW5hbHlzaXMYAQw

Automatically posted by the findit-for-me app (https://goo.gl/Ot9f7N).
Revert in CQ.
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0a866f18159926b39c50f944c7ad2c121970912

commit b0a866f18159926b39c50f944c7ad2c121970912
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Nov 21 14:02:07 2018

Revert "Reenable a test in TwoClientPreferencesSyncTest on CrOS"

This reverts commit 05048d0fa6c68d58bc4f30a0c2c5953f3e8178df.

Reason for revert: test is still flaky on chromeos, e.g.:
https://chromium-swarm.appspot.com/task?id=414c06e82ad5da10&refresh=10&show_raw=1

Original change's description:
> Reenable a test in TwoClientPreferencesSyncTest on CrOS
> 
> Recent changes (linked to the bug) are believed to have deflaked the
> test, so let's reenable it on all platforms.
> 
> Bug:  873902 
> Change-Id: I9804fa4d83c3b1afb9b71356d134736b63b11298
> Reviewed-on: https://chromium-review.googlesource.com/c/1343083
> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609649}

TBR=mastiz@chromium.org,mamir@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  873902 
Change-Id: I6dbf839b515dfc5e374f28b6197e73f1f23ffcac
Reviewed-on: https://chromium-review.googlesource.com/c/1346303
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610039}
[modify] https://crrev.com/b0a866f18159926b39c50f944c7ad2c121970912/chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3085800821a32e046473653f410681e1e47fa06

commit c3085800821a32e046473653f410681e1e47fa06
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Nov 21 16:21:10 2018

Block sync commits if updates pending encryption

ModelTypeWorker::GetContribution() should also be blocked if some
updates are pending a decryption key, otherwise a DCHECK is hit:
[32730:356:1120/142053.301971:FATAL:model_type_worker.cc(471)] Check failed: entries_pending_decryption_.empty().

Since this codepath was the only remaining user of BlockForEncryption()
that didn't verify entries_pending_decryption_ being empty, we change
BlockForEncryption() itself to also require that condition.

Bug:  873902 , 873494 
Change-Id: Ic9f841380da5f81dd440d39698902a6fe2966be4
Reviewed-on: https://chromium-review.googlesource.com/c/1346392
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610095}
[modify] https://crrev.com/c3085800821a32e046473653f410681e1e47fa06/components/sync/engine_impl/model_type_worker.cc

Project Member

Comment 17 by bugdroid1@chromium.org, Nov 21

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58720c1c6e92c89f773cf79c27edeaeb7fc6e3cf

commit 58720c1c6e92c89f773cf79c27edeaeb7fc6e3cf
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Nov 21 20:27:07 2018

Reland "Reenable a test in TwoClientPreferencesSyncTest on CrOS"

This is a reland of 05048d0fa6c68d58bc4f30a0c2c5953f3e8178df

Meanwhile, a patch has landed that is believed to fix the issue
observed in recent flakes.

Original change's description:
> Reenable a test in TwoClientPreferencesSyncTest on CrOS
>
> Recent changes (linked to the bug) are believed to have deflaked the
> test, so let's reenable it on all platforms.
>
> Bug:  873902 
> Change-Id: I9804fa4d83c3b1afb9b71356d134736b63b11298
> Reviewed-on: https://chromium-review.googlesource.com/c/1343083
> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609649}

Bug:  873902 
Change-Id: I0c82df1ed8ca63ec4c0490a3f76ac1c9efddd5c6
Reviewed-on: https://chromium-review.googlesource.com/c/1347351
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610196}
[modify] https://crrev.com/58720c1c6e92c89f773cf79c27edeaeb7fc6e3cf/chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc

Status: Fixed (was: Started)
Looks green, marking as fixed.

Sign in to add a comment