From 064cda85314ceedc980434a5b6536d98ab98d0ce Mon Sep 17 00:00:00 2001
From: Menghan Yang <[email protected]>
Date: Tue, 05 May 2026 09:20:51 -0700
Subject: [PATCH] [Gardener] Revert "Reland "Fix stream closure issues in AAudioStreamWrapper""

This reverts commit 279912fa38186a0480f4a6068d3ff7bc422e0afc.

Reason for revert: suspected to cause the failure of
AudioAndroidInputTest and other audio related tests on multiple bots.
e.g.:
https://ci.chromium.org/ui/p/chromium/builders/ci/android-11-x86-rel/24908/overview

Original change's description:
> Reland "Fix stream closure issues in AAudioStreamWrapper"
>
> This reverts commit 6fd47a31d0139633f12b148acb1ec091807ba890.
>
> Reason for revert: fixed failing tests
>
> Original change's description:
> > Revert "Fix stream closure issues in AAudioStreamWrapper"
> >
> > This reverts commit 272b9db99686ec5e68d9b75e5508d27d269cd991.
> >
> > Reason for revert:
> > LUCI Bisection has identified this change as the cause of a test failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/test-analysis/b/6517095274643456
> >
> > Sample build with failed test: https://ci.chromium.org/b/8683241393451216641
> > Affected test(s):
> > [://chromecast/media\:cast_media_unittests!gtest::CastAudioManagerTest#CanMakeCommunicationsStream](https://ci.chromium.org/ui/test/chromium/:%2F%2Fchromecast%2Fmedia%5C:cast_media_unittests%21gtest::CastAudioManagerTest%23CanMakeCommunicationsStream?q=VHash%3A1babfc145fe3d0dd)
> > [://chromecast/media\:cast_media_unittests!gtest::CastAudioManagerTest#CanMakeMixerStream](https://ci.chromium.org/ui/test/chromium/:%2F%2Fchromecast%2Fmedia%5C:cast_media_unittests%21gtest::CastAudioManagerTest%23CanMakeMixerStream?q=VHash%3A1babfc145fe3d0dd)
> > [://chromecast/media\:cast_media_unittests!gtest::CastAudioManagerTest#CanMakeStream](https://ci.chromium.org/ui/test/chromium/:%2F%2Fchromecast%2Fmedia%5C:cast_media_unittests%21gtest::CastAudioManagerTest%23CanMakeStream?q=VHash%3A1babfc145fe3d0dd)
> >
> > If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Ftest-analysis%2Fb%2F6517095274643456&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F7791487&type=BUG
> >
> > Original change's description:
> > > Fix stream closure issues in AAudioStreamWrapper
> > >
> > > Older versions of Android have bugs in AAudio, where callbacks keep
> > > being run after `AAudioStream_requestStop()` is called. To mitigate
> > > this issue, we added an AAudioDestructionHelper class, which outlives
> > > the chrome wrappers and remains alive for the pending callbacks.
> > >
> > > The helper class also deferred the closure of the AAudio stream until
> > > it was destroyed.
> > >
> > > This CL attempts to fix potential UAF issues if the aaudio stream is
> > > closed and the callback helper destroyed, while a callback is running.
> > > This is done by moving the `AAudioStream_close()` call outside of the
> > > destructor, and making sure locks are properly acquired before doing
> > > so.
> > >
> > > The CL also slightly lengthens the teardown process on older version
> > > of android, by delaying the helper's destructon by 250ms after closing
> > > the AAudio stream.
> > >
> > > Bug: 501331457
> > > Change-Id: I1d7c9eda365823deff6d92990d598f61dfc928c1
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7791487
> > > Commit-Queue: Thomas Guilbert <[email protected]>
> > > Reviewed-by: Dale Curtis <[email protected]>
> > > Cr-Commit-Position: refs/heads/main@{#1622048}
> > >
> >
> > Bug: 501331457
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Change-Id: I84eab6b9ced8ca2678a35ebcd47c09848116f8ca
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7801900
> > Commit-Queue: Thomas Guilbert <[email protected]>
> > Bot-Commit: [email protected] <[email protected]>
> > Reviewed-by: Thomas Guilbert <[email protected]>
> > Cr-Commit-Position: refs/heads/main@{#1622099}
>
> Bug: 501331457
> Change-Id: Ic3b7834c1e87ede6e660315bcb2dd24eac1e8ef9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7801907
> Commit-Queue: Thomas Guilbert <[email protected]>
> Reviewed-by: Dale Curtis <[email protected]>
> Auto-Submit: Thomas Guilbert <[email protected]>
> Reviewed-by: Simeon Anfinrud <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1625048}

Bug: 501331457
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: Ia8d29489365bc60e2cb485f31e101fae02afbd19
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7817176
Auto-Submit: Menghan Yang <[email protected]>
Reviewed-by: Dale Curtis <[email protected]>
Owners-Override: Dale Curtis <[email protected]>
Commit-Queue: Dale Curtis <[email protected]>
Bot-Commit: [email protected] <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1625490}
---

diff --git a/chromecast/media/audio/cast_audio_manager_unittest.cc b/chromecast/media/audio/cast_audio_manager_unittest.cc
index c5f71951..f7bbbaa0 100644
--- a/chromecast/media/audio/cast_audio_manager_unittest.cc
+++ b/chromecast/media/audio/cast_audio_manager_unittest.cc
@@ -67,8 +67,7 @@
 class CastAudioManagerTest : public testing::Test {
  public:
   CastAudioManagerTest()
-      : audio_thread_("CastAudioThread", base::Thread::Restartable{}),
-        task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
+      : audio_thread_("CastAudioThread", base::Thread::Restartable{}) {}
 
   void SetUp() override { CreateAudioManagerForTesting(); }
 
@@ -150,7 +149,7 @@
   }
 
   void RunThreadsUntilIdle() {
-    task_environment_.FastForwardUntilNoTasksRemain();
+    task_environment_.RunUntilIdle();
     audio_thread_.FlushForTesting();
   }
 
@@ -224,7 +223,6 @@
     RunThreadsUntilIdle();
   }
   stream->Close();
-  RunThreadsUntilIdle();
 }
 
 #if BUILDFLAG(ENABLE_PLATFORM_DTS_AUDIO)
@@ -248,7 +246,6 @@
     RunThreadsUntilIdle();
   }
   stream->Close();
