Manage MIDI related objects and sessions' lifecycles correctly

content/renderer/media/midi_message_filter.cc

Index: content/renderer/media/midi_message_filter.cc
diff --git a/content/renderer/media/midi_message_filter.cc b/content/renderer/media/midi_message_filter.cc
index 165451a50f695fc34bb7cc8d33d56d5f07a0bb7c..27fe2a72fa63d74e239f4ff7d5abe2298a63fa48 100644
--- a/content/renderer/media/midi_message_filter.cc
+++ b/content/renderer/media/midi_message_filter.cc
@@ -4,6 +4,8 @@
 
 #include "content/renderer/media/midi_message_filter.h"
 
+#include <algorithm>
+
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/message_loop/message_loop_proxy.h"
@@ -27,12 +29,88 @@ MidiMessageFilter::MidiMessageFilter(
     : sender_(NULL),
       io_message_loop_(io_message_loop),
       main_message_loop_(base::MessageLoopProxy::current()),
-      next_available_id_(0),
+      session_result_(media::MIDI_NOT_INITIALIZED),
       unacknowledged_bytes_sent_(0) {
 }
 
 MidiMessageFilter::~MidiMessageFilter() {}
 
+void MidiMessageFilter::AddClient(blink::WebMIDIAccessorClient* client) {
+  TRACE_EVENT0("midi", "MidiMessageFilter::AddClient");
+  base::AutoLock auto_lock(lock_);
+  if (session_result_ != media::MIDI_NOT_INITIALIZED) {
+    main_message_loop_->PostTask(FROM_HERE,
+        base::Bind(&MidiMessageFilter::HandleClientAdded, this, client));
+  } else if (!clients_waiting_session_queue_.empty()) {
+    clients_waiting_session_queue_.push_back(client);
+  } else {
+    clients_waiting_session_queue_.push_back(client);
+    io_message_loop_->PostTask(FROM_HERE,
+        base::Bind(&MidiMessageFilter::StartSessionOnIOThread, this));
+  }
+}
+
+void MidiMessageFilter::RemoveClient(blink::WebMIDIAccessorClient* client) {
+  base::AutoLock auto_lock(lock_);
+  clients_.erase(client);
+  ClientsQueue::iterator it = std::find(clients_waiting_session_queue_.begin(),
+                                        clients_waiting_session_queue_.end(),
+                                        client);
+  if (it != clients_waiting_session_queue_.end())
+    clients_waiting_session_queue_.erase(it);
+  if (clients_.empty() && clients_waiting_session_queue_.empty()) {
+    session_result_ = media::MIDI_NOT_INITIALIZED;
+    io_message_loop_->PostTask(FROM_HERE,
+        base::Bind(&MidiMessageFilter::EndSessionOnIOThread, this));
+  }
+}
+
+void MidiMessageFilter::SendMidiData(uint32 port,

[scherkus (not reviewing), 2014/10/20 18:05:48]

which thread is this typically called on?

it seems like most of your thread interactions are between IO and main, which
would make me think you could save yourself a lot of trouble by using a
RenderFrameObserver (which handles receiving/sending messages between IO and
main) vs. a MessageFilter

[Takashi Toyoshima, 2014/10/21 01:24:07]

This function should be called from main thread.

Using RenderFrameObserver sounds good idea.
Can I added TODO for now? I'll rewrite this class as RenderFrameObserver in a
separate CL after a series of functional changes for crbug.com/424859.

[scherkus (not reviewing), 2014/10/21 17:06:17]

Sounds good.

I'm not guaranteeing it'll work but I do think it's worth spending some time
investigating whether you can swap it out. It'll reduce the amount of
boilerplate handling messages/clients between IO and main threads.

In general I'm also wary of mixing locks with message passing via PostTask() --
hopefully using RenderFrameObserver works out!

+                                     const uint8* data,
+                                     size_t length,
+                                     double timestamp) {
+  if (length > kMaxUnacknowledgedBytesSent) {
+    // TODO(toyoshim): buffer up the data to send at a later time.
+    // For now we're just dropping these bytes on the floor.
+    return;
+  }
+
+  std::vector<uint8> v(data, data + length);
+  io_message_loop_->PostTask(FROM_HERE,
+      base::Bind(&MidiMessageFilter::SendMidiDataOnIOThread, this,
+                 port, v, timestamp));
+}
+
+void MidiMessageFilter::StartSessionOnIOThread() {
+  TRACE_EVENT0("midi", "MidiMessageFilter::StartSessionOnIOThread");
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  Send(new MidiHostMsg_StartSession());
+}
+
+void MidiMessageFilter::SendMidiDataOnIOThread(uint32 port,
+                                               const std::vector<uint8>& data,
+                                               double timestamp) {
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  size_t n = data.size();
+  if (n > kMaxUnacknowledgedBytesSent ||
+      unacknowledged_bytes_sent_ > kMaxUnacknowledgedBytesSent ||
+      n + unacknowledged_bytes_sent_ > kMaxUnacknowledgedBytesSent) {
+    // TODO(toyoshim): buffer up the data to send at a later time.
+    // For now we're just dropping these bytes on the floor.
+    return;
+  }
+
+  unacknowledged_bytes_sent_ += n;
+
+  // Send to the browser.
+  Send(new MidiHostMsg_SendData(port, data, timestamp));
+}
+
+void MidiMessageFilter::EndSessionOnIOThread() {
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  Send(new MidiHostMsg_EndSession());
+}
+
 void MidiMessageFilter::Send(IPC::Message* message) {
   DCHECK(io_message_loop_->BelongsToCurrentThread());
   if (!sender_) {
@@ -61,7 +139,6 @@ void MidiMessageFilter::OnFilterAdded(IPC::Sender* sender) {
 
 void MidiMessageFilter::OnFilterRemoved() {
   DCHECK(io_message_loop_->BelongsToCurrentThread());
-
   // Once removed, a filter will not be used again.  At this time all
   // delegates must be notified so they release their reference.
   OnChannelClosing();
@@ -72,74 +149,75 @@ void MidiMessageFilter::OnChannelClosing() {
   sender_ = NULL;
 }
 
-void MidiMessageFilter::StartSession(blink::WebMIDIAccessorClient* client) {
-  // Generate and keep track of a "client id" which is sent to the browser
-  // to ask permission to talk to MIDI hardware.
-  // This id is handed back when we receive the answer in OnAccessApproved().
-  if (clients_.find(client) == clients_.end()) {
-    int client_id = next_available_id_++;
-    clients_[client] = client_id;
-
-    io_message_loop_->PostTask(FROM_HERE,
-        base::Bind(&MidiMessageFilter::StartSessionOnIOThread, this,
-                   client_id));
+void MidiMessageFilter::OnSessionStarted(media::MidiResult result,
+                                         MidiPortInfoList inputs,
+                                         MidiPortInfoList outputs) {
+  TRACE_EVENT0("midi", "MidiMessageFilter::OnSessionStarted");
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  base::AutoLock auto_lock(lock_);
+  session_result_ = result;
+  inputs_ = inputs;
+  outputs_ = outputs;
+  // Handle on the main JS thread.
+  for (blink::WebMIDIAccessorClient* client : clients_waiting_session_queue_) {
+    main_message_loop_->PostTask(FROM_HERE,
+        base::Bind(&MidiMessageFilter::HandleClientAdded, this, client));
   }
+  clients_waiting_session_queue_.clear();
 }
 
-void MidiMessageFilter::StartSessionOnIOThread(int client_id) {
-  Send(new MidiHostMsg_StartSession(client_id));
-}
-
-void MidiMessageFilter::RemoveClient(blink::WebMIDIAccessorClient* client) {
-  ClientsMap::iterator i = clients_.find(client);
-  if (i != clients_.end())
-    clients_.erase(i);
-}
-
-// Received from browser.
-
-void MidiMessageFilter::OnSessionStarted(
-    int client_id,
-    media::MidiResult result,
-    MidiPortInfoList inputs,
-    MidiPortInfoList outputs) {
-  // Handle on the main JS thread.
+void MidiMessageFilter::OnDataReceived(uint32 port,
+                                       const std::vector<uint8>& data,
+                                       double timestamp) {
+  TRACE_EVENT0("midi", "MidiMessageFilter::OnDataReceived");
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
   main_message_loop_->PostTask(
       FROM_HERE,
-      base::Bind(&MidiMessageFilter::HandleSessionStarted, this,
-                 client_id, result, inputs, outputs));
+      base::Bind(&MidiMessageFilter::HandleDataReceived, this,
+                 port, data, timestamp));
 }
 
-void MidiMessageFilter::HandleSessionStarted(
-    int client_id,
-    media::MidiResult result,
-    MidiPortInfoList inputs,
-    MidiPortInfoList outputs) {
-  blink::WebMIDIAccessorClient* client = GetClientFromId(client_id);
-  if (!client)
-    return;
+void MidiMessageFilter::OnAcknowledgeSentData(size_t bytes_sent) {
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  DCHECK_GE(unacknowledged_bytes_sent_, bytes_sent);
+  if (unacknowledged_bytes_sent_ >= bytes_sent)
+    unacknowledged_bytes_sent_ -= bytes_sent;
+}
 
+void MidiMessageFilter::HandleClientAdded(
+    blink::WebMIDIAccessorClient* client) {
+  TRACE_EVENT0("midi", "MidiMessageFilter::HandleClientAdded");
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
+  media::MidiResult result;
+  {
+    base::AutoLock auto_lock(lock_);
+    result = session_result_;
+  }
   if (result == media::MIDI_OK) {
     // Add the client's input and output ports.
     const bool active = true;
-    for (size_t i = 0; i < inputs.size(); ++i) {
+    for (media::MidiPortInfo info : inputs_) {

[scherkus (not reviewing), 2014/10/20 18:05:48]

won't this create a copy?

you could use const auto&

[Takashi Toyoshima, 2014/10/21 01:49:46]

Done.

       client->didAddInputPort(
-          base::UTF8ToUTF16(inputs[i].id),
-          base::UTF8ToUTF16(inputs[i].manufacturer),
-          base::UTF8ToUTF16(inputs[i].name),
-          base::UTF8ToUTF16(inputs[i].version),
-          active);
+          base::UTF8ToUTF16(info.id),
+          base::UTF8ToUTF16(info.manufacturer),
+          base::UTF8ToUTF16(info.name),
+          base::UTF8ToUTF16(info.version),
+          true);
     }
 
-    for (size_t i = 0; i < outputs.size(); ++i) {
+    for (media::MidiPortInfo info : outputs_) {

[scherkus (not reviewing), 2014/10/20 18:05:48]

ditto

[Takashi Toyoshima, 2014/10/21 01:49:46]

Done.

       client->didAddOutputPort(
-          base::UTF8ToUTF16(outputs[i].id),
-          base::UTF8ToUTF16(outputs[i].manufacturer),
-          base::UTF8ToUTF16(outputs[i].name),
-          base::UTF8ToUTF16(outputs[i].version),
-          active);
+          base::UTF8ToUTF16(info.id),
+          base::UTF8ToUTF16(info.manufacturer),
+          base::UTF8ToUTF16(info.name),
+          base::UTF8ToUTF16(info.version),
+          true);
     }
   }
+  {
+    base::AutoLock auto_lock(lock_);
+    clients_.insert(client);
+  }
   std::string error;
   std::string message;
   switch (result) {
@@ -162,78 +240,15 @@ void MidiMessageFilter::HandleSessionStarted(
                           base::UTF8ToUTF16(message));
 }
 
-blink::WebMIDIAccessorClient*
-MidiMessageFilter::GetClientFromId(int client_id) {
-  // Iterating like this seems inefficient, but in practice there generally
-  // will be very few clients (usually one).  Additionally, this lookup
-  // usually happens one time during page load. So the performance hit is
-  // negligible.
-  for (ClientsMap::iterator i = clients_.begin(); i != clients_.end(); ++i) {
-    if ((*i).second == client_id)
-      return (*i).first;
-  }
-  return NULL;
-}
-
-void MidiMessageFilter::OnDataReceived(uint32 port,
-                                       const std::vector<uint8>& data,
-                                       double timestamp) {
-  TRACE_EVENT0("midi", "MidiMessageFilter::OnDataReceived");
-
-  main_message_loop_->PostTask(
-      FROM_HERE,
-      base::Bind(&MidiMessageFilter::HandleDataReceived, this,
-                 port, data, timestamp));
-}
-
-void MidiMessageFilter::OnAcknowledgeSentData(size_t bytes_sent) {
-  DCHECK_GE(unacknowledged_bytes_sent_, bytes_sent);
-  if (unacknowledged_bytes_sent_ >= bytes_sent)
-    unacknowledged_bytes_sent_ -= bytes_sent;
-}
-
 void MidiMessageFilter::HandleDataReceived(uint32 port,
                                            const std::vector<uint8>& data,
                                            double timestamp) {
-  DCHECK(!data.empty());
   TRACE_EVENT0("midi", "MidiMessageFilter::HandleDataReceived");
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
+  DCHECK(!data.empty());
 
-  for (ClientsMap::iterator i = clients_.begin(); i != clients_.end(); ++i)
-    (*i).first->didReceiveMIDIData(port, &data[0], data.size(), timestamp);
-}
-
-void MidiMessageFilter::SendMidiData(uint32 port,
-                                     const uint8* data,
-                                     size_t length,
-                                     double timestamp) {
-  if (length > kMaxUnacknowledgedBytesSent) {
-    // TODO(toyoshim): buffer up the data to send at a later time.
-    // For now we're just dropping these bytes on the floor.
-    return;
-  }
-
-  std::vector<uint8> v(data, data + length);
-  io_message_loop_->PostTask(FROM_HERE,
-      base::Bind(&MidiMessageFilter::SendMidiDataOnIOThread, this,
-                 port, v, timestamp));
-}
-
-void MidiMessageFilter::SendMidiDataOnIOThread(uint32 port,
-                                               const std::vector<uint8>& data,
-                                               double timestamp) {
-  size_t n = data.size();
-  if (n > kMaxUnacknowledgedBytesSent ||
-      unacknowledged_bytes_sent_ > kMaxUnacknowledgedBytesSent ||
-      n + unacknowledged_bytes_sent_ > kMaxUnacknowledgedBytesSent) {
-    // TODO(toyoshim): buffer up the data to send at a later time.
-    // For now we're just dropping these bytes on the floor.
-    return;
-  }
-
-  unacknowledged_bytes_sent_ += n;
-
-  // Send to the browser.
-  Send(new MidiHostMsg_SendData(port, data, timestamp));
+  for (blink::WebMIDIAccessorClient* client : clients_)
+    client->didReceiveMIDIData(port, &data[0], data.size(), timestamp);
 }
 
 }  // namespace content