Use MIDIMessageQueue/IsValidWebMIDIData for MIDI byte stream validation

content/browser/renderer_host/media/midi_host.cc

Index: content/browser/renderer_host/media/midi_host.cc
diff --git a/content/browser/renderer_host/media/midi_host.cc b/content/browser/renderer_host/media/midi_host.cc
index 934d5a21f1839fa97a5740fc60d28bba1936a05a..4132882a3fe9d9adf7d74345ba961ee33d1031d9 100644
--- a/content/browser/renderer_host/media/midi_host.cc
+++ b/content/browser/renderer_host/media/midi_host.cc
@@ -16,6 +16,7 @@
 #include "content/public/browser/media_observer.h"
 #include "content/public/browser/user_metrics.h"
 #include "media/midi/midi_manager.h"
+#include "media/midi/midi_message_queue.h"
 
 using media::MIDIManager;
 using media::MIDIPortInfoList;
@@ -33,6 +34,37 @@ static const size_t kAcknowledgementThresholdBytes = 1024 * 1024;  // 1 MB.
 static const uint8 kSysExMessage = 0xf0;
 
 namespace content {
+namespace {
+
+// A class to check if current child process security policy allows the the
+// renderer to use SysEx MIDI messages or not. This class fetches the policy
+// lazily and caches the result to avoid redundant access check.
+class CanSendMIDISysExMessageQuery {
+ public:
+  explicit CanSendMIDISysExMessageQuery(int renderer_process_id)
+      : renderer_process_id_(renderer_process_id),
+        value_(false),
+        initialized_(false) {
+  }
+
+  bool get() {
+    if (!initialized_) {
+      value_ = ChildProcessSecurityPolicyImpl::GetInstance()->
+          CanSendMIDISysExMessage(renderer_process_id_);
+      initialized_ = true;
+    }
+    return value_;
+  }
+
+ private:
+  const int renderer_process_id_;
+  bool value_;
+  bool initialized_;
+
+  DISALLOW_COPY_AND_ASSIGN(CanSendMIDISysExMessageQuery);
+};
+
+}  // namespace
 
 MIDIHost::MIDIHost(int renderer_process_id, media::MIDIManager* midi_manager)
     : renderer_process_id_(renderer_process_id),
@@ -75,6 +107,8 @@ void MIDIHost::OnStartSession(int client_id) {
     if (success) {
       input_ports = midi_manager_->input_ports();
       output_ports = midi_manager_->output_ports();
+      received_messages_queues_.clear();
+      received_messages_queues_.resize(input_ports.size());
     }
   }
 
@@ -94,33 +128,39 @@ void MIDIHost::OnSendData(uint32 port,
   if (data.empty())
     return;
 
-  base::AutoLock auto_lock(in_flight_lock_);
-
-  // Sanity check that we won't send too much.
-  if (sent_bytes_in_flight_ > kMaxInFlightBytes ||
-      data.size() > kMaxInFlightBytes ||
-      data.size() + sent_bytes_in_flight_ > kMaxInFlightBytes)
-    return;
-
-  if (data[0] >= kSysExMessage) {
-    // Blink running in a renderer checks permission to raise a SecurityError in
-    // JavaScript. The actual permission check for security perposes happens
-    // here in the browser process.
-    if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanSendMIDISysExMessage(
-        renderer_process_id_)) {
+  CanSendMIDISysExMessageQuery can_send_sys_ex(renderer_process_id_);

[Takashi Toyoshima, 2013/11/19 01:08:37]

Can we make it a member variable?
IIRC, this code constructs an instance on stack, whenever this method is called.
We never want to add an extra latency for non-sysex messages here.

[yukawa, 2013/11/19 09:41:41]

Hmm, I think the construction cost of |can_send_sys_ex| is regrettable. It never
does any costly tasks until get() is called (and get() will never be called for
non-sysex message).

I'm not sure the policy is really immutable. Is it OK not to check the result of
 CanSendMIDISysExMessage(renderer_process_id_) whenever MIDIHost::OnSendData is
called?  If it's OK, why we don't store the result of CanSendMIDISysExMessage at
the constructor of MIDIHost in the constructor of MIDIHost.

[Takashi Toyoshima, 2013/11/19 12:49:59]

I see. It's ok to keep this code as is.

FYI, ChildSecurityPolicy is set just before calling OnStartSession. Originally
the policy database should be immutable for a session. But actually it can be
modified if the same renderer is going to open another session. This is a
limitation of Chrome security model.

+
+  // Note: In the spec, |data| can be any number of complete MIDI messages.
+  // Running status is disallowed explicitly.
+  media::MIDIMessageQueue message_queue(false);
+  message_queue.Add(data);
+  std::vector<uint8> message;
+  while (true) {
+    message_queue.Get(&message);
+    if (message.empty())
+      break;
+
+    // Sanity check that we won't send too large message.

[Takashi Toyoshima, 2013/11/19 01:08:37]

Here, we concern the buffer size which may be allocated by MIDIManager.
So, in_flight_lock_ + message.size() > kMaxInFlightBytes is right check as
original code did.

[yukawa, 2013/11/19 09:41:41]

Done.

+    if (message.size() > kMaxInFlightBytes)

[Takashi Toyoshima, 2013/11/19 01:08:37]

This check looks redundant.

[yukawa, 2013/11/19 09:41:41]

I put this condition here as an early exit route because this condition does not
require |in_flight_lock_|.  Anyway, I'll adopt your suggestion.

+      continue;
+
+    if (message[0] == kSysExMessage && !can_send_sys_ex.get()) {
+      // Blink running in a renderer checks permission to raise a SecurityError
+      // in JavaScript. The actual permission check for security purposes
+      // happens here in the browser process.
       RecordAction(UserMetricsAction("BadMessageTerminate_MIDI"));
       BadMessageReceived();
       return;
     }
-  }
-
-  midi_manager_->DispatchSendMIDIData(
-      this,
-      port,
-      data,
-      timestamp);
 
-  sent_bytes_in_flight_ += data.size();
+    base::AutoLock auto_lock(in_flight_lock_);
+    // Sanity check that we won't send too much data.
+    if (sent_bytes_in_flight_ > kMaxInFlightBytes ||
+        message.size() + sent_bytes_in_flight_ > kMaxInFlightBytes)

[Takashi Toyoshima, 2013/11/19 01:08:37]

Same TODO here, and any reason to check twice?
I feel the latter check meets our expectation here, and former check looks
redundant.

[yukawa, 2013/11/19 09:41:41]

Done.
BTW, the redundancy came from the original code... 

[Takashi Toyoshima, 2013/11/19 12:49:59]

lol

+      return;
+    midi_manager_->DispatchSendMIDIData(this, port, message, timestamp);
+    sent_bytes_in_flight_ += message.size();
+  }
 }
 
 void MIDIHost::ReceiveMIDIData(
@@ -130,20 +170,31 @@ void MIDIHost::ReceiveMIDIData(
     double timestamp) {
   TRACE_EVENT0("midi", "MIDIHost::ReceiveMIDIData");
 
-  // Check a process security policy to receive a system exclusive message.
-  if (length > 0 && data[0] >= kSysExMessage) {
-    if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanSendMIDISysExMessage(
-        renderer_process_id_)) {
-      // MIDI devices may send a system exclusive messages even if the renderer
-      // doesn't have a permission to receive it. Don't kill the renderer as
-      // OnSendData() does.
-      return;
-    }
-  }
+  if (received_messages_queues_.size() <= port)
+    return;
+
+  // Lazy initialization
+  if (received_messages_queues_[port] == NULL)
+    received_messages_queues_[port] = new media::MIDIMessageQueue(true);
+
+  CanSendMIDISysExMessageQuery can_send_sys_ex(renderer_process_id_);

[Takashi Toyoshima, 2013/11/19 01:08:37]

ditto. I think we can share the same instance between sender side and receiver
side.

[yukawa, 2013/11/19 09:41:41]

See my previous comment.

[Takashi Toyoshima, 2013/11/19 12:49:59]

ok

 
-  // Send to the renderer.
-  std::vector<uint8> v(data, data + length);
-  Send(new MIDIMsg_DataReceived(port, v, timestamp));
+  received_messages_queues_[port]->Add(data, length);
+  std::vector<uint8> message;
+  while (true) {
+    received_messages_queues_[port]->Get(&message);
+    if (message.empty())
+      break;
+
+    // MIDI devices may send a system exclusive messages even if the renderer
+    // doesn't have a permission to receive it. Don't kill the renderer as
+    // OnSendData() does.
+    if (message[0] == kSysExMessage && !can_send_sys_ex.get())
+      continue;
+
+    // Send to the renderer.
+    Send(new MIDIMsg_DataReceived(port, message, timestamp));
+  }
 }
 
 void MIDIHost::AccumulateMIDIBytesSent(size_t n) {