New issue
Advanced search Search tips

Issue 799941 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

"org.chromium.chrome.browser.sync.ui.PassphraseActivityTest#testCallbackAfterBackgrounded" is flaky

Project Member Reported by chromium...@appspot.gserviceaccount.com, Jan 8 2018

Issue description

"org.chromium.chrome.browser.sync.ui.PassphraseActivityTest#testCallbackAfterBackgrounded" is flaky.

This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label.

We have detected 3 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyYwsSBUZsYWtlIlhvcmcuY2hyb21pdW0uY2hyb21lLmJyb3dzZXIuc3luYy51aS5QYXNzcGhyYXNlQWN0aXZpdHlUZXN0I3Rlc3RDYWxsYmFja0FmdGVyQmFja2dyb3VuZGVkDA.

Flaky tests should be disabled within 30 minutes unless culprit CL is found and reverted. Please see more details here: https://sites.google.com/a/chromium.org/dev/developers/tree-sheriffs/sheriffing-bug-queues#triaging-auto-filed-flakiness-bugs
 
Owner: wittman@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 8 2018

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

commit 49ff8037f6ca8b3924769bce5fb484d798c69cc7
Author: Mike Wittman <wittman@chromium.org>
Date: Mon Jan 08 21:59:19 2018

Disable org.chromium.chrome.browser.sync.ui.PassphraseActivityTest#testCallbackAfterBackgrounded

Test has been flakily crashing.

See https://luci-milo.appspot.com/buildbot/tryserver.chromium.android/linux_android_rel_ng/462719

I  790.983s run_tests_on_device(060ddd7af0ea8220)  detected failure in org.chromium.chrome.browser.sync.ui.PassphraseActivityTest#testCallbackAfterBackgrounded. raw output:
I  790.983s run_tests_on_device(060ddd7af0ea8220)    INSTRUMENTATION_STATUS: numtests=1
I  790.983s run_tests_on_device(060ddd7af0ea8220)    INSTRUMENTATION_STATUS: stream=
I  790.983s run_tests_on_device(060ddd7af0ea8220)    org.chromium.chrome.browser.sync.ui.PassphraseActivityTest:
I  790.983s run_tests_on_device(060ddd7af0ea8220)    INSTRUMENTATION_STATUS: id=AndroidJUnitRunner
I  790.983s run_tests_on_device(060ddd7af0ea8220)    INSTRUMENTATION_STATUS: test=testCallbackAfterBackgrounded
I  790.983s run_tests_on_device(060ddd7af0ea8220)    INSTRUMENTATION_STATUS: class=org.chromium.chrome.browser.sync.ui.PassphraseActivityTest
I  790.983s run_tests_on_device(060ddd7af0ea8220)    INSTRUMENTATION_STATUS: current=1
I  790.983s run_tests_on_device(060ddd7af0ea8220)    INSTRUMENTATION_STATUS_CODE: 1
I  790.983s run_tests_on_device(060ddd7af0ea8220)    INSTRUMENTATION_RESULT: shortMsg=Process crashed.
I  790.983s run_tests_on_device(060ddd7af0ea8220)    INSTRUMENTATION_CODE: 0

TBR=nyquist,bsazonov

Bug:  799941 
Change-Id: I629484e71a7c4e58521dd121804d26d3e5ea5a6b
Reviewed-on: https://chromium-review.googlesource.com/854617
Reviewed-by: Mike Wittman <wittman@chromium.org>
Commit-Queue: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527773}
[modify] https://crrev.com/49ff8037f6ca8b3924769bce5fb484d798c69cc7/chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java

Labels: -Sheriff-Chromium
Owner: bsazonov@chromium.org
Status: Assigned (was: Started)
Unfortunately I think that disabling testCallbackAfterBackgrounded won't help to address flakiness around oauth2_token_service_delegate_android.cc. There were several reports of this DCHECK failing on startup: issue 541297, issue 698778.
As this test doesn't do anything specific to accounts, it looks like disabling it won't reduce the flakiness. I'm going to add debugging info to that DCHECK and re-enable the test.
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 11 2018

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

commit 71dc85fdef6c994ce379c087f2ed5b6d6144f4fe
Author: Boris Sazonov <bsazonov@chromium.org>
Date: Thu Jan 11 15:33:46 2018

[Signin][Android] Add info to AccountTokenServiceDelegateAndroid DCHECK

This CL adds debugging info to DCHECK that has been causing test
failures. It also re-enables the test that was disabled because of
this check.

Bug: 541297, 698778,  799941 
Change-Id: Id2cca4d7fedd19c2b6cf2b808fe41338088d75e8
Reviewed-on: https://chromium-review.googlesource.com/857177
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528633}
[modify] https://crrev.com/71dc85fdef6c994ce379c087f2ed5b6d6144f4fe/chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java
[modify] https://crrev.com/71dc85fdef6c994ce379c087f2ed5b6d6144f4fe/chrome/browser/signin/oauth2_token_service_delegate_android.cc

Labels: OS-Android
OK, I've dug to the bottom of it. The issue has been introduced by https://crrev.com/c/817444, because now account list cache in AccountManagerFacade is updated from background thread. This makes the possible scenario possible:

1. AccountTrackerService seeds a list of accounts.
2. Account list is updated from background thread (observers aren't notified yet, as it must happen on UI thread).
3. OAuth2TokenService.validateAccounts checks whether accounts have been seeded by calling AccountTrackerService.checkAndSeedSystemAccounts.
4. As AccountTrackerService doesn't know that account list has been changed, checkAndSeedSystemAccounts returns true.
5. OAuth2TokenService.getSystemAccountNames gets a list of accounts (it's the one from step 2).
6. Some accounts in the list haven't been seeded, so DCHECK fires.
I've uploaded a CL that should fix this issue: https://crrev.com/c/865394. I've tried running PassphraseActivityTest 400 times with this patch applied and there were zero crashes. Without this patch approximately one run out of five ends in a crash.
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 15 2018

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

commit 63243a2ae1b4b20ecfcacb6d7c823a6fe1ca9a92
Author: Boris Sazonov <bsazonov@chromium.org>
Date: Mon Jan 15 12:36:51 2018

[Signin][Android] Fix race condition in accounts seeding

This CL changes the way the account list cache is updated in
AccountManagerFacade. Now the cache is populated on startup by
AccountManagerFacade.InitializeTask on a background thread (as main
thread may be waiting for the same cache to be populated). After
initialization is completed, all changes to the account list are handled
by UpdateTask, that always updates the cache on the main thread. This
approach solves a race condition that has been causing test failures and
simplifies reasoning about the state of the account list in general.

Bug:  799941 
Change-Id: I98635c5bc44e5b4dc392a604e2755cc04cb9f42d
Reviewed-on: https://chromium-review.googlesource.com/865394
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529256}
[modify] https://crrev.com/63243a2ae1b4b20ecfcacb6d7c823a6fe1ca9a92/components/signin/core/browser/android/java/src/org/chromium/components/signin/AccountManagerFacade.java

Status: Fixed (was: Assigned)
There hasn't been any flakes after Jan 15, closing the bug.

Sign in to add a comment