Add thread check to ModuleProcessThread::DeRegisterModule

Add thread check to ModuleProcessThread::DeRegisterModule and remove all unnecessary locking that was there due to one implementation calling from a different thread.

The only thing that was holding us back was the indeterministic teardown of voe::Channel(), but it turned out that fixing it wasn't that hard :)

BUG=webrtc:4508
Review-Url: https://codereview.webrtc.org/2755273004
Cr-Commit-Position: refs/heads/master@{#17315}
Committed: https://chromium.googlesource.com/external/webrtc/+/0a2391f74c001357c661d4fe39671cc82175d5c4

[tommi, 2017-03-18 22:00:40]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 22:00:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-18 22:04:45]

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

[commit-bot: I haz the power, 2017-03-18 22:04:46]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15097)

[tommi, 2017-03-18 23:39:40]

Make voe::Channel termination deterministic and remove locks

[tommi, 2017-03-18 23:39:56]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 23:40:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[tommi, 2017-03-18 23:42:10]

Remove TODO

[tommi, 2017-03-18 23:47:46]

Description was changed from

==========
Add DCHECK to DeRegisterModule

BUG=
==========

to

==========
Add thread check to ModuleProcessThread::DeRegisterModule.

The only thing that was holding us back was the indeterministic teardown of
voe::Channel(), but it turned out that fixing that too, wasn't that hard.

BUG=webrtc:4508
==========

[tommi, 2017-03-18 23:47:46]

tommi@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[tommi, 2017-03-18 23:48:00]

please take a look

[tommi, 2017-03-18 23:54:27]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-18 23:54:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-19 00:25:06]

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

[commit-bot: I haz the power, 2017-03-19 00:25:07]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-03-19 10:09:59]

Description was changed from

==========
Add thread check to ModuleProcessThread::DeRegisterModule.

The only thing that was holding us back was the indeterministic teardown of
voe::Channel(), but it turned out that fixing that too, wasn't that hard.

BUG=webrtc:4508
==========

to

==========
Add thread check to ModuleProcessThread::DeRegisterModule and remove all
unnecessary locking that was there due to one implementation calling from a
different thread.

The only thing that was holding us back was the indeterministic teardown of
voe::Channel(), but it turned out that fixing that too, wasn't that hard.

BUG=webrtc:4508
==========

[tommi, 2017-03-19 10:10:22]

Description was changed from

==========
Add thread check to ModuleProcessThread::DeRegisterModule and remove all
unnecessary locking that was there due to one implementation calling from a
different thread.

The only thing that was holding us back was the indeterministic teardown of
voe::Channel(), but it turned out that fixing that too, wasn't that hard.

BUG=webrtc:4508
==========

to

==========
Add thread check to ModuleProcessThread::DeRegisterModule and remove all
unnecessary locking that was there due to one implementation calling from a
different thread.

The only thing that was holding us back was the indeterministic teardown of
voe::Channel(), but it turned out that fixing it wasn't that hard :)

BUG=webrtc:4508
==========

[the sun, 2017-03-20 19:11:13]

lgtm

https://codereview.webrtc.org/2755273004/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2755273004/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1053: StopSend();
StopSend() looks like it has races. I'm thinking of send_sequence_number_, and
ModuleRtpRtcpImpl::SetSendingStatus(). Nothing new really.

I think we should be ok though, since the only calls to StopSend()/StartSend()
in the code base happen on the libjingle worker thread. Legacy VoEBase users
could still be in for surprises though, until VoEBase has been removed. We've
already communicated that will happen, so I suggest we proceed with your change.

[tommi, 2017-03-21 09:06:53]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2017-03-21 09:06:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[tommi, 2017-03-21 09:07:45]

https://codereview.webrtc.org/2755273004/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2755273004/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:1053: StopSend();
On 2017/03/20 19:11:12, the sun wrote:
> StopSend() looks like it has races. I'm thinking of send_sequence_number_, and
> ModuleRtpRtcpImpl::SetSendingStatus(). Nothing new really.
> 
> I think we should be ok though, since the only calls to StopSend()/StartSend()
> in the code base happen on the libjingle worker thread. Legacy VoEBase users
> could still be in for surprises though, until VoEBase has been removed. We've
> already communicated that will happen, so I suggest we proceed with your
change.

I can look into whether we can put thread checks there to make sure those are
consistently called on the same thread.

[commit-bot: I haz the power, 2017-03-21 09:31:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490087213582970,
"parent_rev": "3257b16156ee3065edbc722c6e215b58a8914ad2", "commit_rev":
"0a2391f74c001357c661d4fe39671cc82175d5c4"}

[commit-bot: I haz the power, 2017-03-21 09:31:54]

Description was changed from

==========
Add thread check to ModuleProcessThread::DeRegisterModule and remove all
unnecessary locking that was there due to one implementation calling from a
different thread.

The only thing that was holding us back was the indeterministic teardown of
voe::Channel(), but it turned out that fixing it wasn't that hard :)

BUG=webrtc:4508
==========

to

==========
Add thread check to ModuleProcessThread::DeRegisterModule and remove all
unnecessary locking that was there due to one implementation calling from a
different thread.

The only thing that was holding us back was the indeterministic teardown of
voe::Channel(), but it turned out that fixing it wasn't that hard :)

BUG=webrtc:4508

Review-Url: https://codereview.webrtc.org/2755273004
Cr-Commit-Position: refs/heads/master@{#17315}
Committed:
https://chromium.googlesource.com/external/webrtc/+/0a2391f74c001357c661d4fe3...
==========

[commit-bot: I haz the power, 2017-03-21 09:31:56]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/0a2391f74c001357c661d4fe3...