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

Issue 731485 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

"org.chromium.chrome.browser.webapps.WebApkUpdateManagerTest#testCanonicalUrlsDifferentShouldUpgrade" is flaky

Project Member Reported by chromium...@appspot.gserviceaccount.com, Jun 9 2017

Issue description

"org.chromium.chrome.browser.webapps.WebApkUpdateManagerTest#testCanonicalUrlsDifferentShouldUpgrade" 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 5 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNybgsSBUZsYWtlImNvcmcuY2hyb21pdW0uY2hyb21lLmJyb3dzZXIud2ViYXBwcy5XZWJBcGtVcGRhdGVNYW5hZ2VyVGVzdCN0ZXN0Q2Fub25pY2FsVXJsc0RpZmZlcmVudFNob3VsZFVwZ3JhZGUM.

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

This flaky test/step was previously tracked in  issue 673385 .
 
Labels: -Sheriff-Chromium OS-Android
Owner: pkotw...@chromium.org
Status: Assigned (was: Untriaged)
Reverting https://codereview.chromium.org/2927723003/
Cc: yolandyan@chromium.org
 Issue 731514  has been merged into this issue.
The original CL https://codereview.chromium.org/2935853002/ has been relanded with a fix for the test flakiness

Cc: dominickn@chromium.org wnwen@chromium.org
From the CL discussion:
>> Thank you for your question! It forced me to do more investigation. The strict
>> mode exception is thrown by SharedPreferencesImpl#awaitLoadLocked() if the
>> SharedPreferences have not finished loading.
>>
http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/...
>> 
>> We would get a strict mode exception if we tried to access SharedPreferences
>> immediately after the strict mode block in
WebappActivity#preInflationStartup()
>> (I checked that this is the case)

> Perhaps this bit of code should be moved to warmUpSharedPrefs then? The whole
> purpose of warming shared prefs is to avoid this issue in one go....

I talked to our "Strict Mode" expert wnwen@. He said that Strict Mode violations due to accessing SharedPreferences before they have finished loading are not a big deal and are to be expected. He explained that the idea of WebappDataStorage#warmUpSharedPrefs() is to make it less likely that a Strict Mode violation occurs (Because doing disk IO on the UI thread is slow) not to guarantee that a Strict Mode violation does not occur.

Comment 5 by wnwen@chromium.org, Jun 14 2017

Thanks for digging in, Peter.

Adding the timeline of warming up shared preferences:

1 - Fetching the preference for the first time, Android kicks of a new thread to read the file from disk
2 - UI thread does work, during this time the file is being read
3 - UI thread reads a value from shared preferences, Android sees that the new thread has not finished reading the file and raises a StrictMode violation, also blocks on the thread finishing
4 - UI thread blocked for a short period of time until the file is read and parsed.

If we were to read the value in #3 in warm up, which is #1, we skip #2 which is the entire point of warming up shared preferences. It is unfortunate that Android raises a StrictMode violation in #3, but the reason is to alert us in case we reach #3 too soon all the time, in which case we need to call warmup earlier or acknowledge that warmup isn't doing much.

Thanks again for considering this in detail rather than just turning it off.
Status: Fixed (was: Assigned)

Sign in to add a comment