Add thread check to ModuleProcessThread::DeRegisterModule

webrtc/voice_engine/channel.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/voice_engine/channel.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "webrtc/audio/utility/audio_frame_operations.h"
 #include "webrtc/base/array_view.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/format_macros.h"
 #include "webrtc/base/location.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/rate_limiter.h"
-#include "webrtc/base/thread_checker.h"
 #include "webrtc/base/timeutils.h"
 #include "webrtc/config.h"
 #include "webrtc/logging/rtc_event_log/rtc_event_log.h"
 #include "webrtc/modules/audio_coding/codecs/audio_format_conversion.h"
 #include "webrtc/modules/audio_device/include/audio_device.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/modules/pacing/packet_router.h"
 #include "webrtc/modules/rtp_rtcp/include/receive_statistics.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h"
@@ skipping 893 matching lines @@
   configuration.event_log = &(*event_log_proxy_);
   configuration.rtt_stats = &(*rtcp_rtt_stats_proxy_);
   configuration.retransmission_rate_limiter =
       retransmission_rate_limiter_.get();
 
   _rtpRtcpModule.reset(RtpRtcp::CreateRtpRtcp(configuration));
   _rtpRtcpModule->SetSendingMediaStatus(false);
 }
 
 Channel::~Channel() {
-  rtp_receive_statistics_->RegisterRtcpStatisticsCallback(NULL);
-  WEBRTC_TRACE(kTraceMemory, kTraceVoice, VoEId(_instanceId, _channelId),
-               "Channel::~Channel() - dtor");
-
-  StopSend();
-  StopPlayout();
-
-  {
-    rtc::CritScope cs(&_fileCritSect);
-    if (input_file_player_) {
-      input_file_player_->RegisterModuleFileCallback(NULL);
-      input_file_player_->StopPlayingFile();
-    }
-    if (output_file_player_) {
-      output_file_player_->RegisterModuleFileCallback(NULL);
-      output_file_player_->StopPlayingFile();
-    }
-    if (output_file_recorder_) {
-      output_file_recorder_->RegisterModuleFileCallback(NULL);
-      output_file_recorder_->StopRecording();
-    }
-  }
-
-  // The order to safely shutdown modules in a channel is:
-  // 1. De-register callbacks in modules
-  // 2. De-register modules in process thread
-  // 3. Destroy modules
-  if (audio_coding_->RegisterTransportCallback(NULL) == -1) {
-    WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId, _channelId),
-                 "~Channel() failed to de-register transport callback"
-                 " (Audio coding module)");
-  }
-  if (audio_coding_->RegisterVADCallback(NULL) == -1) {
-    WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId, _channelId),
-                 "~Channel() failed to de-register VAD callback"
-                 " (Audio coding module)");
-  }
-  // De-register modules in process thread
-  _moduleProcessThreadPtr->DeRegisterModule(_rtpRtcpModule.get());
-
-  // End of modules shutdown
+  RTC_DCHECK(!channel_state_.Get().sending);
+  RTC_DCHECK(!channel_state_.Get().playing);
 }
 
 int32_t Channel::Init() {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
   WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId, _channelId),
                "Channel::Init()");
 
   channel_state_.Reset();
 
   // --- Initial sanity
 
   if ((_engineStatisticsPtr == NULL) || (_moduleProcessThreadPtr == NULL)) {
     WEBRTC_TRACE(kTraceError, kTraceVoice, VoEId(_instanceId, _channelId),
                  "Channel::Init() must call SetEngineInformation() first");
@@ skipping 82 matching lines @@
                      "Channel::Init() failed to register CN (%d/%d) "
                      "correctly - 1",
                      codec.pltype, codec.plfreq);
       }
     }
   }
 
   return 0;
 }
 
+void Channel::Terminate() {
+  RTC_DCHECK(construction_thread_.CalledOnValidThread());
+  // Must be called on the same thread as Init().
+  WEBRTC_TRACE(kTraceMemory, kTraceVoice, VoEId(_instanceId, _channelId),
+               "Channel::Terminate");
+
+  rtp_receive_statistics_->RegisterRtcpStatisticsCallback(NULL);
+
+  StopSend();

[the sun, 2017/03/20 19:11:12]

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:07:45]

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

+  StopPlayout();
+
+  {
+    rtc::CritScope cs(&_fileCritSect);
+    if (input_file_player_) {
+      input_file_player_->RegisterModuleFileCallback(NULL);
+      input_file_player_->StopPlayingFile();
+    }
+    if (output_file_player_) {
+      output_file_player_->RegisterModuleFileCallback(NULL);
+      output_file_player_->StopPlayingFile();
+    }
+    if (output_file_recorder_) {
+      output_file_recorder_->RegisterModuleFileCallback(NULL);
+      output_file_recorder_->StopRecording();
+    }
+  }
+
+  // The order to safely shutdown modules in a channel is:
+  // 1. De-register callbacks in modules
+  // 2. De-register modules in process thread
+  // 3. Destroy modules
+  if (audio_coding_->RegisterTransportCallback(NULL) == -1) {
+    WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId, _channelId),
+                 "Terminate() failed to de-register transport callback"
+                 " (Audio coding module)");
+  }
+
+  if (audio_coding_->RegisterVADCallback(NULL) == -1) {
+    WEBRTC_TRACE(kTraceWarning, kTraceVoice, VoEId(_instanceId, _channelId),
+                 "Terminate() failed to de-register VAD callback"
+                 " (Audio coding module)");
+  }
+
+  // De-register modules in process thread
+  if (_moduleProcessThreadPtr)
+    _moduleProcessThreadPtr->DeRegisterModule(_rtpRtcpModule.get());
+
+  // End of modules shutdown
+}
+
 int32_t Channel::SetEngineInformation(Statistics& engineStatistics,
                                       OutputMixer& outputMixer,
                                       ProcessThread& moduleProcessThread,
                                       AudioDeviceModule& audioDeviceModule,
                                       VoiceEngineObserver* voiceEngineObserver,
                                       rtc::CriticalSection* callbackCritSect) {
   WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId, _channelId),
                "Channel::SetEngineInformation()");
   _engineStatisticsPtr = &engineStatistics;
   _outputMixerPtr = &outputMixer;
@@ skipping 1898 matching lines @@
   int64_t min_rtt = 0;
   if (_rtpRtcpModule->RTT(remoteSSRC, &rtt, &avg_rtt, &min_rtt, &max_rtt) !=
       0) {
     return 0;
   }
   return rtt;
 }
 
 }  // namespace voe
 }  // namespace webrtc