Prevent a race condition with MidiHost IPC sending.

Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost/MidiManagerClient).

Also moves a function call on a MidiManagerClient in MidiManager into the AutoLock
scope.

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org

Committed: https://crrev.com/bd648ad6116ad3eb09356fb8c34fb006dbe86d4c
Cr-Commit-Position: refs/heads/master@{#369091}

[Oliver Chang, 2016-01-12 05:08:27]

ptal. not sure if there is a better fix.

[Oliver Chang, 2016-01-12 05:14:49]

The CQ bit was checked by ochang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-12 05:15:11]

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

[commit-bot: I haz the power, 2016-01-12 06:47:06]

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

[commit-bot: I haz the power, 2016-01-12 06:47:07]

Dry run: This issue passed the CQ dry run.

[Takashi Toyoshima, 2016-01-12 10:20:51]

Thank you for working on this. I'm back from vacation today.

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

https://codereview.chromium.org/1576323002/diff/20001/content/browser/media/m...
content/browser/media/midi_host.cc:70: // If we get here the MidiHost is going
to be destroyed soon. Prevent any
Can we fix the problem by just calling midi_manager_->EndSession(this) and
setting is_session_requested_ false here?
After the EndSession call returns, no method calls not be called from the
MidiManager.

[Oliver Chang, 2016-01-12 16:32:39]

Description was changed from

==========
Prevent a race condition with MidiHost IPC sending.

A new member variable, |is_channel_closing_|, and an associated lock are
added to MidiHost to prevent any IPC messages from being sent after the
channel has closed. OnChannelClosing() and Send() are overriden to
facilitate this.

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org
==========

to

==========
Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost).

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org
==========

[Oliver Chang, 2016-01-12 16:33:41]

Description was changed from

==========
Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost).

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org
==========

to

==========
Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost/MidiManagerClient).

Also moves a function call on a MidiManagerClient in MidiManager into the
AutoLock scope.

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org
==========

[Oliver Chang, 2016-01-12 16:33:50]

Description was changed from

==========
Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost/MidiManagerClient).

Also moves a function call on a MidiManagerClient in MidiManager into the
AutoLock scope.

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org
==========

to

==========
Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost/MidiManagerClient).

Also moves a function call on a MidiManagerClient in MidiManager into the
AutoLock
scope.

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org
==========

[Oliver Chang, 2016-01-12 16:38:11]

ptal at patchset #3, thanks!

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

https://codereview.chromium.org/1576323002/diff/20001/content/browser/media/m...
content/browser/media/midi_host.cc:70: // If we get here the MidiHost is going
to be destroyed soon. Prevent any
On 2016/01/12 10:20:51, Takashi Toyoshima wrote:
> Can we fix the problem by just calling midi_manager_->EndSession(this) and
> setting is_session_requested_ false here?
> After the EndSession call returns, no method calls not be called from the
> MidiManager.

Good point, I changed this to EndSession instead. I had thought previously that
that wasn't sufficient, but thinking about it again it appears to be fine.

[Adam Goode, 2016-01-12 17:36:45]

Thanks for fixing this issue. The destructor was not the right place for this
functionality.

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

https://codereview.chromium.org/1576323002/diff/40001/content/browser/media/m...
content/browser/media/midi_host.cc:62: MidiHost::~MidiHost() {}
Suggest (C++11 style):

MidiHost::~MidiHost() = default;

[Oliver Chang, 2016-01-12 17:48:55]

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

https://codereview.chromium.org/1576323002/diff/40001/content/browser/media/m...
content/browser/media/midi_host.cc:62: MidiHost::~MidiHost() {}
On 2016/01/12 17:36:45, Adam Goode wrote:
> Suggest (C++11 style):
> 
> MidiHost::~MidiHost() = default;

Done.

[Adam Goode, 2016-01-12 17:53:33]

lgtm

I am not an owner though.

[Takashi Toyoshima, 2016-01-13 01:20:18]

LGTM.

[Oliver Chang, 2016-01-13 01:33:57]

The CQ bit was checked by ochang@chromium.org

[commit-bot: I haz the power, 2016-01-13 01:34:44]

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

[commit-bot: I haz the power, 2016-01-13 03:42:45]

Description was changed from

==========
Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost/MidiManagerClient).

Also moves a function call on a MidiManagerClient in MidiManager into the
AutoLock
scope.

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org
==========

to

==========
Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost/MidiManagerClient).

Also moves a function call on a MidiManagerClient in MidiManager into the
AutoLock
scope.

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org
==========

[commit-bot: I haz the power, 2016-01-13 03:42:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-13 03:43:55]

Description was changed from

==========
Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost/MidiManagerClient).

Also moves a function call on a MidiManagerClient in MidiManager into the
AutoLock
scope.

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org
==========

to

==========
Prevent a race condition with MidiHost IPC sending.

EndSession() is moved from MidiHost's destructor into OnChannelClosing() before
the destruction process starts (at which point it is unsafe to call any
functions on the MidiHost/MidiManagerClient).

Also moves a function call on a MidiManagerClient in MidiManager into the
AutoLock
scope.

BUG=576383
R=toyoshim@chromium.org,agoode@chromium.org

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

[commit-bot: I haz the power, 2016-01-13 03:43:56]

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