Implement SequencedTaskChecker.

webrtc/base/sequenced_task_checker_unittest.cc

Index: webrtc/base/sequenced_task_checker_unittest.cc
diff --git a/webrtc/base/sequenced_task_checker_unittest.cc b/webrtc/base/sequenced_task_checker_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..5fe2a9e8f9949a441642b6b4f99ce2fdddec9104
--- /dev/null
+++ b/webrtc/base/sequenced_task_checker_unittest.cc
@@ -0,0 +1,303 @@
+/*
+ *  Copyright (c) 2016 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.
+ */
+
+// Borrowed from Chromium's src/base/threading/thread_checker_unittest.cc.
+
+#include <memory>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/base/checks.h"
+#include "webrtc/base/constructormagic.h"
+#include "webrtc/base/sequenced_task_checker.h"
+#include "webrtc/base/task_queue.h"
+#include "webrtc/base/thread.h"
+
+// Duplicated from base/threading/sequenced_thread_checker.h so that we can be
+// good citizens there and undef the macro.
+#if (!defined(NDEBUG) || defined(DCHECK_ALWAYS_ON))
+#define ENABLE_SEQUENCED_TASK_CHECKER 1

[tommi, 2016/07/08 13:15:15]

this doesn't look like the right place to change this.  Instead, we should be
able to test SequencedTaskCheckerImpl directly when it's available (and not rely
on the SequencedTaskChecker typedef).

[perkj_webrtc, 2016/07/11 08:38:01]

Ok- These tests were a copy of the threadcheker unittests. Now I have rewritten
them quite a lot. Ptal. 

