Commit b4ffd219 authored by mattcary's avatar mattcary Committed by Commit bot

Fix race condition in InstallerDelegateTest.

Remove the unnecessary and dangerous polling in startMonitoring(). mTestDelegate.mIsRunning is set directly in start(), so waiting for mTestDelegate.isRunning() does not guarantee anything. However, it does introduce the possibility of a race if the mHandler.postDelayed() call in mTestDelegate.start() switches threads.

Precisely, if mHandler.postDelayed() stays in the same thread, or causes uninteresting thread switches, the following happens in startMonitoring:

mTestDelegate.start ->
 mIsRunning = true;
 ...

CriteriaHelper.pollForCriteria ->
 mTestDeleted.isRunning() == true, exit immediately.

On the other hand, if there is an interesting thread switch in mHandler.postDelayed(), then we could have

mTestDelegate.start ->
 mIsRunning = true;
 mHandler.postDelayed()
  ... stuff happens ...
  mTestDelegate.run() is called
   mIsRunning = false;
  ... more stuff happens ..

(now back to main test thread)
CriteriaHelper.pollForCriteria ->
 mTestDeleted.isRunning() == false, loop until timeout and the test fails.

Before removing this poll, running locally the test occasionally retried tests on a nexus 4; after removing it it ran 20 times successfully with no retries. I think we have slightly higher confidence now that it won't flake on the build.

BUG=542627

Review URL: https://codereview.chromium.org/1634693002

Cr-Commit-Position: refs/heads/master@{#371477}
parent 41bffb13
...@@ -6,7 +6,6 @@ package org.chromium.chrome.browser.banners; ...@@ -6,7 +6,6 @@ package org.chromium.chrome.browser.banners;
import android.content.pm.PackageInfo; import android.content.pm.PackageInfo;
import android.os.HandlerThread; import android.os.HandlerThread;
import android.test.FlakyTest;
import android.test.InstrumentationTestCase; import android.test.InstrumentationTestCase;
import android.test.mock.MockPackageManager; import android.test.mock.MockPackageManager;
import android.test.suitebuilder.annotation.SmallTest; import android.test.suitebuilder.annotation.SmallTest;
...@@ -91,14 +90,6 @@ public class InstallerDelegateTest extends InstrumentationTestCase ...@@ -91,14 +90,6 @@ public class InstallerDelegateTest extends InstrumentationTestCase
private void startMonitoring() throws InterruptedException { private void startMonitoring() throws InterruptedException {
mTestDelegate.start(); mTestDelegate.start();
mInstallStarted = true; mInstallStarted = true;
// Wait until we know that the Thread is running the InstallerDelegate task.
CriteriaHelper.pollForCriteria(new Criteria() {
@Override
public boolean isSatisfied() {
return mTestDelegate.isRunning();
}
});
} }
private void checkResults(boolean expectedResult) throws InterruptedException { private void checkResults(boolean expectedResult) throws InterruptedException {
...@@ -159,8 +150,6 @@ public class InstallerDelegateTest extends InstrumentationTestCase ...@@ -159,8 +150,6 @@ public class InstallerDelegateTest extends InstrumentationTestCase
/** /**
* Makes sure that the runnable isn't called until returning from start(). * Makes sure that the runnable isn't called until returning from start().
*/ */
/* Appears to be flaky crbug.com/542627 */
@FlakyTest
@SmallTest @SmallTest
public void testRunnableRaceCondition() throws InterruptedException { public void testRunnableRaceCondition() throws InterruptedException {
mPackageManager.isInstalled = true; mPackageManager.isInstalled = true;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment