Manage MIDI related objects and sessions' lifecycles correctly

content/renderer/media/midi_message_filter.cc

 // Copyright (c) 2013 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/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"
 #include "base/strings/utf_string_conversions.h"
 #include "content/common/media/midi_messages.h"
 #include "content/renderer/render_thread_impl.h"
 #include "ipc/ipc_logging.h"
 
 using media::MidiPortInfoList;
 using base::AutoLock;
 
 // The maximum number of bytes which we're allowed to send to the browser
 // before getting acknowledgement back from the browser that they've been
 // successfully sent.
 static const size_t kMaxUnacknowledgedBytesSent = 10 * 1024 * 1024;  // 10 MB.
 
 namespace content {
 
+// TODO(crbug.com/425389): Rewrite this class as a RenderFrameObserver.
 MidiMessageFilter::MidiMessageFilter(
     const scoped_refptr<base::MessageLoopProxy>& io_message_loop)
     : 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) {
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
+  TRACE_EVENT0("midi", "MidiMessageFilter::AddClient");
+  base::AutoLock auto_lock(lock_);
+  if (session_result_ != media::MIDI_NOT_INITIALIZED) {
+    main_message_loop_->PostTask(FROM_HERE,

[Takashi Toyoshima, 2014/10/22 05:57:40]

Oh, I do not need to post this :(

+        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) {
+  DCHECK(main_message_loop_->BelongsToCurrentThread());

[yhirano, 2014/10/21 13:21:20]

I have a question: Imagine the following situation.

1. AddClient(c1) is called on the main thread and c1 is added to
clients_waiting_session_queue_.
2. StartSessionOnIOThread is called on the IO thread.
3. OnSessionStarted is called on the IO thread and clears
clients_waiting_session_queue_.
4. RemoveSession(c1) is called on the main thread.
5. HandleClientAdded(c1) is called and c1 is added to clients_.

Isn't it a problem?
From the Blink point of view, it simply added c1 and remove it later, but the
message filter holds the possibly dangling pointer after that.

[Takashi Toyoshima, 2014/10/22 05:57:40]

Hum... that's good point.
Actually, I notice that we should not lock these client queue since all
functions can be called nn main thread should be reentrant for JavaScript. E.g.,
JavaScript callback may create another client, or send data. I redesign the
thread model around here.

+  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,
+                                     const uint8* data,
+                                     size_t length,
+                                     double timestamp) {
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
+  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);

[palmer, 2014/10/21 21:20:12]

Can we assert that |length| is not > some maximum reasonable value (and then
also enforce that on the browser side)?

[Takashi Toyoshima, 2014/10/22 03:50:56]

line 76 is the check. The data comes from JavaScript. So, assertion is not a
good idea. Maybe we should design some error reporting in W3C, but for now we
just discard it.

[palmer, 2014/10/22 19:43:32]

Oops, I just missed it. Yes, thanks.

+  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_) {
     delete message;
   } else {
     sender_->Send(message);
   }
 }
 
 bool MidiMessageFilter::OnMessageReceived(const IPC::Message& message) {
   DCHECK(io_message_loop_->BelongsToCurrentThread());
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(MidiMessageFilter, message)
     IPC_MESSAGE_HANDLER(MidiMsg_SessionStarted, OnSessionStarted)
     IPC_MESSAGE_HANDLER(MidiMsg_DataReceived, OnDataReceived)
     IPC_MESSAGE_HANDLER(MidiMsg_AcknowledgeSentData, OnAcknowledgeSentData)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void MidiMessageFilter::OnFilterAdded(IPC::Sender* sender) {
   DCHECK(io_message_loop_->BelongsToCurrentThread());
   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();
 }
 
 void MidiMessageFilter::OnChannelClosing() {
   DCHECK(io_message_loop_->BelongsToCurrentThread());
   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::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::HandleDataReceived, this,
+                 port, data, timestamp));
 }
 
-void MidiMessageFilter::RemoveClient(blink::WebMIDIAccessorClient* client) {
-  ClientsMap::iterator i = clients_.find(client);
-  if (i != clients_.end())
-    clients_.erase(i);
+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;
 }
 
-// Received from browser.
-
-void MidiMessageFilter::OnSessionStarted(
-    int client_id,
-    media::MidiResult result,
-    MidiPortInfoList inputs,
-    MidiPortInfoList outputs) {
-  // Handle on the main JS thread.
-  main_message_loop_->PostTask(
-      FROM_HERE,
-      base::Bind(&MidiMessageFilter::HandleSessionStarted, this,
-                 client_id, result, inputs, outputs));
-}
-
-void MidiMessageFilter::HandleSessionStarted(
-    int client_id,
-    media::MidiResult result,
-    MidiPortInfoList inputs,
-    MidiPortInfoList outputs) {
-  blink::WebMIDIAccessorClient* client = GetClientFromId(client_id);
-  if (!client)
-    return;
-
+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 (const auto& info : inputs_) {
       client->didAddInputPort(
-          base::UTF8ToUTF16(inputs[i].id),
-          base::UTF8ToUTF16(inputs[i].manufacturer),
-          base::UTF8ToUTF16(inputs[i].name),
-          base::UTF8ToUTF16(inputs[i].version),
+          base::UTF8ToUTF16(info.id),
+          base::UTF8ToUTF16(info.manufacturer),
+          base::UTF8ToUTF16(info.name),
+          base::UTF8ToUTF16(info.version),
           active);
     }
 
-    for (size_t i = 0; i < outputs.size(); ++i) {
+    for (const auto& info : outputs_) {
       client->didAddOutputPort(
-          base::UTF8ToUTF16(outputs[i].id),
-          base::UTF8ToUTF16(outputs[i].manufacturer),
-          base::UTF8ToUTF16(outputs[i].name),
-          base::UTF8ToUTF16(outputs[i].version),
+          base::UTF8ToUTF16(info.id),
+          base::UTF8ToUTF16(info.manufacturer),
+          base::UTF8ToUTF16(info.name),
+          base::UTF8ToUTF16(info.version),
           active);
     }
   }
+  {
+    base::AutoLock auto_lock(lock_);
+    clients_.insert(client);
+  }
   std::string error;
   std::string message;
   switch (result) {
     case media::MIDI_OK:
       break;
     case media::MIDI_NOT_SUPPORTED:
       error = "NotSupportedError";
       break;
     case media::MIDI_INITIALIZATION_ERROR:
       error = "InvalidStateError";
       message = "Platform dependent initialization failed.";
       break;
     default:
       NOTREACHED();
       error = "InvalidStateError";
       message = "Unknown internal error occurred.";
       break;
   }
   client->didStartSession(result == media::MIDI_OK, base::UTF8ToUTF16(error),
                           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) {
+  TRACE_EVENT0("midi", "MidiMessageFilter::HandleDataReceived");
+  DCHECK(main_message_loop_->BelongsToCurrentThread());
   DCHECK(!data.empty());
-  TRACE_EVENT0("midi", "MidiMessageFilter::HandleDataReceived");
 
-  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