Fix for intermittent tsan2 errors from SendRtpToRtpOnThread and SendSrtpToSrtpOnThread.

webrtc/pc/channel_unittest.cc

 /*
  *  Copyright 2009 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.
  */
 
@@ skipping 11 matching lines @@
 #include "webrtc/media/base/fakertp.h"
 #include "webrtc/media/base/fakescreencapturerfactory.h"
 #include "webrtc/media/base/fakevideocapturer.h"
 #include "webrtc/media/base/mediachannel.h"
 #include "webrtc/media/base/rtpdump.h"
 #include "webrtc/media/base/screencastid.h"
 #include "webrtc/media/base/testutils.h"
 #include "webrtc/p2p/base/faketransportcontroller.h"
 #include "webrtc/pc/channel.h"
 
+#include <atomic>
+
 #define MAYBE_SKIP_TEST(feature)                    \
   if (!(rtc::SSLStreamAdapter::feature())) {  \
     LOG(LS_INFO) << "Feature disabled... skipping"; \
     return;                                         \
   }
 
 using cricket::CA_OFFER;
 using cricket::CA_PRANSWER;
 using cricket::CA_ANSWER;
 using cricket::CA_UPDATE;
@@ skipping 359 matching lines @@
      CreateContent(SECURE, kPcmuCodec, kH264Codec, &content);
      AddLegacyStreamInContent(ssrc, 0, &content);
      sdesc->AddContent("DUMMY_CONTENT_NAME",
                        cricket::NS_JINGLE_RTP, content.Copy());
      return sdesc;
   }
 
   class CallThread : public rtc::SignalThread {
    public:
     typedef bool (ChannelTest<T>::*Method)();
-    CallThread(ChannelTest<T>* obj, Method method, bool* result)
+    CallThread(ChannelTest<T>* obj, Method method, std::atomic<bool>* result)
         : obj_(obj),
           method_(method),
           result_(result) {
       *result = false;
     }
     virtual void DoWork() {
-      bool result = (*obj_.*method_)();
-      if (result_) {
-        *result_ = result;
-      }
+      *result_ = (*obj_.*method_)();

[ossu, 2016/03/01 15:00:38]

Since the pointer is always written to in the constructor, there's no reason to
check if it's valid here.

[Taylor Brandstetter, 2016/03/01 20:03:42]

I think the purpose of the check was so that you can create a CallThread with a
null result parameter, in case you don't care about the result. But it appears
that doesn't happen anywhere.

     }
    private:
     ChannelTest<T>* obj_;
     Method method_;
-    bool* result_;
+    std::atomic<bool>* result_;
   };
