[go: nahoru, domu]

Revert "[M123] [Audio] Propagate socket errors to sinks and sources. Prevent reuse."

This reverts commit 0105c8e541581f18d0a7f6e3a8d629f917385550.

Reason for revert: Exposing latent threading issues in audio input device users and not need in 123 since https://chromium-review.googlesource.com/c/chromium/src/+/5354149 was reverted.

Original change's description:
> [M123] [Audio] Propagate socket errors to sinks and sources. Prevent reuse.
>
> M123: This drops AudioInputDevice and AudioOutputDevice tests which had
> merge conflicts.
>
> If the browser side closes the socket the renderer is only notified
> through a failure to read from the socket. This error was being dropped
> instead of propagated to the RenderCallback and CaptureCallback.
>
> This error signal is then used to prevent reuse of AudioRendererMixer
> instances. This fixes the linked issue (same-origin navigations where
> the mixer is reused between navigations).
>
> There may still be constructions where a RenderFrame which creates a
> shared sink is closed while the sink is being used. I haven't yet been
> able to construct such a scenario though.
>
> Bug: 326903566
>
> (cherry picked from commit 7c882251cf9d4f6fe2b212084674f1ab62897fea)
>
> Change-Id: I45c6ce12327315540590dab9ae545fa7327a9d83
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5331283
> Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
> Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
> Cr-Original-Commit-Position: refs/heads/main@{#1266838}
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5336610
> Auto-Submit: Dale Curtis <dalecurtis@chromium.org>
> Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
> Cr-Commit-Position: refs/branch-heads/6312@{#372}
> Cr-Branched-From: 6711dcdae48edaf98cbc6964f90fac85b7d9986e-refs/heads/main@{#1262506}

Bug: 326903566, 328908259
Change-Id: I259bcc5f907783a16b719cc667a7b16ca71053ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5360711
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/branch-heads/6312@{#515}
Cr-Branched-From: 6711dcdae48edaf98cbc6964f90fac85b7d9986e-refs/heads/main@{#1262506}
diff --git a/media/audio/audio_device_thread.cc b/media/audio/audio_device_thread.cc
index fac2d9b..d000377 100644
--- a/media/audio/audio_device_thread.cc
+++ b/media/audio/audio_device_thread.cc
@@ -63,7 +63,6 @@
 }
 
 AudioDeviceThread::~AudioDeviceThread() {
-  in_shutdown_.Set();
   socket_.Shutdown();
   if (thread_handle_.is_null())
     return;
@@ -113,10 +112,6 @@
     if (bytes_sent != sizeof(buffer_index))
       break;
   }
-
-  if (!in_shutdown_.IsSet()) {
-    callback_->OnSocketError();
-  }
 }
 
 }  // namespace media.
diff --git a/media/audio/audio_device_thread.h b/media/audio/audio_device_thread.h
index 0e94d537..c613f2e 100644
--- a/media/audio/audio_device_thread.h
+++ b/media/audio/audio_device_thread.h
@@ -9,7 +9,6 @@
 
 #include "base/memory/raw_ptr.h"
 #include "base/sync_socket.h"
-#include "base/synchronization/atomic_flag.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_checker.h"
 #include "build/buildflag.h"
@@ -47,9 +46,6 @@
     // Called whenever we receive notifications about pending input data.
     virtual void Process(uint32_t pending_data) = 0;
 
-    // Called if the socket closes outside of destruction.
-    virtual void OnSocketError() = 0;
-
     base::TimeDelta buffer_duration() const {
       return audio_parameters_.GetBufferDuration();
     }
@@ -92,9 +88,6 @@
 #endif
   void ThreadMain() final;
 
-  // Set to true in destruction, but before closing the socket.
-  base::AtomicFlag in_shutdown_;
-
   const raw_ptr<Callback> callback_;
   const char* thread_name_;
   base::CancelableSyncSocket socket_;
diff --git a/media/audio/audio_input_device.cc b/media/audio/audio_input_device.cc
index 6ebd6099b..9677510 100644
--- a/media/audio/audio_input_device.cc
+++ b/media/audio/audio_input_device.cc
@@ -82,8 +82,6 @@
   // Called whenever we receive notifications about pending data.
   void Process(uint32_t pending_data) override;
 
-  void OnSocketError() override;
-
  private:
   const bool enable_uma_;
   base::ReadOnlySharedMemoryRegion shared_memory_region_;
@@ -497,10 +495,4 @@
       "now_time (ms)", (now_time - base::TimeTicks()).InMillisecondsF());
 }
 
-void AudioInputDevice::AudioThreadCallback::OnSocketError() {
-  capture_callback_->OnCaptureError(
-      AudioCapturerSource::ErrorCode::kSocketError,
-      "Socket closed unexpectedly");
-}
-
 }  // namespace media
diff --git a/media/audio/audio_output_device_thread_callback.cc b/media/audio/audio_output_device_thread_callback.cc
index 4600038..98f54db 100644
--- a/media/audio/audio_output_device_thread_callback.cc
+++ b/media/audio/audio_output_device_thread_callback.cc
@@ -106,10 +106,6 @@
                    "delay (ms)", delay.InMillisecondsF());
 }
 
