Fix race condition in InstallerDelegateTest.

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
Committed: https://crrev.com/b4ffd219c5633ddd3eaa4ee20832e83cc0320e31
Cr-Commit-Position: refs/heads/master@{#371477}

[mattcary, 2016-01-25 16:10:55]

Description was changed from

==========
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.


BUG=542627
==========

to

==========
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
==========

[mattcary, 2016-01-25 16:14:09]

Description was changed from

==========
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
==========

to

==========
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
==========

[mattcary, 2016-01-25 16:14:09]

mattcary@chromium.org changed reviewers:
+ dfalcantara@chromium.org, pasko@chromium.org

[gone, 2016-01-25 18:20:13]

lgtm

[pasko, 2016-01-25 18:43:42]

lgtm, thank you

[mattcary, 2016-01-26 08:07:24]

The CQ bit was checked by mattcary@chromium.org

[commit-bot: I haz the power, 2016-01-26 08:07:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1634693002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1634693002/20001

[commit-bot: I haz the power, 2016-01-26 08:40:21]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-01-26 08:40:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-26 08:41:33]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/b4ffd219c5633ddd3eaa4ee20832e83cc0320e31
Cr-Commit-Position: refs/heads/master@{#371477}
==========

[commit-bot: I haz the power, 2016-01-26 08:41:33]

Patchset 2 (id:??) landed as
https://crrev.com/b4ffd219c5633ddd3eaa4ee20832e83cc0320e31
Cr-Commit-Position: refs/heads/master@{#371477}