Fix race in AudioRendererHost around render frame ID validation.

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 97 matching lines @@
     *params = media::AudioParameters::UnavailableDeviceParams();
 }
 
 void UMALogDeviceAuthorizationTime(base::TimeTicks auth_start_time) {
   UMA_HISTOGRAM_CUSTOM_TIMES("Media.Audio.OutputDeviceAuthorizationTime",
                               base::TimeTicks::Now() - auth_start_time,
                               base::TimeDelta::FromMilliseconds(1),
                               base::TimeDelta::FromMilliseconds(5000), 50);
 }
 
-#if DCHECK_IS_ON()
 // 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));
 }
-#endif  // DCHECK_IS_ON()
 
 }  // namespace
 
 class AudioRendererHost::AudioEntry
     : public media::AudioOutputController::EventHandler {
  public:
   AudioEntry(AudioRendererHost* host,
              int stream_id,
              int render_frame_id,
              const media::AudioParameters& params,
@@ skipping 81 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),
-#if DCHECK_IS_ON()
       validate_render_frame_id_function_(&ValidateRenderFrameId),
-#endif  // DCHECK_IS_ON()
       max_simultaneous_streams_(0) {
   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
@@ skipping 103 matching lines @@
   if (!reader->PrepareForeignSocket(PeerHandle(), &socket_descriptor)) {
     ReportErrorAndClose(entry->stream_id());
     return;
   }
 
   Send(new AudioMsg_NotifyStreamCreated(
       entry->stream_id(), foreign_memory_handle, socket_descriptor,
       entry->shared_memory()->requested_size()));
 }
 
+void AudioRendererHost::DidValidateRenderFrame(int stream_id, bool is_valid) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+ if (!is_valid) {
+   DLOG(WARNING) << "Render frame for stream (id=" << stream_id
+                 << ") no longer exists.";
+   ReportErrorAndClose(stream_id);

[o1ka, 2016/09/05 11:58:38]

My concern is that (as far as I understand) this may be scheduled before or
after DoCompleteCreation() => we have two sequences for the same situation,
which complicates the system. Is it possible to somehow make a final decision in
DoCompleteCreation() and to not send AudioMsg_NotifyStreamCreated if frame
validation fails (I've tried to come up with the idea how to do that, but got a
bit lost in all the thread hopping - I do not know that code well enough yet)?

[DaleCurtis, 2016/09/06 19:24:40]

If this isn't okay then none of our error handling code is okay :)

+ }
+}
+
 void AudioRendererHost::DoNotifyStreamStateChanged(int stream_id,
                                                    bool is_playing) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   AudioEntry* const entry = LookupById(stream_id);
   if (!entry)
     return;
 
   Send(new AudioMsg_NotifyStreamStateChanged(
       stream_id,
@@ skipping 196 matching lines @@
   // 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 DCHECK_IS_ON()
-  // 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.
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(validate_render_frame_id_function_, render_process_id_,
-                 render_frame_id,
-                 base::Bind(&AudioRendererHost::DoCreateStream, this, stream_id,
-                            render_frame_id, params, device_unique_id)));
-#else
-  DoCreateStream(stream_id, render_frame_id, params, device_unique_id,
-                 render_frame_id > 0);
-#endif  // DCHECK_IS_ON()
-}
-
-void AudioRendererHost::DoCreateStream(int stream_id,
-                                       int render_frame_id,
-                                       const media::AudioParameters& params,
-                                       const std::string& device_unique_id,
-                                       bool render_frame_id_is_valid) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
   // 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).
+  //   2. An out-of-range render frame ID was provided. 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_id_is_valid) {
+  if (render_frame_id <= 0) {
     SendErrorMessage(stream_id);
     return;
   }
 
+  // Post a task to the UI thread to check that the |render_frame_id| references
+  // a valid render frame. This validation is important for all the reasons
+  // stated in the comments above. This does not block stream creation, but will
+  // force-close the stream later if validation fails.
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(validate_render_frame_id_function_, render_process_id_,
+                 render_frame_id,
+                 base::Bind(&AudioRendererHost::DidValidateRenderFrame, this,
+                            stream_id)));
+
   // 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;
   }
 
   std::unique_ptr<AudioSyncReader> reader(
@@ skipping 234 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