Fix crash with MIDI send for MidiManagerAlsa

Fix crash with MIDI send for MidiManagerAlsa

BUG=564501
Committed: https://crrev.com/a3d22f60a719a6dae77a0586ef32dd12ac463952
Cr-Commit-Position: refs/heads/master@{#363413}

[Adam Goode, 2015-12-05 05:43:38]

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

[Adam Goode, 2015-12-05 05:51:06]

agoode@chromium.org changed reviewers:
+ ochang@chromium.org

[Oliver Chang, 2015-12-05 06:47:17]

lgtm. Thanks a lot for addressing this!

Good point about MidiManager::AccumulateMidiBytesSent releasing the lock too
early, didn't notice the brace positioning. Looking at things again the
Android/USB implementations post the task to the same message loop as the caller
so this shouldn't have mattered there.

What was the motivation for the Linux/Mac implementations having a separate
thread handling these things?

[Adam Goode, 2015-12-05 12:24:18]

On 2015/12/05 06:47:17, Oliver Chang wrote:
> lgtm. Thanks a lot for addressing this!
> 
> Good point about MidiManager::AccumulateMidiBytesSent releasing the lock too
> early, didn't notice the brace positioning. Looking at things again the
> Android/USB implementations post the task to the same message loop as the
caller
> so this shouldn't have mattered there.
> 
> What was the motivation for the Linux/Mac implementations having a separate
> thread handling these things?

The Mac and Linux code was earlier, the Android implementation came later, so
there is likely a chance to go back and refactor/improve things for Mac and
Linux. An example of this consolidation is at https://crbug.com/467442

[Adam Goode, 2015-12-05 12:35:32]

The CQ bit was checked by agoode@chromium.org

[commit-bot: I haz the power, 2015-12-05 12:35:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500153002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500153002/20001

[Adam Goode, 2015-12-05 12:35:45]

The CQ bit was unchecked by agoode@chromium.org

[Takashi Toyoshima, 2015-12-07 06:00:28]

LGTM. Let me push this CL to CQ now.
I'll create separate CL to add comments, and rewrite or add TODO for
AccumulateMidiBytesSent for other platforms.

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

https://codereview.chromium.org/1500153002/diff/20001/media/midi/midi_manager...
media/midi/midi_manager.cc:156: client->AccumulateMidiBytesSent(n);
Good catch.

Can you add a comment here?

We are likely to unlock any mutex before calling a callback as possible. Without
any comment, someone may be revert this fix unexpectedly to avoid a potential
deadlock.

[Takashi Toyoshima, 2015-12-07 06:00:39]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2015-12-07 06:00:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500153002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500153002/20001

[commit-bot: I haz the power, 2015-12-07 06:50:56]

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

[commit-bot: I haz the power, 2015-12-07 06:50:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Takashi Toyoshima, 2015-12-07 07:05:57]

The CQ bit was checked by toyoshim@chromium.org

[commit-bot: I haz the power, 2015-12-07 07:07:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1500153002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1500153002/20001

[commit-bot: I haz the power, 2015-12-07 08:00:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-07 08:01:47]

Description was changed from

==========
Fix crash with MIDI send for MidiManagerAlsa

BUG=564501
==========

to

==========
Fix crash with MIDI send for MidiManagerAlsa

BUG=564501

Committed: https://crrev.com/a3d22f60a719a6dae77a0586ef32dd12ac463952
Cr-Commit-Position: refs/heads/master@{#363413}
==========

[commit-bot: I haz the power, 2015-12-07 08:01:48]

Patchset 2 (id:??) landed as
https://crrev.com/a3d22f60a719a6dae77a0586ef32dd12ac463952
Cr-Commit-Position: refs/heads/master@{#363413}