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 e6a7f8067beff37bea082b5ce503a6ef75567208..c91400e24e97fc94271ac5024be7ac4a763d4fa6 100644
--- a/content/renderer/media/midi_message_filter.cc
+++ b/content/renderer/media/midi_message_filter.cc
@@ -27,12 +27,92 @@ 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) {

[yhirano, 2014/10/20 12:15:14]

Is it OK to behave as if there is no error when session_result != MIDI_OK?

[Takashi Toyoshima, 2014/10/20 17:14:44]

Yes, I think it's ok to cache the error results here.
Basically, the result should not be changed, and user may try it in another tab
if something is wrong.
I think reload attack makes hardware be in worse situation.

+    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_);
+  ClientsSet::iterator set_it = clients_.find(client);

[yhirano, 2014/10/20 12:15:14]

Isn't |client_.erase(client)| enough?

[Takashi Toyoshima, 2014/10/20 17:14:44]

Done.

+  if (set_it != clients_.end())
+    clients_.erase(set_it);
+  for (ClientsQueue::iterator queue_it = clients_waiting_session_queue_.begin();

[yhirano, 2014/10/20 12:15:14]

How about using std::find?

[Takashi Toyoshima, 2014/10/20 17:14:43]

Done.

+       queue_it != clients_waiting_session_queue_.end(); ++queue_it) {
+    if (*queue_it == client) {
+      clients_waiting_session_queue_.erase(queue_it);
+      break;
+    }
+  }
+  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,
+                                     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 +141,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,70 +151,69 @@ 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 (ClientsQueue::iterator it = clients_waiting_session_queue_.begin();

[yhirano, 2014/10/20 12:15:14]

You can use the range-based for.

[Takashi Toyoshima, 2014/10/20 17:14:44]

Done.

+       it != clients_waiting_session_queue_.end(); ++it) {
+    main_message_loop_->PostTask(FROM_HERE,
+        base::Bind(&MidiMessageFilter::HandleClientAdded, this, *it));
   }
+  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;
-
-  if (result == media::MIDI_OK) {
-    // Add the client's input and output ports.
-    for (size_t i = 0; i < inputs.size(); ++i) {
-      client->didAddInputPort(
-          base::UTF8ToUTF16(inputs[i].id),
-          base::UTF8ToUTF16(inputs[i].manufacturer),
-          base::UTF8ToUTF16(inputs[i].name),
-          base::UTF8ToUTF16(inputs[i].version));
-    }
+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;
+}
 
-    for (size_t i = 0; i < outputs.size(); ++i) {
-      client->didAddOutputPort(
-          base::UTF8ToUTF16(outputs[i].id),
-          base::UTF8ToUTF16(outputs[i].manufacturer),
-          base::UTF8ToUTF16(outputs[i].name),
-          base::UTF8ToUTF16(outputs[i].version));
+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) {

[yhirano, 2014/10/20 12:15:14]

You can bring this block out of auto_lock's scope.

[Takashi Toyoshima, 2014/10/20 17:14:43]

Done.

+      // Add the client's input and output ports.
+      for (size_t i = 0; i < inputs_.size(); ++i) {

[yhirano, 2014/10/20 12:15:14]

You can use the range-based for.

[Takashi Toyoshima, 2014/10/20 17:14:44]

Done.

+        client->didAddInputPort(
+            base::UTF8ToUTF16(inputs_[i].id),
+            base::UTF8ToUTF16(inputs_[i].manufacturer),
+            base::UTF8ToUTF16(inputs_[i].name),
+            base::UTF8ToUTF16(inputs_[i].version));
+      }
+
+      for (size_t i = 0; i < outputs_.size(); ++i) {

[yhirano, 2014/10/20 12:15:14]

ditto

[Takashi Toyoshima, 2014/10/20 17:14:44]

Done.

+        client->didAddOutputPort(
+            base::UTF8ToUTF16(outputs_[i].id),
+            base::UTF8ToUTF16(outputs_[i].manufacturer),
+            base::UTF8ToUTF16(outputs_[i].name),
+            base::UTF8ToUTF16(outputs_[i].version));
+      }
     }
+    clients_.insert(client);
   }
   std::string error;
   std::string message;
@@ -159,78 +237,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 (ClientsSet::iterator it = clients_.begin(); it != clients_.end(); ++it)

[yhirano, 2014/10/20 12:15:14]

You can use the range-based for.

[Takashi Toyoshima, 2014/10/20 17:14:44]

Done.

+    (*it)->didReceiveMIDIData(port, &data[0], data.size(), timestamp);
 }
 
 }  // namespace content