Web MIDI: fix multi-threading problem around message buffer handling

Web MIDI: fix multi-threading problem around message buffer handling

Passing message buffer to another thread as const uint8* doesn't work
since it might be invalid before another thread reads it.
Keep on handling it as vector<uint8> from IPC to passing it to the OS.

TEST=manual
BUG=276810
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219796

[Takashi Toyoshima, 2013-08-21 13:48:22]

Hi, can you take a look?

This bug caused message drop problem.

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager.cc
File media/midi/midi_manager.cc (left):

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager.cc...
media/midi/midi_manager.cc:80: client, port_index, data, length, timestamp));
raw buffer could not be passed to another thread correctly.

[scherkus (not reviewing), 2013-08-21 17:46:29]

palmer: can you sanity check my understanding of the types rules?

https://codereview.chromium.org/23379002/diff/3001/content/common/media/midi_...
File content/common/media/midi_messages.h (right):

https://codereview.chromium.org/23379002/diff/3001/content/common/media/midi_...
content/common/media/midi_messages.h:46: unsigned int /* port */,
I believe this should be uint32

http://dev.chromium.org/developers/coding-style#TOC-Types

https://codereview.chromium.org/23379002/diff/3001/content/common/media/midi_...
content/common/media/midi_messages.h:64: size_t /* bytes sent */)
shouldn't this also be an explicitly sized type?

I thought size_t was to be avoided in IPC messages

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager.cc
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager.cc...
media/midi/midi_manager.cc:84: client, port_index, data, timestamp));
do you know how large |data| can typically be?

are we talking bytes, kilobytes, megabytes?

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_ma...
File media/midi/midi_manager_mac.cc (right):

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_ma...
media/midi/midi_manager_mac.cc:167: if (static_cast<size_t>(port_index) >=
destinations_.size())
if you make port_index size_t, you can remove the cast

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_mac.h
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_ma...
media/midi/midi_manager_mac.h:28: unsigned int port_index,
nit: while unsigned is prevalent in blink, this should be size_t

[Takashi Toyoshima, 2013-08-22 05:47:45]

https://codereview.chromium.org/23379002/diff/3001/content/common/media/midi_...
File content/common/media/midi_messages.h (right):

https://codereview.chromium.org/23379002/diff/3001/content/common/media/midi_...
content/common/media/midi_messages.h:46: unsigned int /* port */,
On 2013/08/21 17:46:29, scherkus wrote:
> I believe this should be uint32
> 
> http://dev.chromium.org/developers/coding-style#TOC-Types

Done.

https://codereview.chromium.org/23379002/diff/3001/content/common/media/midi_...
content/common/media/midi_messages.h:64: size_t /* bytes sent */)
I see. A lot of messages which handles size seem to use uint32 instead of
size_t.
I'll use uint32 here.

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager.cc
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager.cc...
media/midi/midi_manager.cc:84: client, port_index, data, timestamp));
Typically, it's just 3 Bytes.

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_ma...
File media/midi/midi_manager_mac.cc (right):

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_ma...
media/midi/midi_manager_mac.cc:167: if (static_cast<size_t>(port_index) >=
destinations_.size())
On 2013/08/21 17:46:29, scherkus wrote:
> if you make port_index size_t, you can remove the cast

Done.

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_mac.h
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_ma...
media/midi/midi_manager_mac.h:28: unsigned int port_index,
On 2013/08/21 17:46:29, scherkus wrote:
> nit: while unsigned is prevalent in blink, this should be size_t

Done.

[palmer, 2013-08-22 18:25:40]

https://codereview.chromium.org/23379002/diff/3001/content/common/media/midi_...
File content/common/media/midi_messages.h (right):

https://codereview.chromium.org/23379002/diff/3001/content/common/media/midi_...
content/common/media/midi_messages.h:64: size_t /* bytes sent */)
On 2013/08/22 05:47:45, Takashi Toyoshima (chromium) wrote:
> I see. A lot of messages which handles size seem to use uint32 instead of
> size_t.
> I'll use uint32 here.

Yes, scherkus is correct that size_t, like all relatively-sized int types, is
not good in IPC.

The reason is that, although size_t is the correct type for sizes, IPC can span
processes built with different processor models (e.g. a 64-bit browser, where
size_t is 64 bits, talking to a 32-bit plugin, where size_t is 32 bits).

