Skip to content

Commit

Permalink
Revert 237558 "Use MIDIMessageQueue/IsValidWebMIDIData for MIDI ..."
Browse files Browse the repository at this point in the history 
Seems to have caused issues running perf tests.

BUG=324160

> Use MIDIMessageQueue/IsValidWebMIDIData for MIDI byte stream validation
> 
> WebMIDI spec draft: http://www.w3.org/TR/webmidi/ 
> 
> WebMIDI API guarantees that MIDIInput::onmessage is called back with a single MIDI message. To guarantee this, this CL introduces MIDIMessageQueue class, which allows you to
> - maintain fragmented MIDI message.
> - Skip any invalid data sequence.
> - Reorder MIDI messages so that "System Real Time Message", which can be inserted at any point of the byte stream, can be placed at the boundary of complete MIDI messages.
> - (Optional) Reconstruct complete MIDI messages from data stream that is compressed with "running status".
> 
> This CL also replaces existing System Exclusive message validation logic in MIDIHost::OnSendData with MIDIHost::IsValidWebMIDIData, which can detect SysEx message even when it is concatenated with non-SysEx messages.
> 
> With this change, renderer/blink can be much simpler and free from this kind of data validation.
> 
> BUG=303599, 317355
> TEST=media_unittests --gtest_filter=MIDI*, content_unittests --gtest_filter=MIDI*
> 
> Review URL: https://codereview.chromium.org/68353002

TBR=yukawa@chromium.org

Review URL: https://codereview.chromium.org/93583002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@237660 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
isherman@chromium.org committed Nov 28, 2013
1 parent 10c6f09 commit aac2360
Show file tree
Hide file tree
Showing 11 changed files with 44 additions and 672 deletions.
135 changes: 39 additions & 96 deletions content/browser/renderer_host/media/midi_host.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,41 +16,26 @@
#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"
#include "media/midi/midi_message_util.h"

using media::MIDIManager;
using media::MIDIPortInfoList;

namespace content {
namespace {

// The total number of bytes which we're allowed to send to the OS
// before knowing that they have been successfully sent.
const size_t kMaxInFlightBytes = 10 * 1024 * 1024; // 10 MB.
static const size_t kMaxInFlightBytes = 10 * 1024 * 1024; // 10 MB.

// We keep track of the number of bytes successfully sent to
// the hardware. Every once in a while we report back to the renderer
// the number of bytes sent since the last report. This threshold determines
// how many bytes will be sent before reporting back to the renderer.
const size_t kAcknowledgementThresholdBytes = 1024 * 1024; // 1 MB.

const uint8 kSysExMessage = 0xf0;
const uint8 kEndOfSysExMessage = 0xf7;
static const size_t kAcknowledgementThresholdBytes = 1024 * 1024; // 1 MB.

bool IsDataByte(uint8 data) {
return (data & 0x80) == 0;
}

bool IsSystemRealTimeMessage(uint8 data) {
return 0xf8 <= data && data <= 0xff;
}
static const uint8 kSysExMessage = 0xf0;

} // namespace
namespace content {

MIDIHost::MIDIHost(int renderer_process_id, media::MIDIManager* midi_manager)
: renderer_process_id_(renderer_process_id),
has_sys_ex_permission_(false),
midi_manager_(midi_manager),
sent_bytes_in_flight_(0),
bytes_sent_since_last_acknowledgement_(0) {
Expand Down Expand Up @@ -90,12 +75,6 @@ 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());
// ChildSecurityPolicy is set just before OnStartSession by
// MIDIDispatcherHost. So we can safely cache the policy.
has_sys_ex_permission_ = ChildProcessSecurityPolicyImpl::GetInstance()->
CanSendMIDISysExMessage(renderer_process_id_);
}
}

Expand All @@ -115,26 +94,32 @@ void MIDIHost::OnSendData(uint32 port,
if (data.empty())
return;

// 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.
if (!has_sys_ex_permission_ &&
(std::find(data.begin(), data.end(), kSysExMessage) != data.end())) {
RecordAction(UserMetricsAction("BadMessageTerminate_MIDI"));
BadMessageReceived();
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_)) {
RecordAction(UserMetricsAction("BadMessageTerminate_MIDI"));
BadMessageReceived();
return;
}
}

if (!IsValidWebMIDIData(data))
return;
midi_manager_->DispatchSendMIDIData(
this,
port,
data,
timestamp);

base::AutoLock auto_lock(in_flight_lock_);
// Sanity check that we won't send too much data.
// TODO(yukawa): Consider to send an error event back to the renderer
// after some future discussion in W3C.
if (data.size() + sent_bytes_in_flight_ > kMaxInFlightBytes)
return;
midi_manager_->DispatchSendMIDIData(this, port, data, timestamp);
sent_bytes_in_flight_ += data.size();
}

Expand All @@ -145,29 +130,20 @@ void MIDIHost::ReceiveMIDIData(
double timestamp) {
TRACE_EVENT0("midi", "MIDIHost::ReceiveMIDIData");

if (received_messages_queues_.size() <= port)
return;

// Lazy initialization
if (received_messages_queues_[port] == NULL)
received_messages_queues_[port] = new media::MIDIMessageQueue(true);

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 && !has_sys_ex_permission_)
continue;

// Send to the renderer.
Send(new MIDIMsg_DataReceived(port, message, timestamp));
// 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;
}
}

// Send to the renderer.
std::vector<uint8> v(data, data + length);
Send(new MIDIMsg_DataReceived(port, v, timestamp));
}

