Manage MIDI related objects and sessions' lifecycles correctly

Manage MIDI related objects and sessions' lifecycles correctly

Following changes are needed to maintain MIDIPortInfoList contents
consistent in each MidiMessageFilter instances when MidiManager
update the list asynchronously.

Essential changes:
- Call EndSession iff a session is open.
- Do not use client_id to communicate between browser and renderer.
  Multiple clients are managed in a MidiMessageFilter existing in
  each renderer.

Minor changes:
- Remove midi_manager_ checks in MIDIHost, and add CHECK insteads.
- Rename MidiManagerFilter::StartSession to MidiManagerFilter::AddClient
  to be aligned with RemoveClient. A session can be shared with multiple
  clients.

After this change, MidiManager::EndSession is called from each Renderer
and RendererHost at the right timing, e.g., GC collects the last MIDIAccess
reference in JavaScript. See, also http://crbug.com/424859.

BUG=424859
Committed: https://crrev.com/71fcb150d9e2cb5e2f05069177ac611ac9c189c0
Cr-Commit-Position: refs/heads/master@{#300851}

[Takashi Toyoshima, 2014-10-18 12:29:12]

toyoshim@chromium.org changed reviewers:
+ yhirano@chromium.org

[Takashi Toyoshima, 2014-10-18 12:35:42]

PTAL

[yhirano, 2014-10-20 12:15:15]

"between browser and render" in the description should be "between browser and
renderer"?

https://codereview.chromium.org/662853003/diff/40001/content/browser/media/mi...
File content/browser/media/midi_host.cc (right):

https://codereview.chromium.org/662853003/diff/40001/content/browser/media/mi...
content/browser/media/midi_host.cc:51: MidiHost::MidiHost(int
renderer_process_id, media::MidiManager* midi_manager)
Please initialize |is_session_requested_|.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:39: if (session_result_ !=
media::MIDI_NOT_INITIALIZED) {
Is it OK to behave as if there is no error when session_result != MIDI_OK?

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:53: ClientsSet::iterator set_it =
clients_.find(client);
Isn't |client_.erase(client)| enough?

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:56: for (ClientsQueue::iterator
queue_it = clients_waiting_session_queue_.begin();
How about using std::find?

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:164: for (ClientsQueue::iterator
it = clients_waiting_session_queue_.begin();
You can use the range-based for.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:198: if (result == media::MIDI_OK)
{
You can bring this block out of auto_lock's scope.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:200: for (size_t i = 0; i <
inputs_.size(); ++i) {
You can use the range-based for.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:208: for (size_t i = 0; i <
outputs_.size(); ++i) {
ditto

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:247: for (ClientsSet::iterator it
= clients_.begin(); it != clients_.end(); ++it)
You can use the range-based for.

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

https://codereview.chromium.org/662853003/diff/40001/media/midi/midi_manager....
media/midi/midi_manager.cc:36: session_is_ready = initialized_;
DCHECK(clients_.find(client) == clients_.end());

If you don't want to trust the renderer,

if (clients_.find(client) != clients_.end()) return;

is also fine.

ditto for pending_clients_

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

https://codereview.chromium.org/662853003/diff/40001/media/midi/midi_manager....
media/midi/midi_manager.cc:124: for (ClientSet::iterator it = clients_.begin();
it != clients_.end(); ++it)
You can use the range-based for.

https://codereview.chromium.org/662853003/diff/40001/media/midi/midi_manager....
media/midi/midi_manager.cc:137: for (ClientSet::iterator it =
pending_clients_.begin();
ditto

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

https://codereview.chromium.org/662853003/diff/40001/content/browser/media/mi...
File content/browser/media/midi_host.cc (right):

https://codereview.chromium.org/662853003/diff/40001/content/browser/media/mi...
content/browser/media/midi_host.cc:51: MidiHost::MidiHost(int
renderer_process_id, media::MidiManager* midi_manager)
Oh, thanks!
But, I'm surprised since there are no compile errors and no try failures on
this.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:39: if (session_result_ !=
media::MIDI_NOT_INITIALIZED) {
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.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:53: ClientsSet::iterator set_it =
clients_.find(client);
On 2014/10/20 12:15:14, yhirano wrote:
> Isn't |client_.erase(client)| enough?

Done.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:56: for (ClientsQueue::iterator
queue_it = clients_waiting_session_queue_.begin();
On 2014/10/20 12:15:14, yhirano wrote:
> How about using std::find?

Done.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:164: for (ClientsQueue::iterator
it = clients_waiting_session_queue_.begin();
On 2014/10/20 12:15:14, yhirano wrote:
> You can use the range-based for.

Done.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:198: if (result == media::MIDI_OK)
{
On 2014/10/20 12:15:14, yhirano wrote:
> You can bring this block out of auto_lock's scope.

Done.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:200: for (size_t i = 0; i <
inputs_.size(); ++i) {
On 2014/10/20 12:15:14, yhirano wrote:
> You can use the range-based for.

Done.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:208: for (size_t i = 0; i <
outputs_.size(); ++i) {
On 2014/10/20 12:15:14, yhirano wrote:
> ditto

Done.

https://codereview.chromium.org/662853003/diff/40001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:247: for (ClientsSet::iterator it
= clients_.begin(); it != clients_.end(); ++it)
On 2014/10/20 12:15:14, yhirano wrote:
> You can use the range-based for.

Done.

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

https://codereview.chromium.org/662853003/diff/40001/media/midi/midi_manager....
media/midi/midi_manager.cc:36: session_is_ready = initialized_;
Nice.
I prefer NOTREACHED() and return.

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

https://codereview.chromium.org/662853003/diff/40001/media/midi/midi_manager....
media/midi/midi_manager.cc:124: for (ClientSet::iterator it = clients_.begin();
it != clients_.end(); ++it)
On 2014/10/20 12:15:15, yhirano wrote:
> You can use the range-based for.

Done.

https://codereview.chromium.org/662853003/diff/40001/media/midi/midi_manager....
media/midi/midi_manager.cc:137: for (ClientSet::iterator it =
pending_clients_.begin();
On 2014/10/20 12:15:15, yhirano wrote:
> ditto

Done.

[Takashi Toyoshima, 2014-10-20 17:18:04]

toyoshim@chromium.org changed reviewers:
+ palmer@chromium.org, scherkus@chromium.org

[Takashi Toyoshima, 2014-10-20 17:18:05]

palmer@chromium.org: Please review changes in midi_messages.h (IPC)

scherkus@chromium.org: Please review changes in media/

[scherkus (not reviewing), 2014-10-20 18:05:48]

few questions

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:68: void
MidiMessageFilter::SendMidiData(uint32 port,
which thread is this typically called on?

it seems like most of your thread interactions are between IO and main, which
would make me think you could save yourself a lot of trouble by using a
RenderFrameObserver (which handles receiving/sending messages between IO and
main) vs. a MessageFilter

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:199: for (media::MidiPortInfo info
: inputs_) {
won't this create a copy?

you could use const auto&

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:208: for (media::MidiPortInfo info
: outputs_) {
ditto

[palmer, 2014-10-20 18:25:49]

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
content/renderer/media/midi_message_filter.h:56: double timestamp);
double? Not e.g. int64 of microseconds or nanoseconds? or base::Time?

[Takashi Toyoshima, 2014-10-20 18:37:57]

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
content/renderer/media/midi_message_filter.h:56: double timestamp);
This is not changed. Just replaced from line 93. We use double since this is
JavaScript exposing value and the type is defined in Web MIDI API spec;
http://webaudio.github.io/web-midi-api/#idl-def-MIDIOutput.
We convert it to platform dependent timestamp in MIDIManager in browser process.
E.g., for OS X, we need to convert it to Core MIDI API compatible timestamp.

[palmer, 2014-10-20 21:34:42]

> This is not changed. Just replaced from line 93. We use double since this is
> JavaScript exposing value and the type is defined in Web MIDI API spec;
> http://webaudio.github.io/web-midi-api/#idl-def-MIDIOutput.
> We convert it to platform dependent timestamp in MIDIManager in browser
process.
> E.g., for OS X, we need to convert it to Core MIDI API compatible timestamp.

SGTM. I'll wait until the more fundamental question from scherkus is answered.

[Takashi Toyoshima, 2014-10-21 01:24:07]

I agreed other points, and will fix them in the next patch set.

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:68: void
MidiMessageFilter::SendMidiData(uint32 port,
This function should be called from main thread.

Using RenderFrameObserver sounds good idea.
Can I added TODO for now? I'll rewrite this class as RenderFrameObserver in a
separate CL after a series of functional changes for crbug.com/424859.

[Takashi Toyoshima, 2014-10-21 01:49:46]

PTAL Patch Set 7

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:199: for (media::MidiPortInfo info
: inputs_) {
On 2014/10/20 18:05:48, scherkus wrote:
> won't this create a copy?
> 
> you could use const auto&

Done.

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:208: for (media::MidiPortInfo info
: outputs_) {
On 2014/10/20 18:05:48, scherkus wrote:
> ditto

Done.

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

https://codereview.chromium.org/662853003/diff/120001/content/browser/media/m...
File content/browser/media/midi_host.cc (right):

https://codereview.chromium.org/662853003/diff/120001/content/browser/media/m...
content/browser/media/midi_host.cc:121: void MidiHost::OnEndSession() {
DCHECK(is_session_requested_);

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
content/renderer/media/midi_message_filter.cc:56:
DCHECK(main_message_loop_->BelongsToCurrentThread());
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.

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

https://codereview.chromium.org/662853003/diff/120001/media/midi/midi_manager...
media/midi/midi_manager.cc:88: base::AutoLock auto_lock(lock_);
Please add a comment that |client| can be in the destruction process and calling
methods is dangerous.

[scherkus (not reviewing), 2014-10-21 17:06:18]

lgtm

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/80001/content/renderer/media/m...
content/renderer/media/midi_message_filter.cc:68: void
MidiMessageFilter::SendMidiData(uint32 port,
On 2014/10/21 01:24:07, Takashi Toyoshima (chromium) wrote:
> This function should be called from main thread.
> 
> Using RenderFrameObserver sounds good idea.
> Can I added TODO for now? I'll rewrite this class as RenderFrameObserver in a
> separate CL after a series of functional changes for crbug.com/424859.

Sounds good.

I'm not guaranteeing it'll work but I do think it's worth spending some time
investigating whether you can swap it out. It'll reduce the amount of
boilerplate handling messages/clients between IO and main threads.

In general I'm also wary of mixing locks with message passing via PostTask() --
hopefully using RenderFrameObserver works out!

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

https://codereview.chromium.org/662853003/diff/120001/content/browser/media/m...
File content/browser/media/midi_host.cc (right):

https://codereview.chromium.org/662853003/diff/120001/content/browser/media/m...
content/browser/media/midi_host.cc:77: IPC_MESSAGE_HANDLER(MidiHostMsg_SendData,
OnSendData)
It's in this function that the browser should assert a sanity limit on the size
of the data. (Currently, it does not set any limit.)

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
content/renderer/media/midi_message_filter.cc:82: std::vector<uint8> v(data,
data + length);
Can we assert that |length| is not > some maximum reasonable value (and then
also enforce that on the browser side)?

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

https://codereview.chromium.org/662853003/diff/120001/media/midi/midi_manager...
media/midi/midi_manager.cc:39: // Should not happen. But just in case on
renderer being hijacked.
Nit: "But just in case the renderer is compromised."

[Takashi Toyoshima, 2014-10-22 03:50:57]

Palmer: Can you read following my two comments firstly? I'm not sure I could
understand your concern correctly, but I hope it is an answer for your question.

https://codereview.chromium.org/662853003/diff/120001/content/browser/media/m...
File content/browser/media/midi_host.cc (right):

https://codereview.chromium.org/662853003/diff/120001/content/browser/media/m...
content/browser/media/midi_host.cc:77: IPC_MESSAGE_HANDLER(MidiHostMsg_SendData,
OnSendData)
line 114 is the check. We do not want to assert it because a hijacked renderer
can send such an unexpected data that can fire the assertion.

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
content/renderer/media/midi_message_filter.cc:82: std::vector<uint8> v(data,
data + length);
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.

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

I'll upload new version that do not need lock, and is safe on client object
handling.

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
content/renderer/media/midi_message_filter.cc:44:
main_message_loop_->PostTask(FROM_HERE,
Oh, I do not need to post this :(

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
content/renderer/media/midi_message_filter.cc:56:
DCHECK(main_message_loop_->BelongsToCurrentThread());
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.

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

https://codereview.chromium.org/662853003/diff/120001/media/midi/midi_manager...
media/midi/midi_manager.cc:39: // Should not happen. But just in case on
renderer being hijacked.
On 2014/10/21 21:20:12, Chromium Palmer wrote:
> Nit: "But just in case the renderer is compromised."

Done.

https://codereview.chromium.org/662853003/diff/120001/media/midi/midi_manager...
media/midi/midi_manager.cc:88: base::AutoLock auto_lock(lock_);
On 2014/10/21 13:21:20, yhirano wrote:
> Please add a comment that |client| can be in the destruction process and
calling
> methods is dangerous.

Done.

[Takashi Toyoshima, 2014-10-22 06:11:01]

Patchset #9 (id:160001) has been deleted

[Takashi Toyoshima, 2014-10-22 06:16:35]

https://codereview.chromium.org/662853003/diff/180001/content/renderer/media/...
File content/renderer/media/midi_message_filter.h (right):

https://codereview.chromium.org/662853003/diff/180001/content/renderer/media/...
content/renderer/media/midi_message_filter.h:70: // TODO(toyoshim):
MidiPortInfoList objects should be notified separately
This will be fixed in the next CL https://codereview.chromium.org/664843002/.
(Note: the next CL should be rebased not to use lock as this CL changed)

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

LGTM

https://codereview.chromium.org/662853003/diff/120001/content/browser/media/m...
File content/browser/media/midi_host.cc (right):

https://codereview.chromium.org/662853003/diff/120001/content/browser/media/m...
content/browser/media/midi_host.cc:77: IPC_MESSAGE_HANDLER(MidiHostMsg_SendData,
OnSendData)
> line 114 is the check. We do not want to assert it because a hijacked renderer
> can send such an unexpected data that can fire the assertion.

Oh good, I see. (I meant "assert" in the general sense, not in the C++ sense of
ASSERT, so your check is fine.)

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
File content/renderer/media/midi_message_filter.cc (right):

https://codereview.chromium.org/662853003/diff/120001/content/renderer/media/...
content/renderer/media/midi_message_filter.cc:82: std::vector<uint8> v(data,
data + length);
Oops, I just missed it. Yes, thanks.

[yhirano, 2014-10-23 03:38:18]

lgtm

[Takashi Toyoshima, 2014-10-23 04:10:37]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2014-10-23 04:10:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/662853003/200001

[commit-bot: I haz the power, 2014-10-23 05:16:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-23 05:16:33]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[Takashi Toyoshima, 2014-10-23 05:36:45]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2014-10-23 05:37:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/662853003/200001

[commit-bot: I haz the power, 2014-10-23 07:18:10]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2014-10-23 07:19:12]

Patchset 10 (id:??) landed as
https://crrev.com/71fcb150d9e2cb5e2f05069177ac611ac9c189c0
Cr-Commit-Position: refs/heads/master@{#300851}