Web MIDI: introduce MidiManager::Shutdown to shutdown gracefully

Web MIDI: introduce MidiManager::Shutdown to shutdown gracefully

Platform dependent MidiManager implementations allocate extra threads
lazily on the IO thread, but it does not have a chance to stop them
correctly on the same IO thread, and stop it on the main thread.

This patch introduces Shutdown() method to be called before the IO
thread stopped so that the MidiManager implementations can stop the
extra threads gracefully on the right thread.

Platform specific fixes follow this change.

BUG=374341
Committed: https://crrev.com/29aaa95a4444e6d4ce9bb789cc7996821f044e52
Cr-Commit-Position: refs/heads/master@{#352564}

[Takashi Toyoshima, 2015-09-04 06:48:04]

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

[Takashi Toyoshima, 2015-09-04 06:48:43]

PTAL.

[yhirano, 2015-09-07 06:00:25]

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

https://codereview.chromium.org/1315793008/diff/40001/media/midi/midi_manager...
media/midi/midi_manager.cc:55: DCHECK(shutted_down_);
Shouldn't we check that it is correctly finalized?
I'm not sure who ensures the condition, though.

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

https://codereview.chromium.org/1315793008/diff/40001/media/midi/midi_manager...
media/midi/midi_manager.h:183: bool shutted_down_;
"shut"'s past participle form is "shut", not "shutted".

[Takashi Toyoshima, 2015-09-07 19:04:39]

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

https://codereview.chromium.org/1315793008/diff/40001/media/midi/midi_manager...
media/midi/midi_manager.cc:55: DCHECK(shutted_down_);
We can do nothing here to recover even if Shutdown() fails. So, I think making
Shutdown returning boolean result and checking it here does not make sense.
It would be easy and reasonable to add DCHECK directly for each operation in
Shutdown().

On the other hand, this shutdown check is useful to write unit tests correctly.

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

https://codereview.chromium.org/1315793008/diff/40001/media/midi/midi_manager...
media/midi/midi_manager.h:183: bool shutted_down_;
On 2015/09/07 06:00:25, yhirano wrote:
> "shut"'s past participle form is "shut", not "shutted".

Done.

[Takashi Toyoshima, 2015-09-07 19:50:33]

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

[Takashi Toyoshima, 2015-09-07 19:51:09]

PTAL.

+kinuko for browser_main_loop.cc.

[yhirano, 2015-09-08 04:59:37]

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

https://codereview.chromium.org/1315793008/diff/40001/media/midi/midi_manager...
media/midi/midi_manager.cc:55: DCHECK(shutted_down_);
On 2015/09/07 19:04:39, Takashi Toyoshima (ooo-Sep.30) wrote:
> We can do nothing here to recover even if Shutdown() fails. So, I think making
> Shutdown returning boolean result and checking it here does not make sense.
> It would be easy and reasonable to add DCHECK directly for each operation in
> Shutdown().
> 
> On the other hand, this shutdown check is useful to write unit tests
correctly.

What I'm worrying about is the case where the posted Finalize runs (on the IO
thread) after the MidiManager is destructed (on the main loop thread). Are you
sure there is no such case?

[Takashi Toyoshima, 2015-09-08 14:11:20]

Ah, good point.
The browser main thread joins the IO thread before deleting MidiManager and so
on. It means tasks posted in Shutdown() should be invoked before the IO thread
stops.

But, I didn't care it in unit tests. I will change this check to confirm the
posted task is surely invoked.

[Takashi Toyoshima, 2015-09-08 17:34:37]

PTAL Patch Set 6.

[yhirano, 2015-09-10 07:37:30]