-void AudioOutputDeviceThreadCallback::OnSocketError() {
-  render_callback_->OnRenderError();
-}
-
 bool AudioOutputDeviceThreadCallback::CurrentThreadIsAudioDeviceThread() {
   return thread_checker_.CalledOnValidThread();
 }
diff --git a/media/audio/audio_output_device_thread_callback.h b/media/audio/audio_output_device_thread_callback.h
index 6359458..904c1fa 100644
--- a/media/audio/audio_output_device_thread_callback.h
+++ b/media/audio/audio_output_device_thread_callback.h
@@ -39,10 +39,6 @@
   // Called whenever we receive notifications about pending data.
   void Process(uint32_t control_signal) override;
 
-  // Called when the AudioDeviceThread shuts down. Unexpected calls are treated
-  // as errors.
-  void OnSocketError() override;
-
   // Returns whether the current thread is the audio device thread or not.
   // Will always return true if DCHECKs are not enabled.
   bool CurrentThreadIsAudioDeviceThread();
diff --git a/media/base/audio_capturer_source.h b/media/base/audio_capturer_source.h
index 0310661c..3745397 100644
--- a/media/base/audio_capturer_source.h
+++ b/media/base/audio_capturer_source.h
@@ -31,7 +31,6 @@
     kUnknown = 0,
     kSystemPermissions = 1,
     kDeviceInUse = 2,
-    kSocketError = 3,
   };
 
   class CaptureCallback {
diff --git a/media/base/audio_renderer_mixer.cc b/media/base/audio_renderer_mixer.cc
index 1ae9491..56c7cb3 100644
--- a/media/base/audio_renderer_mixer.cc
+++ b/media/base/audio_renderer_mixer.cc
@@ -115,11 +115,6 @@
   pause_delay_ = delay;
 }
 
-bool AudioRendererMixer::HasSinkError() {
-  base::AutoLock auto_lock(lock_);
-  return sink_error_;
-}
-
 int AudioRendererMixer::Render(base::TimeDelta delay,
                                base::TimeTicks delay_timestamp,
                                const AudioGlitchInfo& glitch_info,
@@ -152,7 +147,6 @@
 void AudioRendererMixer::OnRenderError() {
   // Call each mixer input and signal an error.
   base::AutoLock auto_lock(lock_);
-  sink_error_ = true;
   for (auto* input : error_callbacks_)
     input->OnRenderError();
 }
diff --git a/media/base/audio_renderer_mixer.h b/media/base/audio_renderer_mixer.h
index dedda46..1ff0356f 100644
--- a/media/base/audio_renderer_mixer.h
+++ b/media/base/audio_renderer_mixer.h
@@ -55,9 +55,6 @@
     return output_params_;
   }
 
-  // Return true if this mixer has ever received an error from its sink.
-  bool HasSinkError();
-
  private:
   // AudioRendererSink::RenderCallback implementation.
   int Render(base::TimeDelta delay,
@@ -100,10 +97,6 @@
   base::TimeDelta pause_delay_ GUARDED_BY(lock_);
   base::TimeTicks last_play_time_ GUARDED_BY(lock_);
   bool playing_ GUARDED_BY(lock_);
-
-  // Set if the mixer receives an error from the sink. Indicates that this
-  // mixer and sink should no longer be reused.
-  bool sink_error_ GUARDED_BY(lock_) = false;
 };
 
 }  // namespace media
diff --git a/media/base/audio_renderer_mixer_unittest.cc b/media/base/audio_renderer_mixer_unittest.cc
index 4219fb61..b59e2700 100644
--- a/media/base/audio_renderer_mixer_unittest.cc
+++ b/media/base/audio_renderer_mixer_unittest.cc
@@ -503,13 +503,9 @@
     EXPECT_CALL(*fake_callbacks_[i], OnRenderError()).Times(1);
   }
 
-  EXPECT_FALSE(mixer_->HasSinkError());
-
   mixer_callback_->OnRenderError();
   for (size_t i = 0; i < mixer_inputs_.size(); ++i)
     mixer_inputs_[i]->Stop();