void MIDIHost::AccumulateMIDIBytesSent(size_t n) {
Expand All @@ -189,37 +165,4 @@ void MIDIHost::AccumulateMIDIBytesSent(size_t n) {
}
}

// static
bool MIDIHost::IsValidWebMIDIData(const std::vector<uint8>& data) {
bool in_sysex = false;
size_t waiting_data_length = 0;
for (size_t i = 0; i < data.size(); ++i) {
const uint8 current = data[i];
if (IsSystemRealTimeMessage(current))
continue; // Real time message can be placed at any point.
if (waiting_data_length > 0) {
if (!IsDataByte(current))
return false; // Error: |current| should have been data byte.
--waiting_data_length;
continue; // Found data byte as expected.
}
if (in_sysex) {
if (data[i] == kEndOfSysExMessage)
in_sysex = false;
else if (!IsDataByte(current))
return false; // Error: |current| should have been data byte.
continue; // Found data byte as expected.
}
if (current == kSysExMessage) {
in_sysex = true;
continue; // Found SysEX
}
waiting_data_length = media::GetMIDIMessageLength(current);
if (waiting_data_length == 0)
return false; // Error: |current| should have been a valid status byte.
--waiting_data_length; // Found status byte
}
return waiting_data_length == 0 && !in_sysex;
}

} // namespace content
27 changes: 5 additions & 22 deletions content/browser/renderer_host/media/midi_host.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,15 @@

#include <vector>

#include "base/gtest_prod_util.h"
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
#include "base/memory/scoped_vector.h"
#include "content/common/content_export.h"
#include "content/public/browser/browser_message_filter.h"
#include "content/public/browser/browser_thread.h"
#include "media/midi/midi_manager.h"

namespace media {
class MIDIManager;
class MIDIMessageQueue;
}

namespace content {
Expand All @@ -36,10 +33,11 @@ class CONTENT_EXPORT MIDIHost
bool* message_was_ok) OVERRIDE;

// MIDIManagerClient implementation.
virtual void ReceiveMIDIData(uint32 port,
const uint8* data,
size_t length,
double timestamp) OVERRIDE;
virtual void ReceiveMIDIData(
uint32 port,
const uint8* data,
size_t length,
double timestamp) OVERRIDE;
virtual void AccumulateMIDIBytesSent(size_t n) OVERRIDE;

// Start session to access MIDI hardware.
Expand All @@ -51,35 +49,20 @@ class CONTENT_EXPORT MIDIHost
double timestamp);

private:
FRIEND_TEST_ALL_PREFIXES(MIDIHostTest, IsValidWebMIDIData);
friend class base::DeleteHelper<MIDIHost>;
friend class BrowserThread;

virtual ~MIDIHost();

// Returns true if |data| fulfills the requirements of MIDIOutput.send API
// defined in the WebMIDI spec.
// - |data| must be any number of complete MIDI messages (data abbreviation
// called "running status" is disallowed).
// - 1-byte MIDI realtime messages can be placed at any position of |data|.
static bool IsValidWebMIDIData(const std::vector<uint8>& data);

int renderer_process_id_;

// Represents if the renderer has a permission to send/receive MIDI SysEX
// messages.
bool has_sys_ex_permission_;

// |midi_manager_| talks to the platform-specific MIDI APIs.
// It can be NULL if the platform (or our current implementation)
// does not support MIDI. If not supported then a call to
// OnRequestAccess() will always refuse access and a call to
// OnSendData() will do nothing.
media::MIDIManager* const midi_manager_;

// Buffers where data sent from each MIDI input port is stored.
ScopedVector<media::MIDIMessageQueue> received_messages_queues_;

// The number of bytes sent to the platform-specific MIDI sending
// system, but not yet completed.
size_t sent_bytes_in_flight_;
Expand Down
90 changes: 0 additions & 90 deletions content/browser/renderer_host/media/midi_host_unittest.cc
View file

This file was deleted.

1 change: 0 additions & 1 deletion content/content_tests.gypi
View file
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,6 @@
'browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc',
'browser/renderer_host/media/media_stream_manager_unittest.cc',
'browser/renderer_host/media/media_stream_ui_proxy_unittest.cc',
'browser/renderer_host/media/midi_host_unittest.cc',
'browser/renderer_host/media/video_capture_buffer_pool_unittest.cc',
'browser/renderer_host/media/video_capture_controller_unittest.cc',
'browser/renderer_host/media/video_capture_host_unittest.cc',
Expand Down
6 changes: 0 additions & 6 deletions media/media.gyp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,10 +400,6 @@
'midi/midi_manager.h',
'midi/midi_manager_mac.cc',
'midi/midi_manager_mac.h',
'midi/midi_message_queue.cc',
'midi/midi_message_queue.h',
'midi/midi_message_util.cc',
'midi/midi_message_util.h',
'midi/midi_port_info.cc',
'midi/midi_port_info.h',
'video/capture/android/video_capture_device_android.cc',
Expand Down Expand Up @@ -984,8 +980,6 @@
'filters/video_decoder_selector_unittest.cc',
'filters/video_frame_stream_unittest.cc',
'filters/video_renderer_impl_unittest.cc',
'midi/midi_message_queue_unittest.cc',
'midi/midi_message_util_unittest.cc',
'video/capture/video_capture_device_unittest.cc',
'webm/cluster_builder.cc',
'webm/cluster_builder.h',
Expand Down
Loading

0 comments on commit aac2360

Please sign in to comment.