New issue
Advanced search Search tips

Issue 854446 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug

Blocked on:
issue 855375



Sign in to add a comment

SigninInteractionControllerTestCase is flaking every other test run

Project Member Reported by justincohen@chromium.org, Jun 20 2018

Issue description

Test run shows lots of OnOAuthError and Failed to get UserInfo for foo2ID


 
Should we disable these tests?
Labels: -Pri-3 Pri-0
This is blocking some CLs from landing, flakily, moving to p0

Comment 3 by jlebel@chromium.org, Jun 20 2018

The error message doesn't to be the real issue.
It looks like there is an issue to enable-disable-enable sync too fast.
https://paste.googleplex.com/5338444469370880

Comment 4 by jlebel@chromium.org, Jun 20 2018

Owner: mastiz@chromium.org

Comment 5 by jlebel@chromium.org, Jun 20 2018

Cc: jlebel@chromium.org
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 20 2018

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

commit fb745f666279d114ce0239d4a66520ab318c43c9
Author: Jérôme Lebel <jlebel@chromium.org>
Date: Wed Jun 20 15:03:49 2018

[iOS] Disabling flaky sync tests

Disabling EarlGrey tests from SigninInteractionControllerTestCase:
 + testSignInSwitchAccountsAndKeepDataSeparate
 + testSignInSwitchAccountsAndImportData
 + testSignInSwitchManagedAccount

Those tests seem to trigger an issue in the sync code when doing enable
disable enable too fast.

Bug:  854446 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I76d10d272297e91650dbe91ef7acee500eac6c75
Reviewed-on: https://chromium-review.googlesource.com/1107881
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Jérôme Lebel <jlebel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568843}
[modify] https://crrev.com/fb745f666279d114ce0239d4a66520ab318c43c9/ios/chrome/browser/ui/signin_interaction/signin_interaction_controller_egtest.mm

Comment 7 by mastiz@chromium.org, Jun 20 2018

Labels: -Pri-0 Pri-1
Status: Started (was: Assigned)
Adjusting to P1 after affected tests have been disabled.

My hypothesis is that ModelTypeController ignores stop events while sync is starting, which is reproduced in tests due to the fast enable-disable-enable sequence.

Although unlikely, if this conditions is met, it could involve leaking sync metadata after sync has been disabled (or for some datatypes, leaking data). I'll confirm this and file a separate bug for that.
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 21 2018

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

commit a2f928a4359865bb5a040916a43201870d42176f
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Jun 21 12:18:29 2018

Add DVLOG traces to ModelTypeController

They are motivated by an ongoing investigation for test
flakiness on iOS, with the suspicion that some of the codepaths
(namely, where calls to OnSyncStopping() could be dropped) can
cause issues (e.g. sync metadata being leaked).

Bug:  854446 
Change-Id: I6fe7da40c36d00e01afa176334f59f9dc66a74e1
Reviewed-on: https://chromium-review.googlesource.com/1108200
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569225}
[modify] https://crrev.com/a2f928a4359865bb5a040916a43201870d42176f/components/sync/driver/model_type_controller.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Jun 21 2018

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

commit 06ca703c06156bed1d5f54c4aeaa58497f527346
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Jun 21 15:32:44 2018

Improve sync enable-disable integration tests

We refactor and extend the tests to:
1. Add support for USS types (which requires migrating the tests from
   reading the directory directly).
2. Capture more complex sequences, combining multiple transitions.
3. Add disabled tests that are under investigation, due to flakiness
   reported on iOS integration tests the exercise analogous transitions.

Bug:  854446 
Change-Id: Ic6aa9294086a8511255a084af08112f3455f078b
Reviewed-on: https://chromium-review.googlesource.com/1110118
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569269}
[modify] https://crrev.com/06ca703c06156bed1d5f54c4aeaa58497f527346/chrome/browser/sync/test/integration/enable_disable_test.cc

Blockedon: 855375
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 25 2018

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

commit b4d3bf77f27e72b4448214e10205eb5216889f58
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Jun 25 14:55:08 2018

Propagate sync activation request to bridges

This struct includes authenticated_account_id, which several bridges are
interested in. This is safer and simpler to use compared to reading
from side channels, and prevents a runtime dependency cycle across some
factories.

TBR=estade@chromium.org,rockot@chromium.org

