Record async "task backtraces"

base/pending_task_unittest.cc

Index: base/pending_task_unittest.cc
diff --git a/base/pending_task_unittest.cc b/base/pending_task_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0a09e0fe4b39be05974a91f20c0911880a7a130a
--- /dev/null
+++ b/base/pending_task_unittest.cc
@@ -0,0 +1,163 @@
+// Copyright (c) 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/pending_task.h"
+
+#include <vector>
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
+#include "base/threading/thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+

[danakj, 2017/02/08 18:06:23]

can you wrap everything in a nested anon namespace so we don't have possibility
to collide with other tests or anything?

[awong, 2017/02/09 20:56:05]

can't since we're friended by MessageLoop. Added note to bug to remove friending
and add anon namespace back when done.

+class PendingTaskTest : public ::testing::Test {
+ public:
+  PendingTaskTest() {}

[danakj, 2017/02/08 18:06:23]

= default

[awong, 2017/02/09 20:56:05]

Done.

+
+  ~PendingTaskTest() override {}

[danakj, 2017/02/08 18:06:23]

= default

[awong, 2017/02/09 20:56:05]

Done.

+
+ protected:
+  typedef std::vector<const void*> ExpectedTrace;

[danakj, 2017/02/08 18:06:23]

using instead of typedef

[awong, 2017/02/09 20:56:05]

done.

I don't recognize this language anymore. :-/

+
+  static void VerifyTraceAndPost(const scoped_refptr<TaskRunner>& task_runner,

[danakj, 2017/02/08 18:06:23]

just TaskRunner*, this doesn't need to use scoped_refptr abilities

[awong, 2017/02/09 20:56:05]

So I sorta in general prefer to use const scoped_refptr<>& in these cases. The
capture in Bind() already stores a refcounted version. Moving to a rawpointer
involves another cast, and an Unretained(). I don't think it makes sense...

[danakj, 2017/02/09 21:04:14]

In that case I agree, but I think it would just require a .get() on the bind.
Unretained is only required for calling member methods, I don't *think* it
applies to static?

[awong, 2017/02/09 21:16:10]

For class methods, there is a special casing of argument one to disallow raw
pointers. However, for other arguments, there is a a check to ensure we don't
accidentally store RefCounted types without a smart pointer in the BindStorage
(this is NOT the same as the callee function signature).

  https://cs.chromium.org/chromium/src/base/bind_internal.h?l=495

Thus if we kept a raw pointer in the callee signature, then either we need to
unwrap the stored arugment from a smart pointer (which doesn't work) or we need
to tell Bind() to store a raw pointer (which is what the Unretained() hint
does).

+                                 const tracked_objects::Location& from_here,
+                                 const std::vector<const void*>& expected_trace,

[danakj, 2017/02/08 18:06:22]

you could take this by value, it's always an rvalue

[awong, 2017/02/09 20:56:05]

Is there a benefit to this? It seems like it's just introducing more of a chance
for a copy...especially through a bind?

[danakj, 2017/02/09 21:04:14]

Oh that's a good point about Bind, I was reading it like a straight function
call, it would copy it out of Callback until this is OnceCallback. I was just
thinking it's less text to parse.

+                                 Closure task) {
+    SCOPED_TRACE(StringPrintf("Callback Depth: %zu", expected_trace.size()));
+
+    // Beyond depth + 1, the trace is nonsensical because there haven't been
+    // enough nested tasks called.
+    size_t window =
+        std::min(arraysize(PendingTask::task_backtrace), expected_trace.size());
+
+    for (size_t i = 0; i < window; i++) {
+      SCOPED_TRACE(StringPrintf("Trace frame: %zu", i));
+      EXPECT_EQ(expected_trace[i],
+                base::MessageLoop::current()
+                    ->current_pending_task_->task_backtrace[i]);
+    }
+    task_runner->PostTask(from_here, std::move(task));
+  }
+
+  static void RunTwo(Closure c1, Closure c2) {
+    c1.Run();
+    c2.Run();
+  }
+};
+
+// Ensure the task backtrace populates correctly.
+TEST_F(PendingTaskTest, SingleThreadedSimple) {
+  base::MessageLoop loop;
+  const tracked_objects::Location& location0 = FROM_HERE;
+  const tracked_objects::Location& location1 = FROM_HERE;
+  const tracked_objects::Location& location2 = FROM_HERE;
+  const tracked_objects::Location& location3 = FROM_HERE;
+  const tracked_objects::Location& location4 = FROM_HERE;
+  const tracked_objects::Location& location5 = FROM_HERE;
+
+  Closure task5 = base::Bind(
+      &PendingTaskTest::VerifyTraceAndPost, loop.task_runner(), location5,
+      ExpectedTrace({location4.program_counter(), location3.program_counter(),

[danakj, 2017/02/08 18:06:23]

you could just pass the init list directly, why the cast to vector here?

[awong, 2017/02/09 20:56:05]

nope! base::Bind() doesn't understand initializer lists! :-/

[danakj, 2017/02/09 21:04:14]

Ah! Right, me forgetting the Bind again here.

+                     location2.program_counter(), location1.program_counter()}),
+      base::Bind(&DoNothing));

[danakj, 2017/02/08 18:06:23]

don't need base:: inside base

[awong, 2017/02/09 21:16:10]

Done.

+  Closure task4 = base::Bind(
+      &PendingTaskTest::VerifyTraceAndPost, loop.task_runner(), location4,
+      ExpectedTrace({location3.program_counter(), location2.program_counter(),
+                     location1.program_counter(), location0.program_counter()}),
+      task5);
+  Closure task3 = base::Bind(
+      &PendingTaskTest::VerifyTraceAndPost, loop.task_runner(), location3,
+      ExpectedTrace({location2.program_counter(), location1.program_counter(),
+                     location0.program_counter(), nullptr}),
+      task4);
+  Closure task2 = base::Bind(
+      &PendingTaskTest::VerifyTraceAndPost, loop.task_runner(), location2,
+      ExpectedTrace(
+          {location1.program_counter(), location0.program_counter(), nullptr}),
+      task3);
+  Closure task1 = base::Bind(
+      &PendingTaskTest::VerifyTraceAndPost, loop.task_runner(), location1,
+      ExpectedTrace({location0.program_counter(), nullptr}), task2);
+
+  loop.task_runner()->PostTask(location0, task1);
+
+  base::RunLoop().RunUntilIdle();
+}
+
+// Post a task onto another thread. Ensure on the other thread, it has the
+// right stack trace.
+TEST_F(PendingTaskTest, MultipleThreads) {
+  base::MessageLoop loop;  // Implicitly "thread a"

[danakj, 2017/02/08 18:06:22]

comment needs .

+  base::Thread thread_b("pt_test_b");
+  base::Thread thread_c("pt_test_c");
+  thread_b.StartAndWaitForTesting();
+  thread_c.StartAndWaitForTesting();
+
+  const tracked_objects::Location& location_a0 = FROM_HERE;
+  const tracked_objects::Location& location_a1 = FROM_HERE;
+  const tracked_objects::Location& location_a2 = FROM_HERE;
+
+  const tracked_objects::Location& location_b0 = FROM_HERE;
+  const tracked_objects::Location& location_b1 = FROM_HERE;
+
+  const tracked_objects::Location& location_c0 = FROM_HERE;
+
+  // Push one frame onto the stack in thread c and then pass back to thread a.
+  Closure task_a2 = base::Bind(
+      &PendingTaskTest::VerifyTraceAndPost, loop.task_runner(), location_a2,
+      ExpectedTrace(
+          {location_c0.program_counter(), location_b1.program_counter(),
+           location_b0.program_counter(), location_a1.program_counter()}),
+      base::Bind(&DoNothing));
+  Closure task_c0 = base::Bind(
+      &PendingTaskTest::VerifyTraceAndPost, loop.task_runner(), location_c0,
+      ExpectedTrace(
+          {location_b1.program_counter(), location_b0.program_counter(),
+           location_a1.program_counter(), location_a0.program_counter()}),
+      task_a2);
+
+  // Push one frame onto the stack in thread b then spawn two tasks, one in
+  // thread b and one in thread c using location_b1.
+  Closure task_b0_fork =
+      base::Bind(&PendingTaskTest::VerifyTraceAndPost,
+                 thread_c.message_loop()->task_runner(), location_b1,

[danakj, 2017/02/08 18:06:23]

prefer thread_c.task_runner() over thread_c.message_loop()->task_runner()

+                 ExpectedTrace({location_b0.program_counter(),
+                                location_a1.program_counter(),
+                                location_a0.program_counter(), nullptr}),
+                 task_c0);
+  Closure task_b0_local =
+      base::Bind(&PendingTaskTest::VerifyTraceAndPost,
+                 thread_c.message_loop()->task_runner(), location_b1,

[danakj, 2017/02/08 18:06:22]

should this be thread_b?

+                 ExpectedTrace({location_b0.program_counter(),
+                                location_a1.program_counter(),
+                                location_a0.program_counter(), nullptr}),
+                 base::Bind(&DoNothing));
+
+  // Push one frame onto the stack in thread a then pass to thread b.

[danakj, 2017/02/08 18:06:22]

This puts 2 frames on thread a, right? Cuz the first block also says push one
frame but it posts to a new thread right away. Here it self-posts first then
bounces.

+  Closure task_a1 = base::Bind(
+      &PendingTaskTest::VerifyTraceAndPost,
+      thread_c.message_loop()->task_runner(), location_b0,

[danakj, 2017/02/08 18:06:22]

the comment says b but this variable says c?

+      ExpectedTrace({location_a1.program_counter(),
+                     location_a0.program_counter(), nullptr}),
+      base::Bind(&PendingTaskTest::RunTwo, task_b0_local, task_b0_fork));
+  Closure task_a0 = base::Bind(
+      &PendingTaskTest::VerifyTraceAndPost, loop.task_runner(), location_a1,
+      ExpectedTrace({location_a0.program_counter(), nullptr}), task_a1);
+
+  loop.task_runner()->PostTask(location_a0, task_a0);
+
+  base::RunLoop().RunUntilIdle();
+
+  thread_b.FlushForTesting();
+  thread_b.Stop();
+
+  thread_c.FlushForTesting();
+  thread_c.Stop();
+}
+
+}  // namespace base