Not sure what this class is for. +STO folk.

I wrote AliveChecker and its test. It sure seems like it could be a test problem. I can take a closer look next week.

wez@: is there any way you can confirm it happens at a timeout?

I looks like what happens is that the |alive_checker_thread_| is still running and executes the delayed task NotifyAliveMultipleTimes(), but the |alive_checker_| is gone.

Re #5: Looking at the test, the  NotifyAliveMultipleTimes posts a fixed number of delayed tasks on the AliveChecker, regardless of whether it has been stopped, or destroyed (see 
https://cs.chromium.org/chromium/src/media/audio/alive_checker_unittest.cc?sq=package:chromium&l=120).

The test will therefore crash if there is enough of a delay between the ResetAliveCheckerOnAliveCheckerThread task being processed, and the thread being torn-down.

Incidentally, we also seem to have NotifyAliveMultipleTimes notifying the AliveChecker the first time on the calling thread, but on subsequent times on the AliveChecker thread. There also seems to be a fair amount of redundant thread-hopping, e.g. the test posts a task to create the AliveChecker on the alive-checker-thread, and then waits for that task to complete; since all the other interaction with the AliveChecker is by tasks posted to the thread, the waiting doesn't seem necessary, and just adds complexity.

I'm able to repro a crash with this patch:

--- a/media/audio/alive_checker_unittest.cc
+++ b/media/audio/alive_checker_unittest.cc
@@ -86,6 +86,11 @@ class AliveCheckerTest : public testing::Test {
         base::BindOnce(&AliveCheckerTest::ResetAliveCheckerOnAliveCheckerThread,
                        base::Unretained(this), &done));
     done.Wait();
