Make ChannelInterface::Enabled() be async.

* Changing return value from bool to void since the operation as async
  effects anyway.
* Removing the `enabled()` accessor due to potential threading issues
  and potential TOCTOU issues. It was only used in one place anyway.
* Applying thread restrictions to member variables.

Bug: none
Change-Id: I51949f5594339952d7b717cfd82f99b532e86b23
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/216182
Commit-Queue: Tommi <tommi@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33830}

pc/channel.cc

@@ -271,16 +271,25 @@ bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) {
   return true;
 }
 
-bool BaseChannel::Enable(bool enable) {
+void BaseChannel::Enable(bool enable) {
-  worker_thread_->Invoke<void>(RTC_FROM_HERE, [this, enable] {
+  RTC_DCHECK_RUN_ON(signaling_thread());
+
+  if (enable == enabled_s_)
+    return;
+
+  enabled_s_ = enable;
+
+  worker_thread_->PostTask(ToQueuedTask(alive_, [this, enable] {
     RTC_DCHECK_RUN_ON(worker_thread());
+    // Sanity check to make sure that enabled_ and enabled_s_
+    // stay in sync.
+    RTC_DCHECK_NE(enabled_, enable);
     if (enable) {
       EnableMedia_w();
     } else {
       DisableMedia_w();
     }
-  });
+  }));
-  return true;
 }
 
 bool BaseChannel::SetLocalContent(const MediaContentDescription* content,
@@ -313,14 +322,14 @@ bool BaseChannel::SetPayloadTypeDemuxingEnabled(bool enabled) {
 
 bool BaseChannel::IsReadyToReceiveMedia_w() const {
   // Receive data if we are enabled and have local content,
-  return enabled() &&
+  return enabled_ &&
          webrtc::RtpTransceiverDirectionHasRecv(local_content_direction_);
 }
 
 bool BaseChannel::IsReadyToSendMedia_w() const {
   // Send outgoing data if we are enabled, have local and remote content,
   // and we have had some form of connectivity.
-  return enabled() &&
+  return enabled_ &&
          webrtc::RtpTransceiverDirectionHasRecv(remote_content_direction_) &&
          webrtc::RtpTransceiverDirectionHasSend(local_content_direction_) &&
          was_ever_writable();

pc/channel.h

@@ -131,7 +131,6 @@ class BaseChannel : public ChannelInterface,
     // TODO(tommi): Delete this variable.
     return transport_name_;
   }
-  bool enabled() const override { return enabled_; }
 
   // This function returns true if using SRTP (DTLS-based keying or SDES).
   bool srtp_active() const {
@@ -167,7 +166,7 @@ class BaseChannel : public ChannelInterface,
   // actually belong to a new channel. See: crbug.com/webrtc/11477
   bool SetPayloadTypeDemuxingEnabled(bool enabled) override;
 
-  bool Enable(bool enable) override;
+  void Enable(bool enable) override;
 
   const std::vector<StreamParams>& local_streams() const override {
     return local_streams_;
@@ -356,7 +355,8 @@ class BaseChannel : public ChannelInterface,
   // Currently the |enabled_| flag is accessed from the signaling thread as
   // well, but it can be changed only when signaling thread does a synchronous
   // call to the worker thread, so it should be safe.
-  bool enabled_ = false;
+  bool enabled_ RTC_GUARDED_BY(worker_thread()) = false;
+  bool enabled_s_ RTC_GUARDED_BY(signaling_thread()) = false;
   bool payload_type_demuxing_enabled_ RTC_GUARDED_BY(worker_thread()) = true;
   std::vector<StreamParams> local_streams_ RTC_GUARDED_BY(worker_thread());
   std::vector<StreamParams> remote_streams_ RTC_GUARDED_BY(worker_thread());

pc/channel_interface.h

@@ -37,10 +37,8 @@ class ChannelInterface {
 
   virtual const std::string& content_name() const = 0;
 
-  virtual bool enabled() const = 0;
-
   // Enables or disables this channel
-  virtual bool Enable(bool enable) = 0;
+  virtual void Enable(bool enable) = 0;
 
   // Used for latency measurements.
   virtual sigslot::signal1<ChannelInterface*>& SignalFirstPacketReceived() = 0;

pc/channel_unittest.cc

@@ -336,6 +336,7 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
                                              SdpType::kOffer, NULL);
     if (result) {
       channel1_->Enable(true);
+      FlushCurrentThread();
       result = channel2_->SetRemoteContent(&remote_media_content1_,
                                            SdpType::kOffer, NULL);
       if (result) {
@@ -349,6 +350,7 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
 
   bool SendAccept() {
     channel2_->Enable(true);
+    FlushCurrentThread();
     return channel1_->SetRemoteContent(&remote_media_content2_,
                                        SdpType::kAnswer, NULL);
   }
@@ -633,7 +635,7 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
     CreateContent(0, kPcmuCodec, kH264Codec, &content1);
     content1.AddStream(stream1);
     EXPECT_TRUE(channel1_->SetLocalContent(&content1, SdpType::kOffer, NULL));
-    EXPECT_TRUE(channel1_->Enable(true));
+    channel1_->Enable(true);
     EXPECT_EQ(1u, media_channel1_->send_streams().size());
 
     EXPECT_TRUE(channel2_->SetRemoteContent(&content1, SdpType::kOffer, NULL));
@@ -646,7 +648,7 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
     EXPECT_TRUE(channel1_->SetRemoteContent(&content2, SdpType::kAnswer, NULL));
     EXPECT_EQ(0u, media_channel1_->recv_streams().size());
     EXPECT_TRUE(channel2_->SetLocalContent(&content2, SdpType::kAnswer, NULL));
-    EXPECT_TRUE(channel2_->Enable(true));
+    channel2_->Enable(true);
     EXPECT_EQ(0u, media_channel2_->send_streams().size());
 
     SendCustomRtp1(kSsrc1, 0);
@@ -690,7 +692,8 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
       EXPECT_FALSE(media_channel2_->playout());
     }
     EXPECT_FALSE(media_channel2_->sending());
-    EXPECT_TRUE(channel1_->Enable(true));
+    channel1_->Enable(true);
+    FlushCurrentThread();
     if (verify_playout_) {
       EXPECT_FALSE(media_channel1_->playout());
     }
@@ -722,7 +725,8 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
       EXPECT_FALSE(media_channel2_->playout());
     }
     EXPECT_FALSE(media_channel2_->sending());
-    EXPECT_TRUE(channel2_->Enable(true));
+    channel2_->Enable(true);
+    FlushCurrentThread();
     if (verify_playout_) {
       EXPECT_TRUE(media_channel2_->playout());
     }
@@ -746,8 +750,9 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
     // Set |content2| to be InActive.
     content2.set_direction(RtpTransceiverDirection::kInactive);
 
-    EXPECT_TRUE(channel1_->Enable(true));
+    channel1_->Enable(true);
-    EXPECT_TRUE(channel2_->Enable(true));
+    channel2_->Enable(true);
+    FlushCurrentThread();
     if (verify_playout_) {
       EXPECT_FALSE(media_channel1_->playout());
     }
@@ -1365,6 +1370,9 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
       thread->ProcessMessages(0);
     }
   }
+  static void FlushCurrentThread() {
+    rtc::Thread::Current()->ProcessMessages(0);
+  }
   void WaitForThreads(rtc::ArrayView<rtc::Thread*> threads) {
     // |threads| and current thread post packets to network thread.
     for (rtc::Thread* thread : threads) {

pc/sdp_offer_answer.cc

@@ -4137,7 +4137,7 @@ void SdpOfferAnswerHandler::EnableSending() {
   RTC_DCHECK_RUN_ON(signaling_thread());
   for (const auto& transceiver : transceivers()->ListInternal()) {
     cricket::ChannelInterface* channel = transceiver->channel();
-    if (channel && !channel->enabled()) {
+    if (channel) {
       channel->Enable(true);
     }
   }

pc/test/mock_channel_interface.h

@@ -28,8 +28,7 @@ class MockChannelInterface : public cricket::ChannelInterface {
   MOCK_METHOD(MediaChannel*, media_channel, (), (const, override));
   MOCK_METHOD(const std::string&, transport_name, (), (const, override));
   MOCK_METHOD(const std::string&, content_name, (), (const, override));
-  MOCK_METHOD(bool, enabled, (), (const, override));
+  MOCK_METHOD(void, Enable, (bool), (override));
-  MOCK_METHOD(bool, Enable, (bool), (override));
   MOCK_METHOD(sigslot::signal1<ChannelInterface*>&,
               SignalFirstPacketReceived,
               (),