+#else
+#define ENABLE_SEQUENCED_TASK_CHECKER 0
+#endif
+
+namespace rtc {
+
+namespace {
+
+// Simple class to exercise the basics of SequencedTaskChecker.
+class SequencedCheckerClass : public SequencedTaskChecker {
+ public:
+  SequencedCheckerClass() {}
+
+  // Verifies that it was called on the same thread as the constructor.
+  void DoStuff() { RTC_DCHECK(IsCurrent()); }

[tommi, 2016/07/08 13:15:15]

since this is a test, it's probably better to do EXPECT_TRUE(IsCurrent());.
If the DCHECK fails, it could crash the process and prevent following tests from
executing.

[perkj_webrtc, 2016/07/11 08:38:01]

Done. - No longer using death tests.

+
+  void DetachFromSequence() { SequencedTaskChecker::Detach(); }

[tommi, 2016/07/08 13:15:16]

Is this method needed or can the caller just call Detach() instead?

[perkj_webrtc, 2016/07/11 08:38:01]

Done.

+
+  static void MethodOnDifferentThreadImpl();
+  static void MethodOnDifferentTaskQueueImpl();
+  static void DetachThenCallFromDifferentThreadImpl();
+  static void DetachThenCallFromDifferentTaskQueueImpl();
+
+ private:
+  RTC_DISALLOW_COPY_AND_ASSIGN(SequencedCheckerClass);
+};
+
+// Calls SequencedCheckerClass::DoStuff on another thread.
+class CallDoStuffOnThread : public Thread {

[tommi, 2016/07/08 13:15:15]

instead of using rtc::Thread (which we're using less and less), can you use
PlatformThread?

[perkj_webrtc, 2016/07/11 08:38:01]

Done.

+ public:
+  explicit CallDoStuffOnThread(SequencedCheckerClass* sequence_checker_class)
+      : Thread(), sequence_checker_class_(sequence_checker_class) {
+    SetName("call_do_stuff_on_thread", NULL);
+  }
+
+  void Run() override { sequence_checker_class_->DoStuff(); }
+
+  // New method. Needed since Thread::Join is protected, and it is called by
+  // the TEST.
+  void Join() { Thread::Join(); }

[tommi, 2016/07/08 13:15:15]

I think you can also do:

using Thread::Join;

[perkj_webrtc, 2016/07/11 08:38:01]

Acknowledged.

+
+ private:
+  SequencedCheckerClass* sequence_checker_class_;
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(CallDoStuffOnThread);
+};
+
+// Deletes SequencedCheckerClass on a different thread.
+class DeleteThreadCheckerClassOnThread : public Thread {
+ public:
+  explicit DeleteThreadCheckerClassOnThread(
+      SequencedCheckerClass* sequence_checker_class)
+      : Thread(), sequenced_checker_class_(sequence_checker_class) {
+    SetName("delete_sequence_checker_class_on_thread", NULL);
+  }
+
+  void Run() override { sequenced_checker_class_.reset(); }
+
+  // New method. Needed since Thread::Join is protected, and it is called by
+  // the TEST.
+  void Join() { Thread::Join(); }
+
+ private:
+  std::unique_ptr<SequencedCheckerClass> sequenced_checker_class_;
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(DeleteThreadCheckerClassOnThread);
+};
+
+}  // namespace
+
+TEST(SequencedTaskCheckerTest, CallsAllowedOnSameThread) {
+  std::unique_ptr<SequencedCheckerClass> sequence_checker_class(
+      new SequencedCheckerClass);

[tommi, 2016/07/08 13:15:15]

new SequencedCheckerClass()

[perkj_webrtc, 2016/07/11 08:38:01]

Done.

+
+  // Verify that DoStuff doesn't assert.
+  sequence_checker_class->DoStuff();
+
+  // Verify that the destructor doesn't assert.
+  sequence_checker_class.reset();
+}
+
+TEST(SequencedTaskCheckerTest, DestructorAllowedOnDifferentThread) {
+  std::unique_ptr<SequencedCheckerClass> sequence_checker_class(
+      new SequencedCheckerClass);
+
+  // Verify that the destructor doesn't assert
+  // when called on a different thread.
+  DeleteThreadCheckerClassOnThread delete_on_thread(
+      sequence_checker_class.release());
+
+  delete_on_thread.Start();
+  delete_on_thread.Join();
+}
+
+TEST(SequencedTaskCheckerTest, DetachFromThread) {
+  std::unique_ptr<SequencedCheckerClass> sequence_checker_class(
+      new SequencedCheckerClass);
+
+  // Verify that DoStuff doesn't assert when called on a different thread after
+  // a call to DetachFromSequence.
+  sequence_checker_class->DetachFromSequence();
+  CallDoStuffOnThread call_on_thread(sequence_checker_class.get());
+
+  call_on_thread.Start();
+  call_on_thread.Join();
+}
+
+TEST(SequencedTaskCheckerTest, DetachFromThreadAndUseOnTaskQueue) {
+  std::unique_ptr<SequencedCheckerClass> sequence_checker_class(
+      new SequencedCheckerClass);
+
+  // Verify that DoStuff doesn't assert when called on a different tq after
+  // a call to DetachFromSequence.
+  sequence_checker_class->DetachFromSequence();
+  static const char kQueueName[] = "DetachFromThreadAndUseOnTaskQueue";
+  TaskQueue queue(kQueueName);
+  Event done_event(false, false);
+  queue.PostTask([&sequence_checker_class, &done_event] {
+    sequence_checker_class->DoStuff();
+    done_event.Set();
+  });
+  EXPECT_TRUE(done_event.Wait(1000));
+}
+
+TEST(SequencedTaskCheckerTest, DetachFromTaskQueueAndUseOnThread) {
+  TaskQueue queue("DetachFromTaskQueueAndUseOnThread");
+  Event done_event(false, false);
+  queue.PostTask([&done_event] {
+    std::unique_ptr<SequencedCheckerClass> sequence_checker_class(
+        new SequencedCheckerClass);
+
+    // Verify that DoStuff doesn't assert when called on a different thread
+    // after a call to DetachFromSequence.
+    sequence_checker_class->DetachFromSequence();
+    CallDoStuffOnThread call_on_thread(sequence_checker_class.get());
+
+    call_on_thread.Start();
+    call_on_thread.Join();
+
+    done_event.Set();
+  });
+  EXPECT_TRUE(done_event.Wait(1000));
+}
+
+#if GTEST_HAS_DEATH_TEST || !ENABLE_SEQUENCED_TASK_CHECKER

[tommi, 2016/07/08 13:15:15]

I'm having problems groking this check... can you add a comment that explains
the behavior in either case?

[perkj_webrtc, 2016/07/11 08:38:01]

Refactored to no longer use death tests.

+
+void SequencedCheckerClass::MethodOnDifferentThreadImpl() {
+  std::unique_ptr<SequencedCheckerClass> sequence_checker_class(
+      new SequencedCheckerClass);
+
+  // DoStuff should assert in debug builds only when called on a
+  // different thread.
+  CallDoStuffOnThread call_on_thread(sequence_checker_class.get());
+
+  call_on_thread.Start();
+  call_on_thread.Join();
+}
+
+#if ENABLE_SEQUENCED_TASK_CHECKER
+TEST(SequencedTaskCheckerDeathTest, MethodNotAllowedOnDifferentThreadInDebug) {
+  ASSERT_DEATH({ SequencedCheckerClass::MethodOnDifferentThreadImpl(); }, "");
+}
+#else
+TEST(SequencedTaskCheckerDeathTest, MethodAllowedOnDifferentThreadInRelease) {
+  SequencedCheckerClass::MethodOnDifferentThreadImpl();
+}
+#endif  // ENABLE_SEQUENCED_TASK_CHECKER
+
+void SequencedCheckerClass::MethodOnDifferentTaskQueueImpl() {
+  std::unique_ptr<SequencedCheckerClass> sequence_checker_class(
+      new SequencedCheckerClass);
+
+  // Verify that DoStuff assert when called on a tq.
+  static const char kQueueName[] = "MethodNotAllowedOnDifferentTq";
+  TaskQueue queue(kQueueName);
+  Event done_event(false, false);
+  queue.PostTask([&sequence_checker_class, &done_event] {
+    sequence_checker_class->DoStuff();
+    done_event.Set();
+  });
+  EXPECT_TRUE(done_event.Wait(1000));
+}
+
+#if ENABLE_SEQUENCED_TASK_CHECKER
+TEST(SequencedTaskCheckerDeathTest, MethodNotAllowedOnDifferentTqInDebug) {
+  ASSERT_DEATH({ SequencedCheckerClass::MethodOnDifferentTaskQueueImpl(); },
+               "");
+}
+#else
+TEST(SequencedTaskCheckerDeathTest, MethodAllowedOnDifferentTqInRelease) {
+  SequencedCheckerClass::MethodOnDifferentTaskQueueImpl();
+}
+#endif  // ENABLE_SEQUENCED_TASK_CHECKER
+
+void SequencedCheckerClass::DetachThenCallFromDifferentTaskQueueImpl() {
+  std::unique_ptr<SequencedCheckerClass> sequence_checker_class(
+      new SequencedCheckerClass);
+
+  // DoStuff doesn't assert when called on a different task queue
+  // after a call to DetachFromSequence.
+  sequence_checker_class->DetachFromSequence();
+
+  Event done_event(false, false);
+  TaskQueue queue1("DetachThenCallFromDifferentTaskQueueImpl1");
+  queue1.PostTask([&sequence_checker_class, &done_event] {
+    sequence_checker_class->DoStuff();
+    done_event.Set();
+  });
+  EXPECT_TRUE(done_event.Wait(1000));
+
+  // DoStuff should assert in debug builds only after moving to
+  // another task queue.
+  TaskQueue queue2("DetachThenCallFromDifferentTaskQueueImpl2");
+  queue2.PostTask([&sequence_checker_class, &done_event] {
+    sequence_checker_class->DoStuff();
+    done_event.Set();
+  });
+  done_event.Wait(1000);
+}
+
+#if ENABLE_SEQUENCED_TASK_CHECKER
+TEST(SequencedTaskCheckerDeathTest, DetachFromTaskQueueInDebug) {
+  ASSERT_DEATH(
+      { SequencedCheckerClass::DetachThenCallFromDifferentTaskQueueImpl(); },
+      "");
+}
+#else
+TEST(SequencedTaskCheckerDeathTest, DetachFromThreadInRelease) {
+  SequencedCheckerClass::DetachThenCallFromDifferentTaskQueueImpl();
+}
+#endif  // ENABLE_THREAD_CHECKER
+
+#endif  // GTEST_HAS_DEATH_TEST || !ENABLE_THREAD_CHECKER
+
+class SequencedTaskCheckerAnnotateTest {
+ public:
+  // Next two function should create warnings when compile (e.g. if used with
+  // specific T).
+  // TODO(danilchap): Find a way to test they do not compile when thread
+  // annotation checks enabled.
+  template <typename T>
+  void access_var_no_annotate() {

[tommi, 2016/07/08 13:15:15]

AccessVarNoAnnotate (since this isn't a getter)

[perkj_webrtc, 2016/07/11 08:38:01]

removed all for now.

+    var_checker_ = 42;
+  }
+
+  template <typename T>
+  void access_fun_no_annotate() {

[tommi, 2016/07/08 13:15:15]

AccessFunctionNoAnnotate etc

[perkj_webrtc, 2016/07/11 08:38:01]

dito

+    function();
+  }
+
+  // Function below should be able to compile.
+  void access_var_annotate_checker() {
+    RTC_DCHECK_RUN_ON(&checker_);
+    var_checker_ = 44;
+  }
+
+  void access_fun_annotate() {
+    RTC_DCHECK_RUN_ON(&checker_);
+    function();
+  }
+
+ private:
+  void function() RUN_ON(checker_) {}
+
+  rtc::SequencedTaskChecker checker_;
+
+  int var_checker_ GUARDED_BY(checker_);
+};
+
+TEST(SequencedTaskCheckerAnnotateTest, test) {
+  SequencedTaskCheckerAnnotateTest tester;
+  tester.access_var_annotate_checker();
+}
+
+// Just in case we ever get lumped together with other compilation units.
+#undef ENABLE_SEQUENCED_TASK_CHECKER

[tommi, 2016/07/08 13:15:15]

this could only happen if another file #includes
sequenced_task_checker_unittest.cc - and we should never do that, right?

[perkj_webrtc, 2016/07/11 08:38:01]

Acknowledged.

+
+}  // namespace rtc