Add back RenderFrameHost sanity-check to AudioRendererHost.

content/browser/renderer_host/media/audio_renderer_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/media/audio_renderer_host.h"
 
 #include <stdint.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 91 matching lines @@
   // better to report a valid value if this is the only problem.
   if (params->channels() > media::limits::kMaxChannels)
     params->set_channels_for_discrete(media::limits::kMaxChannels);
 
   // If hardware parameters are still invalid, use dummy parameters with
   // fake audio path and let the client handle the error.
   if (!params->IsValid())
     *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
     : public media::AudioOutputController::EventHandler {
  public:
   AudioEntry(AudioRendererHost* host,
              int stream_id,
              int render_frame_id,
              const media::AudioParameters& params,
              const std::string& output_device_id,
@@ skipping 80 matching lines @@
                                      const std::string& salt)
     : BrowserMessageFilter(AudioMsgStart),
       render_process_id_(render_process_id),
       audio_manager_(audio_manager),
       mirroring_manager_(mirroring_manager),
       audio_log_(media_internals->CreateAudioLog(
           media::AudioLogFactory::AUDIO_OUTPUT_CONTROLLER)),
       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_);
 }
 
 AudioRendererHost::~AudioRendererHost() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   CHECK(audio_entries_.empty());
 
   // If we had any streams, report UMA stats for the maximum number of
   // simultaneous streams for this render process and for the whole browser
@@ skipping 294 matching lines @@
       stream_id, media::OUTPUT_DEVICE_STATUS_OK, output_params, std::string()));
 }
 
 void AudioRendererHost::OnCreateStream(int stream_id,
                                        int render_frame_id,
                                        const media::AudioParameters& params) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   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;
+  }
 
   // Create the shared memory and share with the renderer process.
   uint32_t shared_memory_size = sizeof(media::AudioOutputBufferParameters) +
                                 AudioBus::CalculateMemorySize(params);
   std::unique_ptr<base::SharedMemory> shared_memory(new base::SharedMemory());
   if (!shared_memory->CreateAndMapAnonymous(shared_memory_size)) {
     SendErrorMessage(stream_id);
     return;
   }
 
@@ skipping 246 matching lines @@
   callback.Run(false, device_info);
 }
 
 bool AudioRendererHost::IsAuthorizationStarted(int stream_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   const auto& i = authorizations_.find(stream_id);
   return i != authorizations_.end();
 }
 
 }  // namespace content