Propagate all errors from AudioOutputStreamBroker.

We cannot retry anything yet, so make sure the renderer knows we're
gone.

Drive-by remove handling of navigations, since it was crashing.

Bug: 787806, 840345
Change-Id: Icbaf49dda9ca12094ada2e9decc57622cd1feff6
Reviewed-on: https://chromium-review.googlesource.com/1047209
Commit-Queue: Max Morin <maxmorin@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557491}

content/browser/media/audio_output_stream_broker.cc

@@ -102,12 +102,11 @@ void AudioOutputStreamBroker::ObserverBindingLost(
     uint32_t reason,
     const std::string& description) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
-  if (reason ==
-      media::mojom::AudioOutputStreamObserver::kPlatformErrorDisconnectReason) {
-    client_.ResetWithReason(media::mojom::AudioOutputStreamProviderClient::
-                                kPlatformErrorDisconnectReason,
-                            std::string());
-  }
+  // TODO(https://crbug.com/787806): Don't propagate errors if we can retry
+  // instead.
+  client_.ResetWithReason(media::mojom::AudioOutputStreamProviderClient::
+                              kPlatformErrorDisconnectReason,
+                          std::string());
 
   Cleanup();
 }

content/browser/media/audio_output_stream_broker_unittest.cc

@@ -258,7 +258,7 @@ TEST(AudioOutputStreamBrokerTest,
 }
 
 TEST(AudioOutputStreamBrokerTest,
-     ObserverDisconnect_DoesNotPropagateErrorButCallsDeleter) {
+     ObserverDisconnect_PropagatesErrorAndCallsDeleter) {
   TestEnvironment env;
   MockStreamFactory::StreamRequestData stream_request_data(
       kDeviceId, TestParams(), env.group);
@@ -269,7 +269,9 @@ TEST(AudioOutputStreamBrokerTest,
 
   EXPECT_TRUE(stream_request_data.requested);
   EXPECT_CALL(env.provider_client,
-              ConnectionError(/*general disconnect reason*/ 0, std::string()));
+              ConnectionError(media::mojom::AudioOutputStreamProviderClient::
+                                  kPlatformErrorDisconnectReason,
+                              std::string()));
   EXPECT_CALL(env.deleter, Run(env.broker.release()))
       .WillOnce(testing::DeleteArg<0>());
 
@@ -282,7 +284,7 @@ TEST(AudioOutputStreamBrokerTest,
 }
 
 TEST(AudioOutputStreamBrokerTest,
-     FactoryDisconnectDuringConstruction_CallsDeleter) {
+     FactoryDisconnectDuringConstruction_PropagatesErrorAndCallsDeleter) {
   TestEnvironment env;
 
   env.broker->CreateStream(env.factory_ptr.get());
@@ -291,7 +293,9 @@ TEST(AudioOutputStreamBrokerTest,
   EXPECT_CALL(env.deleter, Run(env.broker.release()))
       .WillOnce(testing::DeleteArg<0>());
   EXPECT_CALL(env.provider_client,
-              ConnectionError(/*general disconnect reason*/ 0, std::string()));
+              ConnectionError(media::mojom::AudioOutputStreamProviderClient::
+                                  kPlatformErrorDisconnectReason,
+                              std::string()));
 
   env.RunUntilIdle();
 }

content/browser/media/forwarding_audio_stream_factory.cc

@@ -38,6 +38,8 @@ ForwardingAudioStreamFactory::~ForwardingAudioStreamFactory() {
 // static
 ForwardingAudioStreamFactory* ForwardingAudioStreamFactory::ForFrame(
     RenderFrameHost* frame) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
   auto* contents =
       static_cast<WebContentsImpl*>(WebContents::FromRenderFrameHost(frame));
   if (!contents)
@@ -94,19 +96,10 @@ void ForwardingAudioStreamFactory::FrameDeleted(
   CleanupStreamsBelongingTo(render_frame_host);
 }
 
-void ForwardingAudioStreamFactory::DidFinishNavigation(
-    NavigationHandle* navigation_handle) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (!navigation_handle->IsSameDocument()) {
-    // Document of frame will be destroyed, don't keep any streams around.
-    CleanupStreamsBelongingTo(navigation_handle->GetRenderFrameHost());
-  }
-}
-
 void ForwardingAudioStreamFactory::WebContentsDestroyed() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(inputs_.empty());
   DCHECK(outputs_.empty());
+  DCHECK(inputs_.empty());
 }
 
 void ForwardingAudioStreamFactory::CleanupStreamsBelongingTo(
@@ -121,8 +114,8 @@ void ForwardingAudioStreamFactory::CleanupStreamsBelongingTo(
            broker->render_frame_id() == frame_id;
   };
 
-  base::EraseIf(inputs_, match_rfh);
   base::EraseIf(outputs_, match_rfh);
+  base::EraseIf(inputs_, match_rfh);
 
   ResetRemoteFactoryPtrIfIdle();
 }