-  void CallOnThread(typename CallThread::Method method, bool* result) {
+  void CallOnThread(typename CallThread::Method method,
+                    std::atomic<bool>* result) {
     CallThread* thread = new CallThread(this, method, result);
     thread->Start();
     thread->Release();
   }
 
   void CallOnThreadAndWaitForDone(typename CallThread::Method method,
-                                  bool* result) {
+                                  std::atomic<bool>* result) {
     CallThread* thread = new CallThread(this, method, result);
     thread->Start();
     thread->Destroy(true);
   }
 
   bool CodecMatches(const typename T::Codec& c1, const typename T::Codec& c2) {
     return false;  // overridden in specialized classes
   }
 
   void OnMediaMonitor(typename T::Channel* channel,
@@ skipping 873 matching lines @@
       EXPECT_TRUE(SendCustomRtp1(kSsrc1, ++sequence_number1_1));
       EXPECT_TRUE(CheckCustomRtp2(kSsrc1, sequence_number1_1));
       EXPECT_TRUE(SendCustomRtcp2(kSsrc2));
       EXPECT_TRUE(CheckCustomRtcp1(kSsrc2));
       EXPECT_TRUE(SendCustomRtp2(kSsrc2, ++sequence_number2_2));
       EXPECT_TRUE(CheckCustomRtp1(kSsrc2, sequence_number2_2));
   }
 
   // Test that we properly send RTP without SRTP from a thread.
   void SendRtpToRtpOnThread() {
-    bool sent_rtp1, sent_rtp2, sent_rtcp1, sent_rtcp2;
+    std::atomic<bool> sent_rtp1, sent_rtp2, sent_rtcp1, sent_rtcp2;
     CreateChannels(RTCP, RTCP);
     EXPECT_TRUE(SendInitiate());
     EXPECT_TRUE(SendAccept());
     CallOnThread(&ChannelTest<T>::SendRtp1, &sent_rtp1);
     CallOnThread(&ChannelTest<T>::SendRtp2, &sent_rtp2);
     CallOnThread(&ChannelTest<T>::SendRtcp1, &sent_rtcp1);
     CallOnThread(&ChannelTest<T>::SendRtcp2, &sent_rtcp2);
     EXPECT_TRUE_WAIT(CheckRtp1(), 1000);
     EXPECT_TRUE_WAIT(CheckRtp2(), 1000);
     EXPECT_TRUE_WAIT(sent_rtp1, 1000);
     EXPECT_TRUE_WAIT(sent_rtp2, 1000);
     EXPECT_TRUE(CheckNoRtp1());
     EXPECT_TRUE(CheckNoRtp2());
     EXPECT_TRUE_WAIT(CheckRtcp1(), 1000);
     EXPECT_TRUE_WAIT(CheckRtcp2(), 1000);
     EXPECT_TRUE_WAIT(sent_rtcp1, 1000);
     EXPECT_TRUE_WAIT(sent_rtcp2, 1000);
     EXPECT_TRUE(CheckNoRtcp1());
     EXPECT_TRUE(CheckNoRtcp2());
   }
 
   // Test that we properly send SRTP with RTCP from a thread.
   void SendSrtpToSrtpOnThread() {
-    bool sent_rtp1, sent_rtp2, sent_rtcp1, sent_rtcp2;
+    std::atomic<bool> sent_rtp1, sent_rtp2, sent_rtcp1, sent_rtcp2;
     CreateChannels(RTCP | SECURE, RTCP | SECURE);
     EXPECT_TRUE(SendInitiate());
     EXPECT_TRUE(SendAccept());
     CallOnThread(&ChannelTest<T>::SendRtp1, &sent_rtp1);
     CallOnThread(&ChannelTest<T>::SendRtp2, &sent_rtp2);
     CallOnThread(&ChannelTest<T>::SendRtcp1, &sent_rtcp1);
     CallOnThread(&ChannelTest<T>::SendRtcp2, &sent_rtcp2);
     EXPECT_TRUE_WAIT(CheckRtp1(), 1000);
     EXPECT_TRUE_WAIT(CheckRtp2(), 1000);
     EXPECT_TRUE_WAIT(sent_rtp1, 1000);
@@ skipping 253 matching lines @@
     rtc::scoped_ptr<cricket::SessionDescription> sdesc3(
         CreateSessionDescriptionWithStream(3));
     EXPECT_TRUE(channel1_->PushdownRemoteDescription(
         sdesc3.get(), cricket::CA_ANSWER, &err));
     EXPECT_TRUE(media_channel1_->HasSendStream(1));
     EXPECT_FALSE(media_channel1_->HasRecvStream(2));
     EXPECT_TRUE(media_channel1_->HasRecvStream(3));
   }
 
   void TestFlushRtcp() {
-    bool send_rtcp1;
+    std::atomic<bool> send_rtcp1;
 
     CreateChannels(RTCP, RTCP);
     EXPECT_TRUE(SendInitiate());
     EXPECT_TRUE(SendAccept());
     ASSERT_TRUE(GetTransport1());
     ASSERT_TRUE(GetTransport2());
     EXPECT_EQ(2U, GetTransport1()->channels().size());
     EXPECT_EQ(2U, GetTransport2()->channels().size());
 
     // Send RTCP1 from a different thread.
@@ skipping 1052 matching lines @@
   };
   rtc::Buffer payload(data, 3);
   cricket::SendDataResult result;
   ASSERT_TRUE(media_channel1_->SendData(params, payload, &result));
   EXPECT_EQ(params.ssrc,
             media_channel1_->last_sent_data_params().ssrc);
   EXPECT_EQ("foo", media_channel1_->last_sent_data());
 }
 
 // TODO(pthatcher): TestSetReceiver?