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}
diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h
index e168d79..d04c484 100644
--- a/pc/sdp_offer_answer.h
+++ b/pc/sdp_offer_answer.h
@@ -657,8 +657,9 @@
// 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,
diff --git a/rtc_base/unique_id_generator.cc b/rtc_base/unique_id_generator.cc
index d41fa8d..9fa3021 100644
--- a/rtc_base/unique_id_generator.cc
+++ b/rtc_base/unique_id_generator.cc
@@ -26,6 +26,8 @@
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 @@
}
bool UniqueRandomIdGenerator::AddKnownId(uint32_t value) {
+ webrtc::MutexLock lock(&mutex_);
return known_ids_.insert(value).second;
}
diff --git a/rtc_base/unique_id_generator.h b/rtc_base/unique_id_generator.h
index 836dc70..22398fd 100644
--- a/rtc_base/unique_id_generator.h
+++ b/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 @@
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 @@
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 @@
};
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 @@
template <typename TIntegral>
bool UniqueNumberGenerator<TIntegral>::AddKnownId(TIntegral value) {
+ RTC_DCHECK_RUN_ON(&sequence_checker_);
return known_ids_.insert(value).second;
}
} // namespace rtc
diff --git a/rtc_base/unique_id_generator_unittest.cc b/rtc_base/unique_id_generator_unittest.cc
index 868b348..835a57e 100644
--- a/rtc_base/unique_id_generator_unittest.cc
+++ b/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::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 @@
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