@@ -166,8 +159,8 @@ void ForwardingAudioStreamFactory::ResetRemoteFactoryPtrIfIdle() {
 void ForwardingAudioStreamFactory::ResetRemoteFactoryPtr() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   remote_factory_.reset();
-  inputs_.clear();
-  outputs_.clear();
+  // The stream brokers will call a callback to be deleted soon, give them a
+  // chance to signal an error to the client first.
 }
 
 }  // namespace content

content/browser/media/forwarding_audio_stream_factory.h

@@ -68,9 +68,8 @@ class CONTENT_EXPORT ForwardingAudioStreamFactory final
       media::mojom::AudioOutputStreamProviderClientPtr client);
 
   // WebContentsObserver implementation. We observe these events so that we can
-  // clean up streams belonging to a document when that document is destroyed.
+  // clean up streams belonging to a frame when that frame is destroyed.
   void FrameDeleted(RenderFrameHost* render_frame_host) final;
-  void DidFinishNavigation(NavigationHandle* navigation_handle) final;
   void WebContentsDestroyed() final;
 
  private:

content/browser/media/forwarding_audio_stream_factory_unittest.cc

@@ -271,7 +271,7 @@ TEST_F(ForwardingAudioStreamFactoryTest,
     testing::Mock::VerifyAndClear(&*other_rfh_broker);
   }
 
-  factory.FrameDeleted(other_rfh());
+  std::move(other_rfh_broker->deleter).Run(&*other_rfh_broker);
   EXPECT_FALSE(other_rfh_broker)
       << "Output broker should be destructed when deleter is called.";
   EXPECT_TRUE(main_rfh_broker);
@@ -367,39 +367,6 @@ TEST_F(ForwardingAudioStreamFactoryTest, DestroyWebContents_DestroysStreams) {
          "WebContents is destructed.";
 }
 
-TEST_F(ForwardingAudioStreamFactoryTest, DestroyRemoteFactory_CleansUpStreams) {
-  mojom::RendererAudioInputStreamFactoryClientPtr input_client;
-  base::WeakPtr<MockBroker> input_broker =
-      ExpectInputBrokerConstruction(main_rfh());
-  media::mojom::AudioOutputStreamProviderClientPtr output_client;
-  base::WeakPtr<MockBroker> output_broker =
-      ExpectOutputBrokerConstruction(main_rfh());
-
-  ForwardingAudioStreamFactory factory(web_contents(), std::move(connector_),
-                                       std::move(broker_factory_));
-
-  EXPECT_CALL(*input_broker, CreateStream(NotNull()));
-  mojo::MakeRequest(&input_client);
-  factory.CreateInputStream(main_rfh(), kInputDeviceId, kParams,
-                            kSharedMemoryCount, kEnableAgc,
-                            std::move(input_client));
-
-  EXPECT_CALL(*output_broker, CreateStream(NotNull()));
-  mojo::MakeRequest(&output_client);
-  factory.CreateOutputStream(main_rfh(), kOutputDeviceId, kParams,
-                             std::move(output_client));
-
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(input_broker);
-  EXPECT_TRUE(output_broker);
-  pending_factory_request_.reset();  // Triggers connection error.
-  base::RunLoop().RunUntilIdle();
-  EXPECT_FALSE(input_broker) << "Input broker should be destructed when owning "
-                                "WebContents is destructed.";
-  EXPECT_FALSE(output_broker) << "Output broker should be destructed when "
-                                 "owning WebContents is destructed.";
-}
-
 TEST_F(ForwardingAudioStreamFactoryTest, LastStreamDeleted_ClearsFactoryPtr) {
   mojom::RendererAudioInputStreamFactoryClientPtr input_client;
   base::WeakPtr<MockBroker> main_rfh_input_broker =