Use an unsigned, explicitly-sized type that is wide enough to handle the range
of values you expect. And if you find yourself casting it to size_t, which is
likely, make sure that the value does not get truncated. E.g. a uint64_t will
get truncated when cast to size_t in a 32-bit process. (But conversely, a
uint32_t will cast up to a 64-bit process' size_t just fine.)

So, yeah, uint32_t will work. :)

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_mac.h
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/23379002/diff/3001/media/midi/midi_manager_ma...
media/midi/midi_manager_mac.h:28: unsigned int port_index,
On 2013/08/21 17:46:29, scherkus wrote:
> nit: while unsigned is prevalent in blink, this should be size_t

There is a fun thing here. It can often make sense, as a space optimization, to
implement a size_t interface with an unsigned int type that is not as wide as a
size_t. E.g., on a 64-bit machine:

// We create tons of these, and they don't
// need to have more than 2 ** 32 - 1 bytes,
// so save 4 bytes by using uint32_t. (On
// 64-bit, size_t would be 8 bytes.) Plus,
// very short strings, which are common,
// now fit into a single 64-bit word! Yay.
//
// sizeof(String) will be 8 on most systems.
typedef struct String {
    uint32_t count;
    uint8_t bytes[1];
} String;

// Although we have taken a short-cut in the
// implementation, expose the correct interface
// in case we need to go big someday.
String* newString(uint8_t* bytes, size_t count) {
    // Check that we are not caught in our lie!
    // We do this to be safe, even while
    // presenting a not-quite-true but future-proof
    // interface.
    assert(count < UINT32_T_MAX);

    // ...
}

That general idea is behind a lot of Blink/WebKit code. You could argue that the
interfaces should exactly match their implementations --- newString(uint8_t*
bytes, uint32_t count) --- but then you'd be making all the callers do the
safety checks (callers are likely to use size_t, as they should), and the
interface would not be able to transparently grow someday in the future.

[palmer, 2013-08-22 18:31:26]

https://codereview.chromium.org/23379002/diff/16001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/23379002/diff/16001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:190: void
MIDIMessageFilter::SendMIDIData(unsigned int port,
Yeah, harmonize these types. What is the actual range of values for ports? I
assume that is determined in the MIDI standard?

https://codereview.chromium.org/23379002/diff/16001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/23379002/diff/16001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.h:40: void SendMIDIData(unsigned int
port,
You're using size_t elsewhere.

https://codereview.chromium.org/23379002/diff/16001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/23379002/diff/16001/media/midi/midi_manager.h...
media/midi/midi_manager.h:103: // TODO(toyoshim): Make |port_index| size_t to be
consistent with send side.
Or whatever type the MIDI specification calls for. Might even be defined to be 7
or 8 bits?

[Takashi Toyoshima, 2013-08-23 02:39:15]

https://codereview.chromium.org/23379002/diff/16001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/23379002/diff/16001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:190: void
MIDIMessageFilter::SendMIDIData(unsigned int port,
This parameter doesn't depend on MIDI spec. Because this is the number of
devices which is connected to the operating system. In the real situation,
uint16 may be enough. But, size_t is good to be used for the number the range is
ambiguous.

https://codereview.chromium.org/23379002/diff/16001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/23379002/diff/16001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.h:40: void SendMIDIData(unsigned int
port,
Sorry, I missed to revise this in the second upload.

https://codereview.chromium.org/23379002/diff/16001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/23379002/diff/16001/media/midi/midi_manager.h...
media/midi/midi_manager.h:103: // TODO(toyoshim): Make |port_index| size_t to be
consistent with send side.
ditto.

[scherkus (not reviewing), 2013-08-23 19:15:05]

lgtm w/ nit

deferring to palmer@ on matters of size_t / uint32 ... other than addressing the
TODO this looks fine to me (e.g., casting uint32 -> size_t as soon as possible
in the IPC handler)

https://codereview.chromium.org/23379002/diff/18001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/23379002/diff/18001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:114: return;
nit: this should be 2 space indent, not 4

[palmer, 2013-08-23 21:04:08]

I think you'll be able to achieve a safe solution more cleanly by just deciding
now that MIDI ports are uint32_t or uint16_t. Those types will safely cast up to
size_t when necessary, and you don't have to manage the casts yourself.

I suppose I should change my advice to something like, "Counts, sizes, and
indices must be size_t or an unsigned type that safely casts up to size_t."

https://codereview.chromium.org/23379002/diff/18001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/23379002/diff/18001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:221: Send(new
MIDIHostMsg_SendData(port, data, timestamp));
You'll want to cast |port| to uint32_t here and, in theory, check for truncation
in the case of more than 4 billion MIDI devices. :) (Mostly likely faked by an
attacker than real devices plugged in, obviously)

