Dropping sinks from AudioRendererSinkCache on frame navigation.

Frame observer is registered right away on each frame initialization.

Why:
Sink cache lives on the main task runner, but also accepts sink requests from
the media task runner (media pipeline). So the sinks can be cashed on
2 different threads.
However, frame observers can be registered on the main renderer thread only.
Because of the above, on-demand observer registration (i.e. registering an
observer only when a sink is cached for a frame) is racy: navigation can
happen before the observer is registered.

BUG=836420

Change-Id: I9377201d93ae5c12e4fe05c8f23514b68bfddbe1
Reviewed-on: https://chromium-review.googlesource.com/1050127
Commit-Queue: Olga Sharonova <olka@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557342}

content/renderer/media/audio_renderer_sink_cache.h

@@ -17,11 +17,12 @@ class AudioRendererSink;
 }
 
 namespace content {
+class RenderFrame;
 
 // Caches AudioRendererSink instances, provides them to the clients for usage,
 // tracks their used/unused state, reuses them to obtain output device
 // information, garbage-collects unused sinks.
-// Thread safe.
+// Must live on the main render thread. Thread safe.
 class CONTENT_EXPORT AudioRendererSinkCache {
  public:
   virtual ~AudioRendererSinkCache() {}
@@ -29,6 +30,10 @@ class CONTENT_EXPORT AudioRendererSinkCache {
   // Creates default cache, to be used by AudioRendererMixerManager.
   static std::unique_ptr<AudioRendererSinkCache> Create();
 
+  // If called, the cache will drop sinks belonging to the specified frame on
+  // navigation.
+  static void ObserveFrame(RenderFrame* frame);
+
   // Returns output device information for a specified sink.
   virtual media::OutputDeviceInfo GetSinkInfo(int source_render_frame_id,
                                               int session_id,
@@ -49,6 +54,7 @@ class CONTENT_EXPORT AudioRendererSinkCache {
  protected:
   AudioRendererSinkCache() {}
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(AudioRendererSinkCache);
 };
 

content/renderer/media/audio_renderer_sink_cache_impl.cc

@@ -14,14 +14,44 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "content/public/renderer/render_frame.h"
+#include "content/public/renderer/render_frame_observer.h"
 #include "content/renderer/media/audio_device_factory.h"
 #include "media/audio/audio_device_description.h"
 #include "media/base/audio_renderer_sink.h"
 
 namespace content {
 
+AudioRendererSinkCacheImpl* AudioRendererSinkCacheImpl::instance_ = nullptr;
 constexpr int kDeleteTimeoutMs = 5000;
 
+class AudioRendererSinkCacheImpl::FrameObserver : public RenderFrameObserver {
+ public:
+  explicit FrameObserver(content::RenderFrame* render_frame)
+      : RenderFrameObserver(render_frame) {}
+  ~FrameObserver() override{};
+
+ private:
+  // content::RenderFrameObserver implementation:
+  void DidCommitProvisionalLoad(bool is_new_navigation,
+                                bool is_same_document_navigation) override {
+    if (!is_same_document_navigation)
+      DropFrameCache();
+  }
+
+  void OnDestruct() override {
+    DropFrameCache();
+    delete this;
+  }
+
+  void DropFrameCache() {
+    if (AudioRendererSinkCacheImpl::instance_)
+      AudioRendererSinkCacheImpl::instance_->DropSinksForFrame(routing_id());
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(FrameObserver);
+};
+
 namespace {
 
 enum GetOutputDeviceInfoCacheUtilization {
@@ -56,21 +86,30 @@ struct AudioRendererSinkCacheImpl::CacheEntry {
 
 // static
 std::unique_ptr<AudioRendererSinkCache> AudioRendererSinkCache::Create() {
-  return base::WrapUnique(new AudioRendererSinkCacheImpl(
+  return std::make_unique<AudioRendererSinkCacheImpl>(
       base::ThreadTaskRunnerHandle::Get(),
       base::Bind(&AudioDeviceFactory::NewAudioRendererMixerSink),
-      base::TimeDelta::FromMilliseconds(kDeleteTimeoutMs)));
+      base::TimeDelta::FromMilliseconds(kDeleteTimeoutMs));
+}
+
+// static
+void AudioRendererSinkCache::ObserveFrame(RenderFrame* frame) {
+  new AudioRendererSinkCacheImpl::FrameObserver(frame);
 }
 
 AudioRendererSinkCacheImpl::AudioRendererSinkCacheImpl(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    const CreateSinkCallback& create_sink_cb,
+    CreateSinkCallback create_sink_cb,
     base::TimeDelta delete_timeout)
     : task_runner_(std::move(task_runner)),
-      create_sink_cb_(create_sink_cb),
+      create_sink_cb_(std::move(create_sink_cb)),
       delete_timeout_(delete_timeout),
       weak_ptr_factory_(this) {
   weak_this_ = weak_ptr_factory_.GetWeakPtr();
+  if (instance_)
+    LOG(ERROR) << "More that one AudioRendererSinkCache instance created. "
+                  "Allowed in tests only.";
+  instance_ = this;
 }
 
 AudioRendererSinkCacheImpl::~AudioRendererSinkCacheImpl() {
@@ -80,6 +119,9 @@ AudioRendererSinkCacheImpl::~AudioRendererSinkCacheImpl() {
   // is being destroyed anyways.
   for (auto& entry : cache_)
     entry.sink->Stop();
+
+  if (instance_ == this)
+    instance_ = nullptr;
 }
 
 media::OutputDeviceInfo AudioRendererSinkCacheImpl::GetSinkInfo(
@@ -266,6 +308,20 @@ void AudioRendererSinkCacheImpl::CacheOrStopUnusedSink(
   DeleteLaterIfUnused(cache_entry.sink.get());
 }
 
+void AudioRendererSinkCacheImpl::DropSinksForFrame(int source_render_frame_id) {
+  base::AutoLock auto_lock(cache_lock_);
+  cache_.erase(std::remove_if(cache_.begin(), cache_.end(),
+                              [source_render_frame_id](const CacheEntry& val) {
+                                if (val.source_render_frame_id ==
+                                    source_render_frame_id) {
+                                  val.sink->Stop();
+                                  return true;
+                                }
+                                return false;
+                              }),
+               cache_.end());
+}
+
 int AudioRendererSinkCacheImpl::GetCacheSizeForTesting() {
   return cache_.size();
 }

content/renderer/media/audio_renderer_sink_cache_impl.h

@@ -23,6 +23,8 @@ namespace content {
 class CONTENT_EXPORT AudioRendererSinkCacheImpl
     : public AudioRendererSinkCache {
  public:
+  class FrameObserver;
+
   // Callback to be used for AudioRendererSink creation
   using CreateSinkCallback =
       base::RepeatingCallback<scoped_refptr<media::AudioRendererSink>(
@@ -32,7 +34,7 @@ class CONTENT_EXPORT AudioRendererSinkCacheImpl
 
   AudioRendererSinkCacheImpl(
       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-      const CreateSinkCallback& create_sink_callback,
+      CreateSinkCallback create_sink_callback,
       base::TimeDelta delete_timeout);
 
   ~AudioRendererSinkCacheImpl() final;
@@ -50,6 +52,7 @@ class CONTENT_EXPORT AudioRendererSinkCacheImpl
  private:
   friend class AudioRendererSinkCacheTest;
   friend class CacheEntryFinder;
+  friend class AudioRendererSinkCacheImpl::FrameObserver;
 
   struct CacheEntry;
   using CacheContainer = std::vector<CacheEntry>;
@@ -73,10 +76,15 @@ class CONTENT_EXPORT AudioRendererSinkCacheImpl
                              const std::string& device_id,
                              scoped_refptr<media::AudioRendererSink> sink);
 
+  void DropSinksForFrame(int source_render_frame_id);
+
   // To avoid publishing CacheEntry structure in the header.
   int GetCacheSizeForTesting();
 
-  // Task runner for scheduled sink garbage collection.
+  // Global instance, set in constructor and unset in destructor.
+  static AudioRendererSinkCacheImpl* instance_;
+
+  // Renderer main task runner.
   const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   // Callback used for sink creation.

content/renderer/media/audio_renderer_sink_cache_unittest.cc

@@ -84,6 +84,8 @@ class AudioRendererSinkCacheTest : public testing::Test {
     e.Wait();
   }
 
+  void DropSinksForFrame(int frame_id) { cache_->DropSinksForFrame(frame_id); }
+
   base::test::ScopedTaskEnvironment task_env_;
   std::unique_ptr<AudioRendererSinkCacheImpl> cache_;
 
@@ -369,4 +371,36 @@ TEST_F(AudioRendererSinkCacheTest, MultithreadedAccess) {
   EXPECT_EQ(0, sink_count());
 }
 
+TEST_F(AudioRendererSinkCacheTest, StopsAndDropsSinks) {
+  EXPECT_EQ(0, sink_count());
+  scoped_refptr<media::AudioRendererSink> sink1 =
+      cache_->GetSink(kRenderFrameId, "device1").get();
+  scoped_refptr<media::AudioRendererSink> sink2 =
+      cache_->GetSink(kRenderFrameId, "device2").get();
+  EXPECT_EQ(2, sink_count());
+
+  EXPECT_CALL(*static_cast<media::MockAudioRendererSink*>(sink1.get()), Stop());
+  EXPECT_CALL(*static_cast<media::MockAudioRendererSink*>(sink2.get()), Stop());
+  DropSinksForFrame(kRenderFrameId);
+  EXPECT_EQ(0, sink_count());
+}
+
+TEST_F(AudioRendererSinkCacheTest, StopsAndDropsCorrectSinks) {
+  EXPECT_EQ(0, sink_count());
+  scoped_refptr<media::AudioRendererSink> sink1 =
+      cache_->GetSink(kRenderFrameId, "device1").get();
+  scoped_refptr<media::AudioRendererSink> another_sink =
+      cache_->GetSink(kRenderFrameId + 1, "device1").get();
+  scoped_refptr<media::AudioRendererSink> sink2 =
+      cache_->GetSink(kRenderFrameId, "device2").get();
+  EXPECT_EQ(3, sink_count());
+
+  EXPECT_CALL(*static_cast<media::MockAudioRendererSink*>(sink1.get()), Stop());
+  EXPECT_CALL(*static_cast<media::MockAudioRendererSink*>(sink2.get()), Stop());
+  DropSinksForFrame(kRenderFrameId);
+  EXPECT_EQ(1, sink_count());
+  EXPECT_CALL(*static_cast<media::MockAudioRendererSink*>(another_sink.get()),
+              Stop());
+}
+
 }  // namespace content

content/renderer/render_frame_impl.cc

@@ -120,6 +120,7 @@
 #include "content/renderer/manifest/manifest_manager.h"
 #include "content/renderer/media/audio_device_factory.h"
 #include "content/renderer/media/audio_output_ipc_factory.h"
+#include "content/renderer/media/audio_renderer_sink_cache.h"
 #include "content/renderer/media/media_permission_dispatcher.h"
 #include "content/renderer/media/stream/media_stream_device_observer.h"
 #include "content/renderer/media/stream/user_media_client_impl.h"
@@ -1440,6 +1441,8 @@ void RenderFrameImpl::Initialize() {
   if (auto* factory = AudioOutputIPCFactory::get())
     factory->RegisterRemoteFactory(GetRoutingID(), GetRemoteInterfaces());
 
+  AudioRendererSinkCache::ObserveFrame(this);
+
   const base::CommandLine& command_line =
       *base::CommandLine::ForCurrentProcess();
   if (command_line.HasSwitch(switches::kDomAutomationController))