Implementing AudioOutputDevice authorization timeout

media/audio/audio_output_device.cc

Index: media/audio/audio_output_device.cc
diff --git a/media/audio/audio_output_device.cc b/media/audio/audio_output_device.cc
index a3effebfb43af118500197a6eed9a48093563f7d..acda8d4b747c0fbe2632c7f28a8ff755bc2a23e2 100644
--- a/media/audio/audio_output_device.cc
+++ b/media/audio/audio_output_device.cc
@@ -20,6 +20,8 @@
 #include "media/audio/audio_output_controller.h"
 #include "media/base/limits.h"
 
+constexpr int kAuthorizationTimeoutMs = 1000;

[o1ka, 2016/06/08 15:46:06]

Not sure about the value

[o1ka, 2016/06/09 12:02:04]

Looks like renderer hang timeout is defined by content::kHungRendererDelayMs
(https://cs-staging.chromium.org/chromium/src/content/common/content_constants...),

which is (for now) defined as 5000 for android / 30000 otherwise
(https://cs-staging.chromium.org/chromium/src/content/common/content_constants...).

We can probably use something like content::kHungRendererDelayMs/2 as
authorization timeout, or stick with min(3000, kHungRendererDelayMs). Not sure
which strategy is more reasonable: is it normal to wait for 15 seconds?

[DaleCurtis, 2016/06/09 18:25:34]

I don't think we want to block for anymore than 5 seconds, but we should
definitely have a UMA to figure out how often this is ticking as well as one
which includes when the response actually comes in if possible.

[o1ka, 2016/06/10 14:21:08]

Ok. I'm not sure if we should wait for 5 seconds on Android - I think we should
be a little below the threshold.

+
 namespace media {
 
 // Takes care of invoking the render callback on the audio thread.
@@ -154,8 +156,23 @@ void AudioOutputDevice::RequestDeviceAuthorizationOnIOThread() {
   DCHECK(task_runner()->BelongsToCurrentThread());
   DCHECK_EQ(state_, IDLE);
   state_ = AUTHORIZING;
+
+  if (auth_timeout_action_)

[DaleCurtis, 2016/06/09 18:25:34]

Instead, if you use a base::OneShotTimer here you can replace all of this with 

auth_timeout_action_.Start(FROM_HERE, delay, base::Bind(...));

on l.173. It'll take care of posting the task, doing nothing when the timer
isn't running, and canceling it if it is running (as well as cleaning up the
timer on destruction).

[o1ka, 2016/06/10 14:21:08]

Thanks! Done.

+    auth_timeout_action_->Cancel();
+
+  // Create the closure 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.
+  auth_timeout_action_.reset(new base::CancelableClosure(
+      base::Bind(&AudioOutputDevice::ProcessDeviceAuthorizationOnIOThread, this,
+                 OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, media::AudioParameters(),
+                 std::string(), false)));
+
   ipc_->RequestDeviceAuthorization(this, session_id_, device_id_,
                                    security_origin_);
+  task_runner()->PostDelayedTask(
+      FROM_HERE, auth_timeout_action_->callback(),
+      base::TimeDelta::FromMilliseconds(kAuthorizationTimeoutMs));
 }
 
 void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
@@ -229,6 +246,11 @@ void AudioOutputDevice::ShutDownOnIOThread() {
   }
   start_on_authorized_ = false;
 
+  if (auth_timeout_action_) {

[DaleCurtis, 2016/06/09 18:25:34]

No need for this with the timer (or cancellable really), it'll auto drop on
destruction.

[o1ka, 2016/06/10 14:21:08]

I guess we do need this, because  both cancellable and timer are not thread-safe
and should be destroyed on the thread they are used on.
For example:
https://cs-staging.chromium.org/chromium/src/base/timer/timer.h?sq=package:ch...

+    auth_timeout_action_->Cancel();
+    auth_timeout_action_.reset(nullptr);
+  }
+
   // 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,7 +306,36 @@ void AudioOutputDevice::OnDeviceAuthorized(
     OutputDeviceStatus device_status,
     const media::AudioParameters& output_params,
     const std::string& matched_device_id) {
+  ProcessDeviceAuthorizationOnIOThread(device_status, output_params,
+                                       matched_device_id, true);
+}
+
+void AudioOutputDevice::ProcessDeviceAuthorizationOnIOThread(

[o1ka, 2016/06/08 15:46:06]

Will move it to an appropriate place, now it's here to make diff simpler.

+    OutputDeviceStatus device_status,
+    const media::AudioParameters& output_params,
+    const std::string& matched_device_id,
+    bool responce_received) {

[Guido Urdaneta, 2016/06/09 12:09:49]

s/responce/response

or maybe use a more explicit name like |did_time_out|.

[o1ka, 2016/06/10 14:21:08]

Done.

   DCHECK(task_runner()->BelongsToCurrentThread());
+
+  if (responce_received) {
+    if (auth_timeout_action_)

[DaleCurtis, 2016/06/09 18:25:34]

Would just be .Stop() with timer. You can always call it outside of the nest
here and simplifyy these nested conditionals to a single line w/ return.

[o1ka, 2016/06/10 14:21:08]

Done.

+      auth_timeout_action_->Cancel();
+
+    if (state_ == IPC_CLOSED) {
+      // ProcessDeviceAuthorizationOnIOThread has already been called with
+      // |responce_received| set to false, i.e. authorization timed out, but now
+      // we received the authorization responce. Do nothing, since we can't
+      // upgrade device information after |did_receive_auth_| is signalled.
+      // TODO(olka): log UMA stat.
+      return;
+    }
+  }
+
+  if (!responce_received) {

[Guido Urdaneta, 2016/06/09 12:09:49]

else?

[o1ka, 2016/06/09 12:38:28]

Yes, I'll need "else" for UMA stats (those will be in the next patch if all the
reviewers approve the approach).

[o1ka, 2016/06/10 14:21:08]

Done.

+    DCHECK_EQ(OUTPUT_DEVICE_STATUS_ERROR_INTERNAL, device_status);
+    // TODO(olka): log UMA stat.
+  }
+
   DCHECK_EQ(state_, AUTHORIZING);
 
   // It may happen that a second authorization is received as a result to a