Change sync primitives ownership for audio rendering.

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 9e33d82aff60381664dae784eabeba681f814309..e959f1c8bc4250bb7b1ccb66fbe9fd24ab66f97a 100644
--- a/content/browser/renderer_host/media/audio_renderer_host.cc
+++ b/content/browser/renderer_host/media/audio_renderer_host.cc
@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/lazy_instance.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/shared_memory.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/process/process.h"
@@ -134,15 +135,16 @@ void ValidateRenderFrameId(int render_process_id,
 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,
-             std::unique_ptr<base::SharedMemory> shared_memory,
-             std::unique_ptr<media::AudioOutputController::SyncReader> reader);
   ~AudioEntry() override;
 
+  // Returns nullptr on failure.
+  static std::unique_ptr<AudioRendererHost::AudioEntry> Create(
+      AudioRendererHost* host,
+      int stream_id,
+      int render_frame_id,
+      const media::AudioParameters& params,
+      const std::string& output_device_id);
+
   int stream_id() const {
     return stream_id_;
   }
@@ -151,18 +153,20 @@ class AudioRendererHost::AudioEntry
 
   media::AudioOutputController* controller() const { return controller_.get(); }
 
-  base::SharedMemory* shared_memory() {
-    return shared_memory_.get();
-  }
-
-  media::AudioOutputController::SyncReader* reader() const {
-    return reader_.get();
-  }
+  AudioSyncReader* reader() const { return reader_.get(); }
 
+  // Used by ARH to track the number of active streams for UMA stats.
   bool playing() const { return playing_; }
   void set_playing(bool playing) { playing_ = playing; }
 
  private:
+  AudioEntry(AudioRendererHost* host,
+             int stream_id,
+             int render_frame_id,
+             const media::AudioParameters& params,
+             const std::string& output_device_id,
+             std::unique_ptr<AudioSyncReader> reader);
+
   // media::AudioOutputController::EventHandler implementation.
   void OnCreated() override;
   void OnPlaying() override;
@@ -175,16 +179,15 @@ class AudioRendererHost::AudioEntry
   // The routing ID of the source RenderFrame.
   const int render_frame_id_;
 
-  // Shared memory for transmission of the audio data.  Used by |reader_|.
-  const std::unique_ptr<base::SharedMemory> shared_memory_;
-
   // The synchronous reader to be used by |controller_|.
-  const std::unique_ptr<media::AudioOutputController::SyncReader> reader_;
+  const std::unique_ptr<AudioSyncReader> reader_;
 
   // The AudioOutputController that manages the audio stream.
   const scoped_refptr<media::AudioOutputController> controller_;
 
   bool playing_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioEntry);
 };
 
 AudioRendererHost::AudioEntry::AudioEntry(
@@ -193,12 +196,10 @@ AudioRendererHost::AudioEntry::AudioEntry(
     int render_frame_id,
     const media::AudioParameters& params,
     const std::string& output_device_id,
-    std::unique_ptr<base::SharedMemory> shared_memory,
-    std::unique_ptr<media::AudioOutputController::SyncReader> reader)
+    std::unique_ptr<AudioSyncReader> reader)
     : host_(host),
       stream_id_(stream_id),
       render_frame_id_(render_frame_id),