+
+    // Allow the AliveChecker thread to continue running long enough to process
+    // one more notification after it has processed the reset task.
+    base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(kNotifyIntervalMs));
+
   }
 
   void CreateAliveChecker(bool stop_at_first_alive_notification,
@@ -121,6 +126,10 @@ class AliveCheckerTest : public testing::Test {
                                 base::TimeDelta delay) {
     alive_checker_->NotifyAlive();
     if (remaining_notifications > 1) {
+
+      // Pause long enough for the AliveChecker to think we're "dead".
+      base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(kTimeoutMs));
+
       alive_checker_thread_.task_runner()->PostDelayedTask(
           FROM_HERE,
           base::BindOnce(&AliveCheckerTest::NotifyAliveMultipleTimes,

This introduces a delay after NotifyAlive(), long enough to cause there to be NotifyAliveMultipleTimes tasks pending even after the WaitUntilDetectedDeadWithTimeout deadline (normally all the tasks would run within kNotifyIntervalMs * 10).  We then introduce a delay after the reset task is posted, long enough to allow any in-flight NotifyAliveMultipleTimes task to run - then we see one of those execute, and crash trying to use the already-reset AliveChecker.

You can get the same effect by adding the Sleep() during teardown, but increasing the number of NotifyAliveMultipleTimes() notifications, rather than putting the Sleep in there, since that also causes there to be a delayed task pending that will touch the deleted object if they run.

I'm not sure whether the intention is actually that NotifyAliveMultipleTimes() is a blocking call; that isn't clear from the thread, and since most of the other calls are blocking, it may be that the test author assumed that one is blocking as well, and the design of the test certainly implies that - with non-blocking NotifyAliveMultipleTimes I don't think the test waits for long enough to detect a dead signal, right now.

Incidentally, it would be helpful to add some comments to describe the behaviours the tests are expecting - the current comment describes more or less verbatim what calls the test makes, without explaining _why_ those calls are testing something useful. In the case of NotifyThenStop, IIUC the idea is that after Stop()ing the checker, you should never get a "dead" notification, no matter what.

HTH

FWIW, my recommendation would be to remove the alive-checker-thread and instead rebuild these tests using a mock clock.

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

commit 165013536a7eee0361dd46be6fe210dcec094d28
Author: Wez <wez@chromium.org>
Date: Wed Dec 13 00:37:49 2017

Disable some AliveCheckerTests, which are inherently flaky.

Bug: 789804
Change-Id: I58f9b55c85efd1e3dc6cca47798aa6e3b2aad1b7
Reviewed-on: https://chromium-review.googlesource.com/822732
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523624}
[modify] https://crrev.com/165013536a7eee0361dd46be6fe210dcec094d28/media/audio/alive_checker_unittest.cc

AliveCheckterTest.NotifyThenDetectDead flaked on the Fuchsia (dbg) bot:
https://build.chromium.org/p/chromium.fyi/builders/Fuchsia%20%28dbg%29/builds/15163

This is another test which uses NotifyAliveMultipleTimes(), leading to a crash:
#01: pc 0x7d89e3a74416 sp 0xb731cdfcd80 (libmedia.so,0x253416)
#02: pc 0x7d89e3a73e63 sp 0xb731cdfcef0 (libmedia.so,0x252e63)
#03: media::AliveCheckerTest::NotifyAliveMultipleTimes(int, base::TimeDelta) at ??:?
#04: void base::internal::FunctorTraits<void (media::AliveCheckerTest::*)(int, base::TimeDelta), void>::Invoke<media::AliveCheckerTest*, int, base::TimeDelta>(void (media::AliveCheckerTest::*)(int, base::TimeDelta), media::AliveCheckerTest*&&, int&&, base::TimeDelta&&) at ??:?
#05: void base::internal::InvokeHelper<false, void>::MakeItSo<void (media::AliveCheckerTest::*)(int, base::TimeDelta), media::AliveCheckerTest*, int, base::TimeDelta>(void (media::AliveCheckerTest::*&&)(int, base::TimeDelta), media::AliveCheckerTest*&&, int&&, base::TimeDelta&&) at ??:?
#06: void base::internal::Invoker<base::internal::BindState<void (media::AliveCheckerTest::*)(int, base::TimeDelta), base::internal::UnretainedWrapper<media::AliveCheckerTest>, int, base::TimeDelta>, void ()>::RunImpl<void (media::AliveCheckerTest::*)(int, base::TimeDelta), std::__1::tuple<base::internal::UnretainedWrapper<media::AliveCheckerTest>, int, base::TimeDelta>, 0ul, 1ul, 2ul>(void (media::AliveCheckerTest::*&&)(int, base::TimeDelta), std::__1::tuple<base::internal::UnretainedWrapper<media::AliveCheckerTest>, int, base::TimeDelta>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul>) at ??:?
#07: base::internal::Invoker<base::internal::BindState<void (media::AliveCheckerTest::*)(int, base::TimeDelta), base::internal::UnretainedWrapper<media::AliveCheckerTest>, int, base::TimeDelta>, void ()>::RunOnce(base::internal::BindStateBase*) at ??:?
...

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

commit 8a5ef6b01900b3ca19827854fbafd754f3e2e197
Author: Wez <wez@chromium.org>
Date: Wed Dec 20 21:11:46 2017

Disable some more AliveCheckerTests.

This disables the remaining tests which use the NotifyAliveMultipleTimes
helper function, until they can be rewritten to use e.g. a MOCK_TIME
ScopedTaskEnvironment instance.

Bug: 789804
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I496fd5c76ebaed8df56a6c8b93225a2d3b0fe493
Reviewed-on: https://chromium-review.googlesource.com/836736
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525454}
[modify] https://crrev.com/8a5ef6b01900b3ca19827854fbafd754f3e2e197/media/audio/alive_checker_unittest.cc

AliveCheckerTest.SuspendResumeWithAutoStop_NotifyBeforeSuspendAndRestart failed on fuchsia/x64/Debug in https://build.chromium.org/p/chromium.fyi/builders/Fuchsia%20%28dbg%29/builds/16935; the test itself failed some expectations:

[00462.339] 04060.04119> [ RUN      ] AliveCheckerTest.SuspendResumeWithAutoStop_NotifyBeforeSuspendAndRestart
[00462.339] 04060.04119> ../../media/audio/alive_checker_unittest.cc:455: Failure
[00462.340] 04060.04119> Value of: GetDetectedDead()
[00462.340] 04060.04119>   Actual: true
[00462.340] 04060.04119> Expected: false
[00462.340] 04060.04119> ../../media/audio/alive_checker_unittest.cc:462: Failure
[00462.340] 04060.04119> Value of: WaitUntilDetectedDeadWithTimeout( base::TimeDelta::FromMilliseconds(kTimeoutMs + kCheckIntervalMs + 10))
[00462.341] 04060.04119>   Actual: true
[00462.341] 04060.04119> Expected: false
[00462.341] 04060.04119> ../../media/audio/alive_checker_unittest.cc:463: Failure
[00462.342] 04060.04119> Value of: GetDetectedDead()
[00462.342] 04060.04119>   Actual: true
[00462.343] 04060.04119> Expected: false

and it then appears to have stalled or taken sufficiently long that it was terminated by the TestLauncher.