Add back RenderFrameHost sanity-check to AudioRendererHost.

content/browser/renderer_host/media/audio_renderer_host.cc

Index: content/browser/renderer_host/media/audio_renderer_host.cc
diff --git a/content/browser/renderer_host/media/audio_renderer_host.cc b/content/browser/renderer_host/media/audio_renderer_host.cc
index 947c00cd4331e84959852f61e8023fffb8a00680..a7c3091410dd8bbfdd64c348c505e620b2c7d6d8 100644
--- a/content/browser/renderer_host/media/audio_renderer_host.cc
+++ b/content/browser/renderer_host/media/audio_renderer_host.cc
@@ -109,6 +109,18 @@ void MaybeFixAudioParameters(media::AudioParameters* params) {
     *params = media::AudioParameters::UnavailableDeviceParams();
 }
 
+// Check that the routing ID references a valid RenderFrameHost, and run
+// |callback| on the IO thread with true if the ID is valid.
+void ValidateRenderFrameId(int render_process_id,
+                           int render_frame_id,
+                           const base::Callback<void(bool)>& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  const bool frame_exists =
+      !!RenderFrameHost::FromID(render_process_id, render_frame_id);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(callback, frame_exists));
+}
+
 }  // namespace
 
 class AudioRendererHost::AudioEntry
@@ -209,7 +221,8 @@ AudioRendererHost::AudioRendererHost(int render_process_id,
       media_stream_manager_(media_stream_manager),
       num_playing_streams_(0),
       salt_(salt),
-      max_simultaneous_streams_(0) {
+      max_simultaneous_streams_(0),
+      validate_render_frame_id_function_(&ValidateRenderFrameId) {
   DCHECK(audio_manager_);
   DCHECK(media_stream_manager_);
 }
@@ -524,27 +537,61 @@ void AudioRendererHost::OnCreateStream(int stream_id,
   DVLOG(1) << "AudioRendererHost@" << this << "::OnCreateStream"
            << "(stream_id=" << stream_id << ")";
 
+  // Determine whether to use the device_unique_id from an authorization, or an
+  // empty string (i.e., when no previous authorization was requested, assume
+  // default device).
+  std::string device_unique_id;
   const auto& auth_data = authorizations_.find(stream_id);
+  if (auth_data != authorizations_.end()) {
+    CHECK(auth_data->second.first);
+    device_unique_id.swap(auth_data->second.second);
+    authorizations_.erase(auth_data);
+  }
 
-  // If no previous authorization requested, assume default device
-  if (auth_data == authorizations_.end()) {
-    DoCreateStream(stream_id, render_frame_id, params, std::string());
+  // When DCHECKs are turned on, hop over to the UI thread to validate the
+  // |render_frame_id|, then continue stream creation on the IO thread. See
+  // comment at top of DoCreateStream() for further details.
+  if (DCHECK_IS_ON()) {

[o1ka, 2016/07/08 14:09:38]

#if DCHECK_IS_ON() ?

[DaleCurtis, 2016/07/08 18:44:33]

Hmm, I think we want #if syntax here instead, there are issues with this syntax
and linking if functions are excluded.

[miu, 2016/07/08 21:21:37]

Done.

[miu, 2016/07/08 21:21:37]

Done.

+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(
+            validate_render_frame_id_function_, render_process_id_,

[o1ka, 2016/07/08 14:09:38]

What if validate_render_frame_id_function_ is nullptr? (set for testing, or just
because of future code modifications). Add DCHECK?

[miu, 2016/07/08 21:21:37]

It can't be. The ARH ctor sets it, so a test would have to explicitly set it to
nullptr. If any future code change should set it to nullptr, then the developer
will immediately know because the process will crash when attempting to run any
tests (or commit the change in the CQ).

[o1ka, 2016/07/11 10:35:50]

I would say it's still more human-friendly to add a DCHECK here: it's much more
explicit than a crash in a callback and requires no debugging to understand what
the problem is.

[miu, 2016/07/14 21:36:12]

Yep. The DCHECK() is already being done here:
https://cs.chromium.org/chromium/src/base/bind_internal.h?rcl=1468509561&l=385

+            render_frame_id,
+            base::Bind(&AudioRendererHost::DoCreateStream, this, stream_id,
+                       render_frame_id, params, device_unique_id)));
     return;
   }
-
-  CHECK(auth_data->second.first);
-  DoCreateStream(stream_id, render_frame_id, params, auth_data->second.second);
-  authorizations_.erase(auth_data);
+  DoCreateStream(stream_id, render_frame_id, params, device_unique_id,
+                 render_frame_id > 0);
 }
 
 void AudioRendererHost::DoCreateStream(int stream_id,
                                        int render_frame_id,
                                        const media::AudioParameters& params,
-                                       const std::string& device_unique_id) {
+                                       const std::string& device_unique_id,
+                                       bool render_frame_is_valid) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // media::AudioParameters is validated in the deserializer.
-  if (LookupById(stream_id) != NULL) {
+  // Fail early if either of two sanity-checks fail:
+  //   1. There should not yet exist an AudioEntry for the given |stream_id|
+  //      since the renderer may not create two streams with the same ID.
+  //   2. The render frame ID was either invalid or the render frame host it
+  //      references has shutdown before the request could be fulfilled (race
+  //      condition). Renderers must *always* specify a valid render frame ID
+  //      for each audio output they create, as several browser-level features
+  //      depend on this (e.g., OOM manager, UI audio indicator, muting, audio
+  //      capture).
+  // Note: media::AudioParameters is validated in the deserializer, so there is
+  // no need to check that here.
+  if (LookupById(stream_id)) {
+    SendErrorMessage(stream_id);
+    return;
+  }
+  if (!render_frame_is_valid) {
+    // If this DCHECK triggers for your audio feature, please read the comment
+    // above. Stubbing/faking this value is not an acceptable solution.
+    DCHECK_GT(render_frame_id, 0)

[o1ka, 2016/07/08 14:09:38]

Do not quite understand why this DCHECK? why not DCHECK(render_frame_is_valid)?

[miu, 2016/07/08 21:21:37]

My point was to further check whether the ID was invalid because it was <= 0,
which means someone purposely hacked-in a bogus value (such as
MSG_ROUTING_NONE).

I removed this since, on second glace, I think I was just being too excessive
here. Better to keep things simple.

[o1ka, 2016/07/11 10:35:50]

Acknowledged.

+        << "BUG: Invalid render frame ID provided by renderer.";
     SendErrorMessage(stream_id);
     return;
   }