-      shared_memory_(std::move(shared_memory)),
       reader_(std::move(reader)),
       controller_(media::AudioOutputController::Create(host->audio_manager_,
                                                        this,
@@ -206,11 +207,27 @@ AudioRendererHost::AudioEntry::AudioEntry(
                                                        output_device_id,
                                                        reader_.get())),
       playing_(false) {
-  DCHECK(controller_.get());
+  DCHECK(controller_);
 }
 
 AudioRendererHost::AudioEntry::~AudioEntry() {}
 
+// static
+std::unique_ptr<AudioRendererHost::AudioEntry>
+AudioRendererHost::AudioEntry::Create(AudioRendererHost* host,
+                                      int stream_id,
+                                      int render_frame_id,
+                                      const media::AudioParameters& params,
+                                      const std::string& output_device_id) {
+  std::unique_ptr<AudioSyncReader> reader(AudioSyncReader::Create(params));
+  if (!reader) {
+    return {};

[DaleCurtis, 2016/09/16 23:29:12]

Just return nullptr?

[Max Morin, 2016/09/19 10:13:01]

Done.

+  }
+  return base::WrapUnique(new AudioEntry(host, stream_id, render_frame_id,
+                                         params, output_device_id,
+                                         std::move(reader)));
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // AudioRendererHost implementations.
 
@@ -326,41 +343,37 @@ void AudioRendererHost::DoCompleteCreation(int stream_id) {
     return;
   }
 
-  // Once the audio stream is created then complete the creation process by
-  // mapping shared memory and sharing with the renderer process.
-  base::SharedMemoryHandle foreign_memory_handle;
-  if (!entry->shared_memory()->ShareToProcess(PeerHandle(),
-                                              &foreign_memory_handle)) {
-    // If we failed to map and share the shared memory then close the audio
-    // stream and send an error message.
-    ReportErrorAndClose(entry->stream_id());
-    return;
-  }
-
-  AudioSyncReader* reader = static_cast<AudioSyncReader*>(entry->reader());
+  // Now construction is done and we are ready to send the shared memory and the
+  // sync socket to the renderer.
+  base::SharedMemory* shared_memory = entry->reader()->shared_memory();
+  base::CancelableSyncSocket* foreign_socket =
+      entry->reader()->foreign_socket();
 
+  base::SharedMemoryHandle foreign_memory_handle;
   base::SyncSocket::TransitDescriptor socket_descriptor;
+  size_t shared_memory_size = shared_memory->requested_size();

[DaleCurtis, 2016/09/16 23:29:12]

Do we sanitize this anywhere else? Otherwise seems like renderer could request
arbitrary blocks?

[Max Morin, 2016/09/19 10:13:01]

This is sanitized in the IPC system:
https://cs.chromium.org/chromium/src/media/base/ipc/media_param_traits.cc?sq=....

 
-  // If we failed to prepare the sync socket for the renderer then we fail
-  // the construction of audio stream.
-  if (!reader->PrepareForeignSocket(PeerHandle(), &socket_descriptor)) {
+  if (!(shared_memory->ShareToProcess(PeerHandle(), &foreign_memory_handle) &&
+        foreign_socket->PrepareTransitDescriptor(PeerHandle(),
+                                                 &socket_descriptor))) {
+    // Something went wrong in preparing the IPC handles.
     ReportErrorAndClose(entry->stream_id());
     return;
   }
 
   Send(new AudioMsg_NotifyStreamCreated(
-      entry->stream_id(), foreign_memory_handle, socket_descriptor,
-      entry->shared_memory()->requested_size()));
+      stream_id, foreign_memory_handle, socket_descriptor,
+      base::checked_cast<uint32_t>(shared_memory_size)));
 }
 
 void AudioRendererHost::DidValidateRenderFrame(int stream_id, bool is_valid) {
- DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
- if (!is_valid) {
-   DLOG(WARNING) << "Render frame for stream (id=" << stream_id
-                 << ") no longer exists.";
-   ReportErrorAndClose(stream_id);
- }
+  if (!is_valid) {
+    DLOG(WARNING) << "Render frame for stream (id=" << stream_id
+                  << ") no longer exists.";
+    ReportErrorAndClose(stream_id);
+  }
 }
 
 void AudioRendererHost::DoNotifyStreamStateChanged(int stream_id,
@@ -608,18 +621,9 @@ void AudioRendererHost::OnCreateStream(int stream_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(
-      new AudioSyncReader(shared_memory.get(), params));
-  if (!reader->Init()) {
+  std::unique_ptr<AudioEntry> entry = AudioEntry::Create(
+      this, stream_id, render_frame_id, params, device_unique_id);
+  if (!entry) {
     SendErrorMessage(stream_id);
     return;
   }
@@ -629,9 +633,6 @@ void AudioRendererHost::OnCreateStream(int stream_id,
   if (media_observer)
     media_observer->OnCreatingAudioStream(render_process_id_, render_frame_id);
 
-  std::unique_ptr<AudioEntry> entry(
-      new AudioEntry(this, stream_id, render_frame_id, params, device_unique_id,
-                     std::move(shared_memory), std::move(reader)));
   if (mirroring_manager_) {
     mirroring_manager_->AddDiverter(
         render_process_id_, entry->render_frame_id(), entry->controller());