Add locking to UniqueRandomIdGenerator.

The SdpOfferAnswerHandler::ssrc_generator_ variable is used from
multiple threads.

Adding thread checks + tests for UniqueNumberGenerator along the way.

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

pc/sdp_offer_answer.h

@@ -657,8 +657,9 @@ class SdpOfferAnswerHandler : public SdpStateProvider,
   // specified by the user (or by the remote party).
   // The generator is not used directly, instead it is passed on to the
   // channel manager and the session description factory.
-  rtc::UniqueRandomIdGenerator ssrc_generator_
-      RTC_GUARDED_BY(signaling_thread());
+  // TODO(bugs.webrtc.org/12666): This variable is used from both the signaling
+  // and worker threads. See if we can't restrict usage to a single thread.
+  rtc::UniqueRandomIdGenerator ssrc_generator_;
 
   // A video bitrate allocator factory.
   // This can be injected using the PeerConnectionDependencies,

rtc_base/unique_id_generator.cc

@@ -26,6 +26,8 @@ UniqueRandomIdGenerator::UniqueRandomIdGenerator(ArrayView<uint32_t> known_ids)
 UniqueRandomIdGenerator::~UniqueRandomIdGenerator() = default;
 
 uint32_t UniqueRandomIdGenerator::GenerateId() {
+  webrtc::MutexLock lock(&mutex_);
+
   RTC_CHECK_LT(known_ids_.size(), std::numeric_limits<uint32_t>::max() - 1);
   while (true) {
     auto pair = known_ids_.insert(CreateRandomNonZeroId());
@@ -36,6 +38,7 @@ uint32_t UniqueRandomIdGenerator::GenerateId() {
 }
 
 bool UniqueRandomIdGenerator::AddKnownId(uint32_t value) {
+  webrtc::MutexLock lock(&mutex_);
   return known_ids_.insert(value).second;
 }
 

rtc_base/unique_id_generator.h

@@ -16,6 +16,9 @@
 #include <string>
 
 #include "api/array_view.h"
+#include "api/sequence_checker.h"
+#include "rtc_base/synchronization/mutex.h"
+#include "rtc_base/system/no_unique_address.h"
 
 namespace rtc {
 
@@ -47,9 +50,10 @@ class UniqueNumberGenerator {
   bool AddKnownId(TIntegral value);
 
  private:
+  RTC_NO_UNIQUE_ADDRESS webrtc::SequenceChecker sequence_checker_;
   static_assert(std::is_integral<TIntegral>::value, "Must be integral type.");
-  TIntegral counter_;
-  std::set<TIntegral> known_ids_;
+  TIntegral counter_ RTC_GUARDED_BY(sequence_checker_);
+  std::set<TIntegral> known_ids_ RTC_GUARDED_BY(sequence_checker_);
 };
 
 // This class will generate unique ids. Ids are 32 bit unsigned integers.
@@ -76,7 +80,10 @@ class UniqueRandomIdGenerator {
   bool AddKnownId(uint32_t value);
 
  private:
-  std::set<uint32_t> known_ids_;
+  // TODO(bugs.webrtc.org/12666): This lock is needed due to an instance in
+  // SdpOfferAnswerHandler being shared between threads.
+  webrtc::Mutex mutex_;
+  std::set<uint32_t> known_ids_ RTC_GUARDED_BY(&mutex_);
 };
 
 // This class will generate strings. A common use case is for identifiers.
@@ -104,18 +111,23 @@ class UniqueStringGenerator {
 };
 
 template <typename TIntegral>
-UniqueNumberGenerator<TIntegral>::UniqueNumberGenerator() : counter_(0) {}
+UniqueNumberGenerator<TIntegral>::UniqueNumberGenerator() : counter_(0) {
+  sequence_checker_.Detach();
+}
 
 template <typename TIntegral>
 UniqueNumberGenerator<TIntegral>::UniqueNumberGenerator(
     ArrayView<TIntegral> known_ids)
-    : counter_(0), known_ids_(known_ids.begin(), known_ids.end()) {}
+    : counter_(0), known_ids_(known_ids.begin(), known_ids.end()) {
+  sequence_checker_.Detach();
+}
 
 template <typename TIntegral>
 UniqueNumberGenerator<TIntegral>::~UniqueNumberGenerator() {}
 
 template <typename TIntegral>
 TIntegral UniqueNumberGenerator<TIntegral>::GenerateNumber() {
+  RTC_DCHECK_RUN_ON(&sequence_checker_);
   while (true) {
     RTC_CHECK_LT(counter_, std::numeric_limits<TIntegral>::max());
     auto pair = known_ids_.insert(counter_++);
@@ -127,6 +139,7 @@ TIntegral UniqueNumberGenerator<TIntegral>::GenerateNumber() {
 
 template <typename TIntegral>
 bool UniqueNumberGenerator<TIntegral>::AddKnownId(TIntegral value) {
+  RTC_DCHECK_RUN_ON(&sequence_checker_);
   return known_ids_.insert(value).second;
 }
 }  // namespace rtc

rtc_base/unique_id_generator_unittest.cc

@@ -15,6 +15,7 @@
 
 #include "absl/algorithm/container.h"
 #include "api/array_view.h"
+#include "api/task_queue/task_queue_base.h"
 #include "rtc_base/gunit.h"
 #include "rtc_base/helpers.h"
 #include "test/gmock.h"
@@ -23,6 +24,21 @@ using ::testing::IsEmpty;
 using ::testing::Test;
 
 namespace rtc {
+namespace {
+// Utility class that registers itself as the currently active task queue.
+class FakeTaskQueue : public webrtc::TaskQueueBase {
+ public:
+  FakeTaskQueue() : task_queue_setter_(this) {}
+
+  void Delete() override {}
+  void PostTask(std::unique_ptr<webrtc::QueuedTask> task) override {}
+  void PostDelayedTask(std::unique_ptr<webrtc::QueuedTask> task,
+                       uint32_t milliseconds) override {}
+
+ private:
+  CurrentTaskQueueSetter task_queue_setter_;
+};
+}  // namespace
 
 template <typename Generator>
 class UniqueIdGeneratorTest : public Test {};
@@ -148,4 +164,39 @@ TYPED_TEST(UniqueIdGeneratorTest,
   EXPECT_FALSE(generator2.AddKnownId(id));
 }
 
+// Tests that it's OK to construct the generator in one execution environment
+// (thread/task queue) but use it in another.
+TEST(UniqueNumberGenerator, UsedOnSecondaryThread) {
+  const auto* current_tq = webrtc::TaskQueueBase::Current();
+  // Construct the generator before `fake_task_queue` to ensure that it is
+  // constructed in a different execution environment than what
+  // `fake_task_queue` will represent.
+  UniqueNumberGenerator<uint32_t> generator;
+
+  FakeTaskQueue fake_task_queue;
+  // Sanity check to make sure we're in a different runtime environment.
+  ASSERT_NE(current_tq, webrtc::TaskQueueBase::Current());
+
+  // Generating an id should be fine in this context.
+  generator.GenerateNumber();
+}
+
+#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
+TEST(UniqueNumberGeneratorDeathTest, FailsWhenUsedInWrongContext) {
+  // Instantiate the generator before the `loop`. This ensures that
+  // thread/sequence checkers will pick up a different thread environment than
+  // `fake_task_queue` will represent.
+  UniqueNumberGenerator<uint32_t> generator;
+  // Generate an ID on the current thread. This causes the generator to attach
+  // to the current thread context.
+  generator.GenerateNumber();
+
+  // Instantiate a fake task queue that will register itself as the current tq.
+  FakeTaskQueue fake_task_queue;
+
+  // Attempting to generate an id should now trigger a dcheck.
+  EXPECT_DEATH(generator.GenerateNumber(), "");
+}
+#endif
+
 }  // namespace rtc