-
-  EXPECT_TRUE(mixer_->HasSinkError());
 }
 
 TEST_P(AudioRendererMixerBehavioralTest, OnRenderErrorPausedInput) {
diff --git a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.cc b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.cc
index 0e0229c..c7a6bf2 100644
--- a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.cc
+++ b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.cc
@@ -146,7 +146,7 @@
   base::AutoLock auto_lock(mixers_lock_);
 
   auto it = mixers_.find(key);
-  if (it != mixers_.end() && !it->second.mixer->HasSinkError()) {
+  if (it != mixers_.end()) {
     auto new_count = ++it->second.ref_count;
     CHECK(new_count != std::numeric_limits<decltype(new_count)>::max());
 
@@ -161,12 +161,6 @@
     sink->Stop();
 
     return it->second.mixer;
-  } else if (it != mixers_.end() && it->second.mixer->HasSinkError()) {
-    DVLOG(1) << "Not reusing mixer with errors: " << it->second.mixer;
-
-    // Move bad mixers out of the reuse map.
-    dead_mixers_[key] = it->second;
-    mixers_.erase(it);
   }
 
   const media::AudioParameters& mixer_output_params =
@@ -211,31 +205,12 @@
       [](const std::pair<MixerKey, AudioRendererMixerReference>& val) {
         return val.second.mixer;
       });
-
-  // If a mixer isn't in the normal map, check the map for mixers w/ errors.
-  bool dead_mixer = false;
-  if (it == mixers_.end()) {
-    it = base::ranges::find(
-        dead_mixers_, mixer,
-        [](const std::pair<MixerKey, AudioRendererMixerReference>& val) {
-          return val.second.mixer;
-        });
-    DCHECK(it != dead_mixers_.end());
-    dead_mixer = true;
-  }
+  DCHECK(it != mixers_.end());
 
   // Only remove the mixer if AudioRendererMixerManager is the last owner.
   it->second.ref_count--;
   if (it->second.ref_count == 0) {
     delete it->second.mixer;
-    if (dead_mixer) {
-      dead_mixers_.erase(it);
-    } else {
-      mixers_.erase(it);
-    }
-  } else if (!dead_mixer && it->second.mixer->HasSinkError()) {
-    // Move bad mixers out of the reuse map.
-    dead_mixers_[it->first] = it->second;
     mixers_.erase(it);
   }
 }
diff --git a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.h b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.h
index cc3fc45..d7e5ebc 100644
--- a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.h
+++ b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager.h
@@ -180,11 +180,6 @@
 
   // Active mixers.
   AudioRendererMixerMap mixers_;
-
-  // Mixers which encountered errors, but can't yet be destroyed since they are
-  // still owned by an input.
-  AudioRendererMixerMap dead_mixers_;
-
   base::Lock mixers_lock_;
 };
 
diff --git a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager_test.cc b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager_test.cc
index 6a22f21..3c6a324 100644
--- a/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager_test.cc
+++ b/third_party/blink/renderer/modules/media/audio/audio_renderer_mixer_manager_test.cc
@@ -118,7 +118,6 @@
 
   // Number of instantiated mixers.
   size_t mixer_count() { return manager_->mixers_.size(); }
-  size_t dead_mixer_count() { return manager_->dead_mixers_.size(); }
 
  protected:
   scoped_refptr<media::MockAudioRendererSink> GetSink(
@@ -204,55 +203,6 @@
   EXPECT_EQ(0u, mixer_count());
 }
 
-TEST_F(AudioRendererMixerManagerTest, ReturnMixerWithError) {
-  mock_sink_ = CreateNormalSink();
-  auto* local_sink = mock_sink_.get();
-
-  // There should be no mixers outstanding to start with.
-  EXPECT_EQ(0u, mixer_count());
-
-  media::AudioParameters params1(
-      media::AudioParameters::AUDIO_PCM_LINEAR,
-      media::ChannelLayoutConfig::FromLayout<kChannelLayout>(), kSampleRate,
-      kBufferSize);
-
-  media::AudioRendererMixer* mixer1 =
-      GetMixer(kFrameToken, params1, AudioLatency::Type::kPlayback,
-               kDefaultDeviceId, SinkUseState::kNewSink);
-  ASSERT_TRUE(mixer1);
-  EXPECT_EQ(1u, mixer_count());
-
-  // The same parameters should return the same mixer1.
-  EXPECT_EQ(mixer1,
-            GetMixer(kFrameToken, params1, AudioLatency::Type::kPlayback,
-                     kDefaultDeviceId, SinkUseState::kExistingSink));
-  EXPECT_EQ(1u, mixer_count());
-
-  // Trigger an error in mixer1.
-  local_sink->callback()->OnRenderError();
-
-  // Return the extra mixer we just acquired, it should not be deleted, but put
-  // into the dead mixer map.
-  ReturnMixer(mixer1);
-  EXPECT_EQ(0u, mixer_count());
-  EXPECT_EQ(1u, dead_mixer_count());
-
-  // Using the same params should create a new mixer due to the error.
-  media::AudioRendererMixer* mixer2 =
-      GetMixer(kFrameToken, params1, AudioLatency::Type::kPlayback,
-               kDefaultDeviceId, SinkUseState::kNewSink);
-  ASSERT_TRUE(mixer2);
-  EXPECT_EQ(1u, mixer_count());
-  EXPECT_EQ(1u, dead_mixer_count());
-  EXPECT_NE(mixer1, mixer2);
-
-  // Return both outstanding mixers.
-  ReturnMixer(mixer1);
-  EXPECT_EQ(0u, dead_mixer_count());
-  ReturnMixer(mixer2);
-  EXPECT_EQ(0u, mixer_count());
-}
-
 // Verify GetMixer() correctly deduplicates mixer with irrelevant AudioParameter
 // differences.
 TEST_F(AudioRendererMixerManagerTest, MixerReuse) {