Or, alternately and more cleanly, just use uint32_t everywhere and avoid the
need to cast. On the platforms Chrome runs on (i.e., no 16-bit machines :) ),
uint32_t will always cast safely (and implicitly) to size_t when used in offsets
and stuff. This is what I was getting at yesterday when asking what the MIDI
standard specifies for the number of ports/devices. As you say, uint16_t would
probably suffice.

[scherkus (not reviewing), 2013-08-23 21:06:13]

On 2013/08/23 21:04:08, Chromium Palmer wrote:
> I think you'll be able to achieve a safe solution more cleanly by just
deciding
> now that MIDI ports are uint32_t or uint16_t. Those types will safely cast up
to
> size_t when necessary, and you don't have to manage the casts yourself.
> 
> I suppose I should change my advice to something like, "Counts, sizes, and
> indices must be size_t or an unsigned type that safely casts up to size_t."
> 
>
https://codereview.chromium.org/23379002/diff/18001/content/renderer/media/mi...
> File content/renderer/media/midi_message_filter.cc (right):
> 
>
https://codereview.chromium.org/23379002/diff/18001/content/renderer/media/mi...
> content/renderer/media/midi_message_filter.cc:221: Send(new
> MIDIHostMsg_SendData(port, data, timestamp));
> You'll want to cast |port| to uint32_t here and, in theory, check for
truncation
> in the case of more than 4 billion MIDI devices. :) (Mostly likely faked by an
> attacker than real devices plugged in, obviously)
> 
> Or, alternately and more cleanly, just use uint32_t everywhere and avoid the
> need to cast. On the platforms Chrome runs on (i.e., no 16-bit machines :) ),
> uint32_t will always cast safely (and implicitly) to size_t when used in
offsets
> and stuff. This is what I was getting at yesterday when asking what the MIDI
> standard specifies for the number of ports/devices. As you say, uint16_t would
> probably suffice.

(to avoid any confusion and unnecessary back-and-forth... palmer@ when you say
uint32_t / uint16_t, do you intend on having toyoshim@ use Chromium's uint32 /
uint16 typedefs?)

[palmer, 2013-08-23 21:07:10]

By the way, this CL will probably L G to M very soon. I feel the pain of time
zones; I created a 10 AM Pacific calendar event to remind myself to check your
CL first thing Monday morning (after the end of your Monday work day).

[Takashi Toyoshima, 2013-08-26 08:31:42]

https://codereview.chromium.org/23379002/diff/18001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/23379002/diff/18001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:114: return;
On 2013/08/23 19:15:06, scherkus wrote:
> nit: this should be 2 space indent, not 4

Done.

https://codereview.chromium.org/23379002/diff/18001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/23379002/diff/18001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:221: Send(new
MIDIHostMsg_SendData(port, data, timestamp));
Applied uint32 cast here.

By the way, we used port index here, but in the future, we should support
dynamic device adding/removing to confirm the spec. There, to use index to
identify a port is dangerous because device list update happens asynchronously
between browser process and JavaScript of renderer. The order will be changed.

Now, using uint32 sounds the best solution, but I'll fix this later to support
dynamic device configuration. I will use uint32 to identify each device. But it
must not be an index, but hash like something. Browser process will resolve the
hash and map it to the actual port index in a way which asynchronous operations
don't affect.

[palmer, 2013-08-26 17:43:24]

LGTM after you complete the change to uint32_t.

