From 6e25065a76fb379b75d4af8482c7c67d29eb73b0 Mon Sep 17 00:00:00 2001 From: Henrik Grunell Date: Thu, 23 Jun 2016 18:24:14 +0200 Subject: [PATCH] Implementing AudioOutputDevice authorization timeout by scheduling a delayed cancelable AudioOutputDevice::OnDeviceAuthorized() call. BUG=615589 Review-Url: https://codereview.chromium.org/2043883005 Cr-Commit-Position: refs/heads/master@{#399685} (cherry picked from commit 698458b0a4cd539c3945d1cb988d9a4a9a96d64d) Review URL: https://codereview.chromium.org/2090863003 . Cr-Commit-Position: refs/branch-heads/2743@{#456} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} --- .../renderer/media/audio_device_factory.cc | 10 +++- media/audio/audio_output_device.cc | 31 ++++++++++- media/audio/audio_output_device.h | 17 +++++- media/audio/audio_output_device_unittest.cc | 54 +++++++++++++++++-- media/base/output_device_info.h | 1 + tools/metrics/histograms/histograms.xml | 9 ++++ 6 files changed, 114 insertions(+), 8 deletions(-) diff --git a/content/renderer/media/audio_device_factory.cc b/content/renderer/media/audio_device_factory.cc index 40d8455f5c402..085bf0d13c94e 100644 --- a/content/renderer/media/audio_device_factory.cc +++ b/content/renderer/media/audio_device_factory.cc @@ -4,7 +4,10 @@ #include "content/renderer/media/audio_device_factory.h" +#include + #include "base/logging.h" +#include "content/common/content_constants_internal.h" #include "content/renderer/media/audio_input_message_filter.h" #include "content/renderer/media/audio_message_filter.h" #include "content/renderer/media/audio_renderer_mixer_manager.h" @@ -20,6 +23,7 @@ namespace content { AudioDeviceFactory* AudioDeviceFactory::factory_ = NULL; namespace { +const int64_t kMaxAuthorizationTimeoutMs = 4000; scoped_refptr NewOutputDevice( int render_frame_id, @@ -29,7 +33,11 @@ scoped_refptr NewOutputDevice( AudioMessageFilter* const filter = AudioMessageFilter::Get(); scoped_refptr device(new media::AudioOutputDevice( filter->CreateAudioOutputIPC(render_frame_id), filter->io_task_runner(), - session_id, device_id, security_origin)); + session_id, device_id, security_origin, + // Set authorization request timeout at 80% of renderer hung timeout, but + // no more than kMaxAuthorizationTimeout. + base::TimeDelta::FromMilliseconds(std::min(kHungRendererDelayMs * 8 / 10, + kMaxAuthorizationTimeoutMs)))); device->RequestDeviceAuthorization(); return device; } diff --git a/media/audio/audio_output_device.cc b/media/audio/audio_output_device.cc index 05b123f780a05..553cc08176d94 100644 --- a/media/audio/audio_output_device.cc +++ b/media/audio/audio_output_device.cc @@ -12,8 +12,10 @@ #include "base/callback_helpers.h" #include "base/macros.h" +#include "base/metrics/histogram_macros.h" #include "base/threading/thread_restrictions.h" #include "base/time/time.h" +#include "base/timer/timer.h" #include "base/trace_event/trace_event.h" #include "build/build_config.h" #include "media/audio/audio_device_description.h" @@ -52,7 +54,8 @@ AudioOutputDevice::AudioOutputDevice( const scoped_refptr& io_task_runner, int session_id, const std::string& device_id, - const url::Origin& security_origin) + const url::Origin& security_origin, + base::TimeDelta authorization_timeout) : ScopedTaskRunnerObserver(io_task_runner), callback_(NULL), ipc_(std::move(ipc)), @@ -64,7 +67,8 @@ AudioOutputDevice::AudioOutputDevice( security_origin_(security_origin), stopping_hack_(false), did_receive_auth_(true, false), - device_status_(OUTPUT_DEVICE_STATUS_ERROR_INTERNAL) { + device_status_(OUTPUT_DEVICE_STATUS_ERROR_INTERNAL), + auth_timeout_(authorization_timeout) { CHECK(ipc_); // The correctness of the code depends on the relative values assigned in the @@ -155,6 +159,16 @@ void AudioOutputDevice::RequestDeviceAuthorizationOnIOThread() { state_ = AUTHORIZING; ipc_->RequestDeviceAuthorization(this, session_id_, device_id_, security_origin_); + + // Create the timer on the thread it's used on. It's guaranteed to be + // deleted on the same thread since users must call Stop() before deleting + // AudioOutputDevice; see ShutDownOnIOThread(). + auth_timeout_action_.reset(new base::OneShotTimer()); + auth_timeout_action_->Start( + FROM_HERE, auth_timeout_, + base::Bind(&AudioOutputDevice::OnDeviceAuthorized, this, + OUTPUT_DEVICE_STATUS_ERROR_TIMED_OUT, media::AudioParameters(), + std::string())); } void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) { @@ -228,6 +242,9 @@ void AudioOutputDevice::ShutDownOnIOThread() { } start_on_authorized_ = false; + // Destoy the timer on the thread it's used on. + auth_timeout_action_.reset(); + // We can run into an issue where ShutDownOnIOThread is called right after // OnStreamCreated is called in cases where Start/Stop are called before we // get the OnStreamCreated callback. To handle that corner case, we call @@ -284,6 +301,16 @@ void AudioOutputDevice::OnDeviceAuthorized( const media::AudioParameters& output_params, const std::string& matched_device_id) { DCHECK(task_runner()->BelongsToCurrentThread()); + + auth_timeout_action_.reset(); + + // Do nothing if late authorization is received after timeout. + if (state_ == IPC_CLOSED) + return; + + UMA_HISTOGRAM_BOOLEAN("Media.Audio.Render.OutputDeviceAuthorizationTimedOut", + device_status == OUTPUT_DEVICE_STATUS_ERROR_TIMED_OUT); + DCHECK_EQ(state_, AUTHORIZING); // It may happen that a second authorization is received as a result to a diff --git a/media/audio/audio_output_device.h b/media/audio/audio_output_device.h index 532f45fe4a13e..c0faacd11517f 100644 --- a/media/audio/audio_output_device.h +++ b/media/audio/audio_output_device.h @@ -77,6 +77,10 @@ #include "media/base/media_export.h" #include "media/base/output_device_info.h" +namespace base { +class OneShotTimer; +} + namespace media { class MEDIA_EXPORT AudioOutputDevice @@ -90,7 +94,8 @@ class MEDIA_EXPORT AudioOutputDevice const scoped_refptr& io_task_runner, int session_id, const std::string& device_id, - const url::Origin& security_origin); + const url::Origin& security_origin, + base::TimeDelta authorization_timeout); // Request authorization to use the device specified in the constructor. void RequestDeviceAuthorization(); @@ -145,6 +150,13 @@ class MEDIA_EXPORT AudioOutputDevice void ShutDownOnIOThread(); void SetVolumeOnIOThread(double volume); + // Process device authorization result on the IO thread. + void ProcessDeviceAuthorizationOnIOThread( + OutputDeviceStatus device_status, + const media::AudioParameters& output_params, + const std::string& matched_device_id, + bool timed_out); + // base::MessageLoop::DestructionObserver implementation for the IO loop. // If the IO loop dies before we do, we shut down the audio thread from here. void WillDestroyCurrentMessageLoop() override; @@ -201,6 +213,9 @@ class MEDIA_EXPORT AudioOutputDevice AudioParameters output_params_; OutputDeviceStatus device_status_; + const base::TimeDelta auth_timeout_; + std::unique_ptr auth_timeout_action_; + DISALLOW_COPY_AND_ASSIGN(AudioOutputDevice); }; diff --git a/media/audio/audio_output_device_unittest.cc b/media/audio/audio_output_device_unittest.cc index af2b141cc8f53..d066b6f46b30a 100644 --- a/media/audio/audio_output_device_unittest.cc +++ b/media/audio/audio_output_device_unittest.cc @@ -17,9 +17,11 @@ #include "base/sync_socket.h" #include "base/task_runner.h" #include "base/test/test_timeouts.h" +#include "base/threading/thread.h" #include "base/threading/thread_task_runner_handle.h" #include "media/audio/audio_output_device.h" #include "media/audio/sample_rates.h" +#include "media/base/test_helpers.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gmock_mutant.h" #include "testing/gtest/include/gtest/gtest.h" @@ -42,6 +44,7 @@ namespace { const char kDefaultDeviceId[] = ""; const char kNonDefaultDeviceId[] = "valid-nondefault-device-id"; const char kUnauthorizedDeviceId[] = "unauthorized-device-id"; +const int kAuthTimeoutForTestingMs = 500; class MockRenderCallback : public AudioRendererSink::RenderCallback { public: @@ -99,7 +102,9 @@ class AudioOutputDeviceTest void ExpectRenderCallback(); void WaitUntilRenderCallback(); void StopAudioDevice(); + void CreateDevice(const std::string& device_id); void SetDevice(const std::string& device_id); + void CheckDeviceStatus(OutputDeviceStatus device_status); protected: // Used to clean up TLS pointers that the test(s) will initialize. @@ -139,11 +144,16 @@ AudioOutputDeviceTest::~AudioOutputDeviceTest() { audio_device_ = NULL; } -void AudioOutputDeviceTest::SetDevice(const std::string& device_id) { +void AudioOutputDeviceTest::CreateDevice(const std::string& device_id) { audio_output_ipc_ = new MockAudioOutputIPC(); - audio_device_ = new AudioOutputDevice(base::WrapUnique(audio_output_ipc_), - io_loop_.task_runner(), 0, device_id, - url::Origin()); + audio_device_ = new AudioOutputDevice( + base::WrapUnique(audio_output_ipc_), io_loop_.task_runner(), 0, device_id, + url::Origin(), + base::TimeDelta::FromMilliseconds(kAuthTimeoutForTestingMs)); +} + +void AudioOutputDeviceTest::SetDevice(const std::string& device_id) { + CreateDevice(device_id); EXPECT_CALL(*audio_output_ipc_, RequestDeviceAuthorization(audio_device_.get(), 0, device_id, _)); audio_device_->RequestDeviceAuthorization(); @@ -160,6 +170,11 @@ void AudioOutputDeviceTest::SetDevice(const std::string& device_id) { &callback_); } +void AudioOutputDeviceTest::CheckDeviceStatus(OutputDeviceStatus status) { + DCHECK(!io_loop_.task_runner()->BelongsToCurrentThread()); + EXPECT_EQ(status, audio_device_->GetOutputDeviceInfo().device_status()); +} + void AudioOutputDeviceTest::ReceiveAuthorization(OutputDeviceStatus status) { device_status_ = status; if (device_status_ != OUTPUT_DEVICE_STATUS_OK) @@ -318,6 +333,37 @@ TEST_P(AudioOutputDeviceTest, UnauthorizedDevice) { StopAudioDevice(); } +TEST_P(AudioOutputDeviceTest, AuthorizationTimedOut) { + base::Thread thread("DeviceInfo"); + thread.Start(); + + CreateDevice(kNonDefaultDeviceId); + EXPECT_CALL(*audio_output_ipc_, + RequestDeviceAuthorization(audio_device_.get(), 0, + kNonDefaultDeviceId, _)); + EXPECT_CALL(*audio_output_ipc_, CloseStream()); + + // Request authorization; no reply from the browser. + audio_device_->RequestDeviceAuthorization(); + + media::WaitableMessageLoopEvent event; + + // Request device info on another thread. + thread.task_runner()->PostTaskAndReply( + FROM_HERE, + base::Bind(&AudioOutputDeviceTest::CheckDeviceStatus, + base::Unretained(this), OUTPUT_DEVICE_STATUS_ERROR_TIMED_OUT), + event.GetClosure()); + + io_loop_.RunUntilIdle(); + + // Runs the loop and waits for |thread| to call event's closure. + event.RunAndWait(); + + audio_device_->Stop(); + io_loop_.RunUntilIdle(); +} + INSTANTIATE_TEST_CASE_P(Render, AudioOutputDeviceTest, Values(false)); } // namespace media. diff --git a/media/base/output_device_info.h b/media/base/output_device_info.h index 9014672e74ab0..9f90fed74f213 100644 --- a/media/base/output_device_info.h +++ b/media/base/output_device_info.h @@ -18,6 +18,7 @@ enum OutputDeviceStatus { OUTPUT_DEVICE_STATUS_OK = 0, OUTPUT_DEVICE_STATUS_ERROR_NOT_FOUND, OUTPUT_DEVICE_STATUS_ERROR_NOT_AUTHORIZED, + OUTPUT_DEVICE_STATUS_ERROR_TIMED_OUT, OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, OUTPUT_DEVICE_STATUS_LAST = OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, }; diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 8f04ef8678720..f0b9bfd493266 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -20513,6 +20513,15 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries. + + olka@chromium.org + + Whether audio output device timed out waiting for authorization reply from + the browser side. + + + henrika@chromium.org