https://codereview.chromium.org/1315793008/diff/100001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1315793008/diff/100001/media/midi/midi_manage...
media/midi/midi_manager.cc:68: if (session_thread_runner_) {
We need to protect more code with lock in order to avoid
initialized-but-not-finalized situations.

With the current code, StartSession and Shutdown may run in the same time and
MiniManager will get confused then.

In addition to that, it seems possible to run StartSession after Shutdown is
called (or, even after Finalize is done).

[Takashi Toyoshima, 2015-09-10 08:19:15]

https://codereview.chromium.org/1315793008/diff/100001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1315793008/diff/100001/media/midi/midi_manage...
media/midi/midi_manager.cc:68: if (session_thread_runner_) {
You are right. I change code to protect session_thread_runner and remove the
race situations around StartInitialization, CompleteInitialization, and
Finalize.

[Takashi Toyoshima, 2015-09-10 08:44:15]

Also, add more checks in unit tests. Maybe we should have new unit tests for
checking multi-threading in the future.
Anyway, PTAL patch set 8.

[yhirano, 2015-09-11 12:32:49]

I'm a bit confused.

It seems we need to block IPC operations as well.

By the way, is it possible to move MidiManager ownership from BrowserMainLoop to
MidiHost? Currently the midi host is the only caller of MidiManager methods,
BrowserMessageFilter::OnMessageReceived is *usually* called on IO thread, and
MidiHost is destructed on IO thread. IIUC creating MidiManager in MidiHost (on
IO thread) solves the problem.

Please correct me if I'm wrong.

[yhirano, 2015-09-11 15:56:06]

On 2015/09/11 12:32:49, yhirano wrote:
> I'm a bit confused.
> 
> It seems we need to block IPC operations as well.
> 
> By the way, is it possible to move MidiManager ownership from BrowserMainLoop
to
> MidiHost? Currently the midi host is the only caller of MidiManager methods,
> BrowserMessageFilter::OnMessageReceived is *usually* called on IO thread, and
> MidiHost is destructed on IO thread. IIUC creating MidiManager in MidiHost (on
> IO thread) solves the problem.
> 
> Please correct me if I'm wrong.

I found my mistake. There are multiple MidiHost instances for MidiManager, so
MidiHost cannot own MidiManager. I retract my comment above.

[yhirano, 2015-09-11 15:59:47]

https://codereview.chromium.org/1315793008/diff/140001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1315793008/diff/140001/media/midi/midi_manage...
media/midi/midi_manager.cc:116: if (finalized_)
Should we call CompleteStartSession with INITIALIZATION_ERROR?

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

https://codereview.chromium.org/1315793008/diff/140001/media/midi/midi_manage...
media/midi/midi_manager.h:63: virtual void AccumulateMidiBytesSent(size_t n) =
0;
Should we have Detach() which will be called in Finalize()?

[Takashi Toyoshima, 2015-09-17 07:30:49]

ptal

https://codereview.chromium.org/1315793008/diff/140001/media/midi/midi_manage...
File media/midi/midi_manager.cc (right):

https://codereview.chromium.org/1315793008/diff/140001/media/midi/midi_manage...
media/midi/midi_manager.cc:116: if (finalized_)
This isn't easy because I do not want to call client callback inside a lock. Let
me clean up StartSession().

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

https://codereview.chromium.org/1315793008/diff/140001/media/midi/midi_manage...
media/midi/midi_manager.h:63: virtual void AccumulateMidiBytesSent(size_t n) =
0;
Do you already have any real concerns in client side on shutdown?
Otherwise, let me just leave TODO here.

[Takashi Toyoshima, 2015-09-18 02:59:58]

Note: patch set 9 has a warning to cause build errors. I fixed it in patch set
10.

[yhirano, 2015-09-28 07:50:00]

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

https://codereview.chromium.org/1315793008/diff/140001/media/midi/midi_manage...
media/midi/midi_manager.h:63: virtual void AccumulateMidiBytesSent(size_t n) =
0;
On 2015/09/17 07:30:49, Takashi Toyoshima (ooo-Sep.30) wrote:
> Do you already have any real concerns in client side on shutdown?
> Otherwise, let me just leave TODO here.

0: A MidiManager m has a client c.
1: m->Shutdown is called on the main thread.
2: m->Finalize is called on the IO thread.
3: m->DispatchSendData is called on the IO thread by c.

Is this unrealistic?

[Takashi Toyoshima, 2015-10-02 15:19:24]

PTAL

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

https://codereview.chromium.org/1315793008/diff/140001/media/midi/midi_manage...
media/midi/midi_manager.h:63: virtual void AccumulateMidiBytesSent(size_t n) =
0;
I see. Let me fix this.

[yhirano, 2015-10-05 06:59:02]

lgtm

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

https://codereview.chromium.org/1315793008/diff/220001/media/midi/midi_manage...
media/midi/midi_manager.h:67: virtual void Detached() = 0;
[optional] OnDetached() or Detach() might be better?

[Takashi Toyoshima, 2015-10-05 07:25:10]

Kinuko: can you review content/browser/browser_main_loop.cc?

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

https://codereview.chromium.org/1315793008/diff/220001/media/midi/midi_manage...
media/midi/midi_manager.h:67: virtual void Detached() = 0;
ok, will change to Detach().

[kinuko, 2015-10-05 08:56:32]

browser_main_loop lgtm

https://codereview.chromium.org/1315793008/diff/220001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1315793008/diff/220001/content/browser/browse...
content/browser/browser_main_loop.cc:694: // but must be created on the main
thread. The service ctor is
(are these changes related?)

[Takashi Toyoshima, 2015-10-05 16:11:06]

https://codereview.chromium.org/1315793008/diff/220001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1315793008/diff/220001/content/browser/browse...
content/browser/browser_main_loop.cc:694: // but must be created on the main
thread. The service ctor is
Yeah, I just want to make the term consistent with other places. This comment is
the only place to say "io thread" instead of "IO thread".

https://codereview.chromium.org/1315793008/diff/220001/content/browser/browse...
content/browser/browser_main_loop.cc:965: // Request shutdown to clean up
allocated resources on the IO thread.
I noticed that when I wrote this comment.

[Takashi Toyoshima, 2015-10-06 06:42:35]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2015-10-06 06:42:35]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1315793008/#ps240001
(title: "rename to Detach")

[commit-bot: I haz the power, 2015-10-06 06:42:57]

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

[commit-bot: I haz the power, 2015-10-06 07:54:29]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-10-06 07:55:41]

Patchset 13 (id:??) landed as
https://crrev.com/29aaa95a4444e6d4ce9bb789cc7996821f044e52
Cr-Commit-Position: refs/heads/master@{#352564}