-  RunThreadsUntilIdle();
 }
 #endif  // BUILDFLAG(ENABLE_PLATFORM_DTS_AUDIO))
 #endif  // BUILDFLAG(IS_ANDROID)
@@ -300,7 +297,6 @@
   RunThreadsUntilIdle();
 
   stream->Close();
-  RunThreadsUntilIdle();
 }
 
 TEST_F(CastAudioManagerTest, CanMakeCommunicationsStream) {
@@ -322,7 +318,6 @@
   task_environment_.RunUntilIdle();
 
   stream->Close();
-  RunThreadsUntilIdle();
 }
 
 }  // namespace chromecast::media
diff --git a/media/audio/android/aaudio_stream_wrapper.cc b/media/audio/android/aaudio_stream_wrapper.cc
index 9875e498..2d3ae4bb 100644
--- a/media/audio/android/aaudio_stream_wrapper.cc
+++ b/media/audio/android/aaudio_stream_wrapper.cc
@@ -34,8 +34,6 @@
 
 namespace {
 
-constexpr base::TimeDelta kCloseDelay = base::Seconds(1);
-
 constexpr char kAAudioBufferSizeInFramesMetricsPrefix[] =
     "Media.Audio.Android.AAudioBufferSizeInFrames.";
 constexpr char kAAudioFramesPerDataCallbackMetricsPrefix[] =
@@ -183,23 +181,9 @@
 
   ~AAudioDestructionHelper() {
     CHECK(is_closing_);
-    CHECK(!aaudio_stream_);
-  }
-
-  // Called on the default sequence.
-  static void CloseStreamAndDestroySoon(
-      std::unique_ptr<AAudioDestructionHelper> helper) {
-    // `AAudioStream_requestStop()` should have been called at least
-    // `kCloseDelay` ago, and callbacks should have hopefully stopped.
-    helper->CloseStream();
-
-    // Further delay destroying `helper` by a moderate amount, out of an
-    // abundance of caution, in case calling `AAudioStream_close()` is really
-    // the way to stop callbacks on older version of android.
-    constexpr base::TimeDelta kDestructionDelay = kCloseDelay / 4;
-    base::SequencedTaskRunner::GetCurrentDefault()->PostDelayedTask(
-        FROM_HERE, base::DoNothingWithBoundArgs(std::move(helper)),
-        kDestructionDelay);
+    if (aaudio_stream_) {
+      AAudioStream_close(aaudio_stream_);
+    }
   }
 
   AAudioStreamWrapper* GetAndLockWrapper() EXCLUSIVE_LOCK_FUNCTION() {
@@ -209,8 +193,6 @@
 
   void UnlockWrapper() UNLOCK_FUNCTION() { lock_.Release(); }
 
-  // Saves `stream` to be closed later. `CloseStream()` must be called
-  // explicitly before the helper is destroyed.
   void DeferStreamClosure(AAudioStream* stream) {

diff --git a/chromecast/media/audio/cast_audio_manager_unittest.cc b/chromecast/media/audio/cast_audio_manager_unittest.cc
index c5f71951..f7bbbaa0 100644
--- a/chromecast/media/audio/cast_audio_manager_unittest.cc
+++ b/chromecast/media/audio/cast_audio_manager_unittest.cc
@@ -67,8 +67,7 @@
 class CastAudioManagerTest : public testing::Test {
  public:
   CastAudioManagerTest()
-      : audio_thread_("CastAudioThread", base::Thread::Restartable{}),
-        task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {}
+      : audio_thread_("CastAudioThread", base::Thread::Restartable{}) {}
 
   void SetUp() override { CreateAudioManagerForTesting(); }
 
@@ -150,7 +149,7 @@
   }
 
   void RunThreadsUntilIdle() {
-    task_environment_.FastForwardUntilNoTasksRemain();
+    task_environment_.RunUntilIdle();
     audio_thread_.FlushForTesting();
   }
 
@@ -224,7 +223,6 @@
     RunThreadsUntilIdle();
   }
   stream->Close();
-  RunThreadsUntilIdle();
 }
 
 #if BUILDFLAG(ENABLE_PLATFORM_DTS_AUDIO)
@@ -248,7 +246,6 @@
     RunThreadsUntilIdle();
   }
   stream->Close();
-  RunThreadsUntilIdle();
 }
 #endif  // BUILDFLAG(ENABLE_PLATFORM_DTS_AUDIO))
 #endif  // BUILDFLAG(IS_ANDROID)
@@ -300,7 +297,6 @@
   RunThreadsUntilIdle();
 
   stream->Close();
-  RunThreadsUntilIdle();
 }
 
 TEST_F(CastAudioManagerTest, CanMakeCommunicationsStream) {
@@ -322,7 +318,6 @@
   task_environment_.RunUntilIdle();
 
   stream->Close();
-  RunThreadsUntilIdle();
 }
 
 }  // namespace chromecast::media