https://codereview.chromium.org/23379002/diff/45001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/23379002/diff/45001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:118: static_cast<size_t>(port),
You don't need to do this if you define |SendMIDIData| and
|SendMIDIDataOnIOThread| in midi_message_filter.{h,cc} to take a uint32_t.

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:187: void
MIDIMessageFilter::SendMIDIData(size_t port,
Declare this to be uint32_t, and on line 203 also.

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:218: Send(new
MIDIHostMsg_SendData(static_cast<uint32>(port), data, timestamp));
The need for this cast goes away once you declare |port| as uint32_t.

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.h:39: void SendMIDIData(size_t port,
Define this to be uint32_t.

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.h:91: void
SendMIDIDataOnIOThread(size_t port,
Define this to be uint32_t.

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager.cc
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager.c...
media/midi/midi_manager.cc:71: size_t port_index,
uint32_t

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager.h...
media/midi/midi_manager.h:103: // TODO(toyoshim): Make |port_index| size_t to be
consistent with send side.
uint32_t. And, why not do it now?

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager_m...
File media/midi/midi_manager_mac.cc (right):

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager_m...
media/midi/midi_manager_mac.cc:150: size_t port_index,
uint32_t

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager_m...
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager_m...
media/midi/midi_manager_mac.h:28: size_t port_index,
uint32_t

[scherkus (not reviewing), 2013-08-26 18:07:41]

On 2013/08/26 17:43:24, Chromium Palmer wrote:
> LGTM after you complete the change to uint32_t.

palmer: why the _t suffixed version versus the uint8/16/32 typedefs from
base/basictypes.h?

[palmer, 2013-08-26 18:30:50]

> palmer: why the _t suffixed version versus the uint8/16/32 typedefs from
> base/basictypes.h?

Oh, I typically say the _t names because they are standard ANSI C/C++ names. The
basictypes.h ones are better for us, because we have to work with
not-yet-ANSI-compliant systems like older versions of MSVC++ that don't come
with stdint.h. So yeah use the typedefs from basictypes.h.

[Takashi Toyoshima, 2013-08-27 07:14:32]

Thanks

https://codereview.chromium.org/23379002/diff/45001/content/browser/renderer_...
File content/browser/renderer_host/media/midi_host.cc (right):

https://codereview.chromium.org/23379002/diff/45001/content/browser/renderer_...
content/browser/renderer_host/media/midi_host.cc:118: static_cast<size_t>(port),
On 2013/08/26 17:43:25, Chromium Palmer wrote:
> You don't need to do this if you define |SendMIDIData| and
> |SendMIDIDataOnIOThread| in midi_message_filter.{h,cc} to take a uint32_t.

Done.

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:187: void
MIDIMessageFilter::SendMIDIData(size_t port,
On 2013/08/26 17:43:25, Chromium Palmer wrote:
> Declare this to be uint32_t, and on line 203 also.

Done.

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.cc:218: Send(new
MIDIHostMsg_SendData(static_cast<uint32>(port), data, timestamp));
On 2013/08/26 17:43:25, Chromium Palmer wrote:
> The need for this cast goes away once you declare |port| as uint32_t.

Done.

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.h:39: void SendMIDIData(size_t port,
On 2013/08/26 17:43:25, Chromium Palmer wrote:
> Define this to be uint32_t.

Done.

https://codereview.chromium.org/23379002/diff/45001/content/renderer/media/mi...
content/renderer/media/midi_message_filter.h:91: void
SendMIDIDataOnIOThread(size_t port,
On 2013/08/26 17:43:25, Chromium Palmer wrote:
> Define this to be uint32_t.

Done.

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager.cc
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager.c...
media/midi/midi_manager.cc:71: size_t port_index,
On 2013/08/26 17:43:25, Chromium Palmer wrote:
> uint32_t

Done.

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager.h
File media/midi/midi_manager.h (right):

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager.h...
media/midi/midi_manager.h:103: // TODO(toyoshim): Make |port_index| size_t to be
consistent with send side.
Maybe I should split all type related changes to another CL since this original
change focus on a use after free bug in browser process. Also receiving side has
another nits I want to fix them together. Anyway I'll fix them too in this CL.

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager_m...
File media/midi/midi_manager_mac.cc (right):

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager_m...
media/midi/midi_manager_mac.cc:150: size_t port_index,
On 2013/08/26 17:43:25, Chromium Palmer wrote:
> uint32_t

Done.

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager_m...
File media/midi/midi_manager_mac.h (right):

https://codereview.chromium.org/23379002/diff/45001/media/midi/midi_manager_m...
media/midi/midi_manager_mac.h:28: size_t port_index,
On 2013/08/26 17:43:25, Chromium Palmer wrote:
> uint32_t

Done.

[commit-bot: I haz the power, 2013-08-27 07:23:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/23379002/62001

[commit-bot: I haz the power, 2013-08-27 15:30:27]

Change committed as 219796