Bug: 850428, 834042 , 854446 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ib17a4503ff9d284e1a5f4e222e7eeb95b08a024f
Reviewed-on: https://chromium-review.googlesource.com/1111716
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570037}
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/chrome/browser/consent_auditor/consent_auditor_factory.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/chrome/browser/extensions/api/sessions/sessions_apitest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/chrome/browser/sync/user_event_service_factory.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/consent_auditor/consent_sync_bridge_impl.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/consent_auditor/consent_sync_bridge_impl.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/consent_auditor/consent_sync_bridge_impl_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/BUILD.gn
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/driver/model_type_controller.cc
[rename] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model/data_type_activation_request.cc
[rename] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model/data_type_activation_request.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/user_events/user_event_service_impl_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/user_events/user_event_sync_bridge.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/user_events/user_event_sync_bridge.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync/user_events/user_event_sync_bridge_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync_sessions/session_sync_bridge.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync_sessions/session_sync_bridge.h
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/components/sync_sessions/session_sync_bridge_unittest.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/ios/chrome/browser/consent_auditor/consent_auditor_factory.cc
[modify] https://crrev.com/b4d3bf77f27e72b4448214e10205eb5216889f58/ios/chrome/browser/sync/ios_user_event_service_factory.cc

Status update: I no longer repro failures on trunk (linux, newly added browser tests).

I suspect there is still a minor issue (and I have a fix for it), but since I can't make tests fail, I'm not convinced it will repro on iOS either.

Hence, I propose to reenable iOS tests and monitor flakes closely.
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 26 2018

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

commit 661767b98d165550e49b5a3f32ec3df80f48b6b2
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Jun 26 11:31:56 2018

Revert "[iOS] Disabling flaky sync tests"

This reverts commit fb745f666279d114ce0239d4a66520ab318c43c9.

Reason for revert: underlying issue no longer reproduces on
other platforms after recent improvements, so optimistically
assuming it was also fixed for iOS.

Original change's description:
> [iOS] Disabling flaky sync tests
> 
> Disabling EarlGrey tests from SigninInteractionControllerTestCase:
>  + testSignInSwitchAccountsAndKeepDataSeparate
>  + testSignInSwitchAccountsAndImportData
>  + testSignInSwitchManagedAccount
> 
> Those tests seem to trigger an issue in the sync code when doing enable
> disable enable too fast.
> 
> Bug:  854446 
> Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
> Change-Id: I76d10d272297e91650dbe91ef7acee500eac6c75
> Reviewed-on: https://chromium-review.googlesource.com/1107881
> Reviewed-by: Justin Cohen <justincohen@chromium.org>
> Commit-Queue: Jérôme Lebel <jlebel@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#568843}

TBR=justincohen@chromium.org,jlebel@chromium.org

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

Bug:  854446 
Change-Id: Ic809289ce4662d099250668d6f23100e52de9587
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/1114719
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570374}
[modify] https://crrev.com/661767b98d165550e49b5a3f32ec3df80f48b6b2/ios/chrome/browser/ui/signin_interaction/signin_interaction_controller_egtest.mm

I'd appreciate if iOS folks could advice on what should be monitored. Taking a look at flakiness dashboards ([1]), the update frequency is very low.

[1] https://test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&testType=ios_chrome_ui_egtests
Cc: mastiz@chromium.org
 Issue 857189  has been merged into this issue.
Disabling tests again, given the observed flakes, in https://chromium-review.googlesource.com/c/chromium/src/+/1117578
Project Member

Comment 17 by bugdroid1@chromium.org, Jun 27 2018

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

commit e6aa52bf65d037b1d119bd71c21eec17c5dcd341
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Jun 27 20:33:35 2018

Reland "[iOS] Disabling flaky sync tests"

This reverts commit 661767b98d165550e49b5a3f32ec3df80f48b6b2.

Reason for revert: tests still flaky.

Original change's description:
> Revert "[iOS] Disabling flaky sync tests"
> 
> This reverts commit fb745f666279d114ce0239d4a66520ab318c43c9.
> 
> Reason for revert: underlying issue no longer reproduces on
> other platforms after recent improvements, so optimistically
> assuming it was also fixed for iOS.
> 
> Original change's description:
> > [iOS] Disabling flaky sync tests
> > 
> > Disabling EarlGrey tests from SigninInteractionControllerTestCase:
> >  + testSignInSwitchAccountsAndKeepDataSeparate
> >  + testSignInSwitchAccountsAndImportData
> >  + testSignInSwitchManagedAccount
> > 
> > Those tests seem to trigger an issue in the sync code when doing enable
> > disable enable too fast.
> > 
> > Bug:  854446 
> > Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
> > Change-Id: I76d10d272297e91650dbe91ef7acee500eac6c75
> > Reviewed-on: https://chromium-review.googlesource.com/1107881
> > Reviewed-by: Justin Cohen <justincohen@chromium.org>
> > Commit-Queue: Jérôme Lebel <jlebel@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#568843}
> 
> TBR=justincohen@chromium.org,jlebel@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug:  854446 
> Change-Id: Ic809289ce4662d099250668d6f23100e52de9587
> Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
> Reviewed-on: https://chromium-review.googlesource.com/1114719
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#570374}

TBR=justincohen@chromium.org,jlebel@chromium.org,mastiz@chromium.org

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

Bug:  854446 
Change-Id: I2a0a2e9d60988783fca93ef8313594d57b7ab906
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/1117578
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570888}
[modify] https://crrev.com/e6aa52bf65d037b1d119bd71c21eec17c5dcd341/ios/chrome/browser/ui/signin_interaction/signin_interaction_controller_egtest.mm

 Issue 857186  has been merged into this issue.
Project Member

Comment 19 by bugdroid1@chromium.org, Jul 4

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

commit c8923dd5da2b94e56ccd7691f99ca6689c096348
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Jul 04 15:07:00 2018

Revert "Reland "[iOS] Disabling flaky sync tests""

This reverts commit e6aa52bf65d037b1d119bd71c21eec17c5dcd341.

Reason for revert: recent patches fix a related problem, in
23919ce6813da7b70e2d5cf488345f5f7f42fa1d. We haven't been able
to confirm the fix being effective for the flakes observed in
bots because they no longer repro locally, but it is definitely
related so let's reenable the tests and monitor flakes.

Original change's description:
> Reland "[iOS] Disabling flaky sync tests"
> 
> This reverts commit 661767b98d165550e49b5a3f32ec3df80f48b6b2.
> 
> Reason for revert: tests still flaky.
> 
> Original change's description:
> > Revert "[iOS] Disabling flaky sync tests"
> > 
> > This reverts commit fb745f666279d114ce0239d4a66520ab318c43c9.
> > 
> > Reason for revert: underlying issue no longer reproduces on
> > other platforms after recent improvements, so optimistically
> > assuming it was also fixed for iOS.
> > 
> > Original change's description:
> > > [iOS] Disabling flaky sync tests
> > > 
> > > Disabling EarlGrey tests from SigninInteractionControllerTestCase:
> > >  + testSignInSwitchAccountsAndKeepDataSeparate
> > >  + testSignInSwitchAccountsAndImportData
> > >  + testSignInSwitchManagedAccount
> > > 
> > > Those tests seem to trigger an issue in the sync code when doing enable
> > > disable enable too fast.
> > > 
> > > Bug:  854446 
> > > Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
> > > Change-Id: I76d10d272297e91650dbe91ef7acee500eac6c75
> > > Reviewed-on: https://chromium-review.googlesource.com/1107881
> > > Reviewed-by: Justin Cohen <justincohen@chromium.org>
> > > Commit-Queue: Jérôme Lebel <jlebel@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#568843}
> > 
> > TBR=justincohen@chromium.org,jlebel@chromium.org
> > 
> > # Not skipping CQ checks because original CL landed > 1 day ago.
> > 
> > Bug:  854446 
> > Change-Id: Ic809289ce4662d099250668d6f23100e52de9587
> > Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
> > Reviewed-on: https://chromium-review.googlesource.com/1114719
> > Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> > Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#570374}
> 
> TBR=justincohen@chromium.org,jlebel@chromium.org,mastiz@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug:  854446 
> Change-Id: I2a0a2e9d60988783fca93ef8313594d57b7ab906
> Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
> Reviewed-on: https://chromium-review.googlesource.com/1117578
> Reviewed-by: Mikel Astiz <mastiz@chromium.org>
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#570888}

TBR=justincohen@chromium.org,jlebel@chromium.org,mastiz@chromium.org

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

Bug:  854446 
Change-Id: I5874890bed60c173c35e49fe86933a06bd214120
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/1126260
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572570}
[modify] https://crrev.com/c8923dd5da2b94e56ccd7691f99ca6689c096348/ios/chrome/browser/ui/signin_interaction/signin_interaction_controller_egtest.mm

Status: Fixed (was: Started)
No flakes in more than a week, so considering this finally fixed!

Sign in to add a comment