Add Support for Random Mojo Message Delays

Adds periodic pausing/resuming of random mojo bindings, to test whether Mojo users are relying on FIFO ordering of messages sent over different message pipes.

Bug: 830815
Change-Id: Ie765813235a6ff720b2c4c3be550ad474b7be566
Reviewed-on: https://chromium-review.googlesource.com/c/1325551
Commit-Queue: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612321}

content/browser/browser_main_loop.cc

@@ -124,6 +124,7 @@
 #include "media/mojo/buildflags.h"
 #include "mojo/core/embedder/embedder.h"
 #include "mojo/core/embedder/scoped_ipc_support.h"
+#include "mojo/public/cpp/bindings/mojo_buildflags.h"
 #include "mojo/public/cpp/bindings/sync_call_restrictions.h"
 #include "net/base/network_change_notifier.h"
 #include "net/socket/client_socket_factory.h"
@@ -244,6 +245,10 @@
 #include "base/mac/foundation_util.h"
 #endif
 
+#if BUILDFLAG(MOJO_RANDOM_DELAYS_ENABLED)
+#include "mojo/public/cpp/bindings/lib/test_random_mojo_delays.h"
+#endif
+
 // One of the linux specific headers defines this as a macro.
 #ifdef DestroyAll
 #undef DestroyAll
@@ -1595,6 +1600,10 @@ void BrowserMainLoop::InitializeMojo() {
     parts_->ServiceManagerConnectionStarted(
         ServiceManagerConnection::GetForProcess());
   }
+
+#if BUILDFLAG(MOJO_RANDOM_DELAYS_ENABLED)
+  mojo::BeginRandomMojoDelays();
+#endif
 }
 
 base::FilePath BrowserMainLoop::GetStartupTraceFileName() const {

content/renderer/renderer_main.cc

@@ -35,6 +35,7 @@
 #include "content/renderer/render_thread_impl.h"
 #include "content/renderer/renderer_main_platform_delegate.h"
 #include "media/media_buildflags.h"
+#include "mojo/public/cpp/bindings/mojo_buildflags.h"
 #include "ppapi/buildflags/buildflags.h"
 #include "services/service_manager/sandbox/switches.h"
 #include "third_party/blink/public/platform/scheduler/web_thread_scheduler.h"
@@ -59,6 +60,10 @@
 #include "content/renderer/pepper/pepper_plugin_registry.h"
 #endif
 
+#if BUILDFLAG(MOJO_RANDOM_DELAYS_ENABLED)
+#include "mojo/public/cpp/bindings/lib/test_random_mojo_delays.h"
+#endif
+
 namespace content {
 namespace {
 
@@ -208,6 +213,10 @@ int RendererMain(const MainFunctionParams& parameters) {
     if (need_sandbox)
       should_run_loop = platform.EnableSandbox();
 
+#if BUILDFLAG(MOJO_RANDOM_DELAYS_ENABLED)
+    mojo::BeginRandomMojoDelays();
+#endif
+
     base::HighResolutionTimerManager hi_res_timer_manager;
 
     if (should_run_loop) {

mojo/public/cpp/bindings/BUILD.gn

@@ -8,12 +8,21 @@ import("//tools/ipc_fuzzer/ipc_fuzzer.gni")
 
 declare_args() {
   enable_mojo_tracing = false
+
+  # enable_random_mojo_delays starts a task runner that periodically pauses
+  # random Mojo bindings and later resumes them, in order to test whether parts
+  # of the code implicitly rely on FIFO processing of messages sent on different
+  # message pipes (which they should not).
+  enable_random_mojo_delays = false
 }
 
 buildflag_header("mojo_buildflags") {
   header = "mojo_buildflags.h"
 
-  flags = [ "MOJO_TRACE_ENABLED=$enable_mojo_tracing" ]
+  flags = [
+    "MOJO_TRACE_ENABLED=$enable_mojo_tracing",
+    "MOJO_RANDOM_DELAYS_ENABLED=$enable_random_mojo_delays",
+  ]
 }
 
 # Headers and sources which generated bindings can depend upon. No need for
@@ -167,6 +176,13 @@ component("bindings") {
     "unique_ptr_impl_ref_traits.h",
   ]
 
+  if (enable_random_mojo_delays) {
+    sources += [
+      "lib/test_random_mojo_delays.cc",
+      "lib/test_random_mojo_delays.h",
+    ]
+  }
+
   if (enable_ipc_fuzzer && !is_nacl_nonsfi) {
     sources += [
       "lib/message_dumper.cc",

mojo/public/cpp/bindings/lib/binding_state.cc

@@ -3,8 +3,12 @@
 // found in the LICENSE file.
 
 #include "mojo/public/cpp/bindings/lib/binding_state.h"
-
 #include "mojo/public/cpp/bindings/lib/task_runner_helper.h"
+#include "mojo/public/cpp/bindings/mojo_buildflags.h"
+
+#if BUILDFLAG(MOJO_RANDOM_DELAYS_ENABLED)
+#include "mojo/public/cpp/bindings/lib/test_random_mojo_delays.h"
+#endif
 
 namespace mojo {
 namespace internal {
@@ -41,6 +45,8 @@ void BindingStateBase::Close() {
   if (!router_)
     return;
 
+  weak_ptr_factory_.InvalidateWeakPtrs();
+
   endpoint_client_.reset();
   router_->CloseMessagePipe();
   router_ = nullptr;
@@ -92,6 +98,7 @@ void BindingStateBase::BindInternal(
 
   auto sequenced_runner =
       GetTaskRunnerToUseFromUserProvidedTaskRunner(std::move(runner));
+
   MultiplexRouter::Config config =
       passes_associated_kinds
           ? MultiplexRouter::MULTI_INTERFACE
@@ -106,7 +113,12 @@ void BindingStateBase::BindInternal(
       router_->CreateLocalEndpointHandle(kMasterInterfaceId), stub,
       std::move(request_validator), has_sync_methods,
       std::move(sequenced_runner), interface_version));
+
+#if BUILDFLAG(MOJO_RANDOM_DELAYS_ENABLED)
+  MakeBindingRandomlyPaused(base::SequencedTaskRunnerHandle::Get(),
+                            weak_ptr_factory_.GetWeakPtr());
+#endif
 }
 
-}  // namesapce internal
+}  // namespace internal
 }  // namespace mojo

mojo/public/cpp/bindings/lib/binding_state.h

@@ -31,6 +31,7 @@
 #include "mojo/public/cpp/system/core.h"
 
 namespace mojo {
+
 namespace internal {
 
 class MOJO_CPP_BINDINGS_EXPORT BindingStateBase {
@@ -117,6 +118,7 @@ class BindingState : public BindingStateBase {
   }
 
   InterfaceRequest<Interface> Unbind() {
+    weak_ptr_factory_.InvalidateWeakPtrs();
     endpoint_client_.reset();
     InterfaceRequest<Interface> request(router_->PassMessagePipe());
     router_ = nullptr;
@@ -136,7 +138,7 @@ class BindingState : public BindingStateBase {
   DISALLOW_COPY_AND_ASSIGN(BindingState);
 };
 
-}  // namesapce internal
+}  // namespace internal
 }  // namespace mojo
 
 #endif  // MOJO_PUBLIC_CPP_BINDINGS_LIB_BINDING_STATE_H_

mojo/public/cpp/bindings/lib/multiplex_router.h

@@ -45,8 +45,7 @@ namespace internal {
 // single message pipe.
 //
 // It is created on the sequence where the master interface of the message pipe
-// lives. Although it is ref-counted, it is guarateed to be destructed on the
-// same sequence.
+// lives.
 // Some public methods are only allowed to be called on the creating sequence;
 // while the others are safe to call from any sequence. Please see the method
 // comments for more details.

mojo/public/cpp/bindings/lib/test_random_mojo_delays.cc

+// Copyright 2018 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 <list>
+
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/no_destructor.h"
+#include "base/rand_util.h"
+#include "base/sequence_checker.h"
+#include "base/sequenced_task_runner.h"
+#include "base/synchronization/lock.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
+#include "base/time/time.h"
+#include "mojo/public/cpp/bindings/lib/binding_state.h"
+#include "mojo/public/cpp/bindings/lib/test_random_mojo_delays.h"
+
+namespace mojo {
+namespace internal {
+
+namespace {
+constexpr int kInverseProbabilityOfDelay = 8;
+constexpr int kInverseProbabilityOfNotResuming = 10;
+constexpr base::TimeDelta kMillisecondsToResume =
+    base::TimeDelta::FromMilliseconds(2);
+constexpr base::TimeDelta kPauseBindingsFrequency =
+    base::TimeDelta::FromMilliseconds(7);
+}  // namespace
+
+// TODO(mpdenton) This only adds random delays on method call processing. This
+// also should add random delays on response processing. It is a mistake if a
+// user assumes a response callback is received and run before a subsequent
+// asynch call (over a different message pipe), and these random delays won't
+// make it any more likely to find the mistake during testing.
+class RandomMojoDelays {
+ public:
+  RandomMojoDelays()
+      : runner_for_pauses_(
+            base::CreateSequencedTaskRunnerWithTraits(base::TaskTraits())) {
+    DETACH_FROM_SEQUENCE(runner_for_pauses_sequence_checker);
+  }
+
+  void Start() {
+    runner_for_pauses_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&RandomMojoDelays::PauseRandomBindingStateBases,
+                       base::Unretained(this)));
+  }
+
+  // TODO(mpdenton) what about bindings with associated interfaces? Apparently
+  // you cannot pause on those? May need to change DCHECK to if(...) return;
+  void AddBindingStateBase(scoped_refptr<base::SequencedTaskRunner> runner,
+                           base::WeakPtr<BindingStateBase> binding_state_base) {
+    runner_for_pauses_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&RandomMojoDelays::AddBindingStateBaseInternal,
+                       base::Unretained(this), std::move(runner),
+                       std::move(binding_state_base)));
+  }
+
+ private:
+  using BindingList = std::list<base::WeakPtr<BindingStateBase>>;
+
+  // Must be called on |runner_for_pauses_| sequence. Adds a BindingStateBase
+  // for random pausing purposes.
+  void AddBindingStateBaseInternal(
+      scoped_refptr<base::SequencedTaskRunner> runner,
+      base::WeakPtr<BindingStateBase> binding_state_base) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(runner_for_pauses_sequence_checker);
+
+    binding_state_base_map_[std::move(runner)].push_back(
+        std::move(binding_state_base));
+  }
+
+  // Adds a list of BindingStateBases to be randomly paused. Used to re-attach
+  // a list of BindingStateBases to the map after randomly pausing some of the
+  // bindings on their bound sequences.
+  void AddBindingStateBaseList(scoped_refptr<base::SequencedTaskRunner> runner,
+                               BindingList binding_state_bases) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(runner_for_pauses_sequence_checker);
+
+    BindingList& list = binding_state_base_map_[std::move(runner)];
+    list.splice(list.end(), std::move(binding_state_bases));
+  }
+
+  // Resumes all bindings in |paused_binding_state_bases|.
+  void ResumeFrozenBindingStateBasesOnTaskRunner(
+      BindingList binding_state_bases,
+      BindingList paused_binding_state_bases) {
+    auto it = paused_binding_state_bases.begin();
+    while (it != paused_binding_state_bases.end()) {
+      base::WeakPtr<BindingStateBase> wptr = *it;
+      if (!wptr) {
+        // This WeakPtr was invalidated. We'll delete it
+        // from the binding_state_bases list on the next PauseLoop.
+        it = paused_binding_state_bases.erase(it);
+        continue;
+      }
+      // Skip the resume with a 1/kInverseProbabilityOfNotResuming chance.
+      if (base::RandInt(1, kInverseProbabilityOfNotResuming) >= 2) {
+        wptr->ResumeIncomingMethodCallProcessing();
+        it = paused_binding_state_bases.erase(it);
+        continue;
+      }
+      it++;
+    }
+    if (!paused_binding_state_bases.empty()) {
+      // Because we haven't resumed all the bindings, we should schedule another
+      // resumption task in the future.
+      // TODO(mpdenton) similar problem as below: can freeze shutdown if we
+      // forget to unpause bindings.
+      base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE,
+          base::BindOnce(
+              &RandomMojoDelays::ResumeFrozenBindingStateBasesOnTaskRunner,
+              base::Unretained(this), std::move(binding_state_bases),
+              std::move(paused_binding_state_bases)),
+          kMillisecondsToResume);
+      return;
+    }
+    // Re-attach the bindings to the global map for future pausing.
+    runner_for_pauses_->PostTask(
+        FROM_HERE, base::BindOnce(&RandomMojoDelays::AddBindingStateBaseList,
+                                  base::Unretained(this),
+                                  base::SequencedTaskRunnerHandle::Get(),
+                                  std::move(binding_state_bases)));
+  }
+
+  // Pause a random selection of bindings in the list |binding_state_bases|,
+  // and set them to resume in the future.
+  void PauseRandomBindingStateBasesOnTaskRunner(
+      BindingList binding_state_bases) {
+    BindingList paused_binding_state_bases;
+    auto it = binding_state_bases.begin();
+    while (it != binding_state_bases.end()) {
+      // Remove any BindingStateBases that have been destroyed already.
+      base::WeakPtr<BindingStateBase> wptr = *it;
+      if (!wptr) {
+        it = binding_state_bases.erase(it);
+        continue;
+      }
+      if (base::RandInt(1, kInverseProbabilityOfDelay) >= 2) {
+        it++;
+        continue;
+      }
+      wptr->PauseIncomingMethodCallProcessing();
+      paused_binding_state_bases.push_back(wptr);
+      it++;
+    }
+    // Set the bindings to resume soon.
+    // TODO(mpdenton) may cause deadlock on shutdown if this doesn't run. But
+    // there is no PostDelayedTaskWithTraits for a SequencedTaskRunner.
+    if (paused_binding_state_bases.size() > 0) {
+      base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+          FROM_HERE,
+          base::BindOnce(
+              &RandomMojoDelays::ResumeFrozenBindingStateBasesOnTaskRunner,
+              base::Unretained(this), std::move(binding_state_bases),
+              std::move(paused_binding_state_bases)),
+          kMillisecondsToResume);
+    } else if (binding_state_bases.size() > 0) {
+      // If we did not pause any bindings, re-attach the bindings to the global
+      // map for future pausing, if there are any left after deleting all the
+      // invalidated weak ptrs.
+      runner_for_pauses_->PostTask(
+          FROM_HERE, base::BindOnce(&RandomMojoDelays::AddBindingStateBaseList,
+                                    base::Unretained(this),
+                                    base::SequencedTaskRunnerHandle::Get(),
+                                    std::move(binding_state_bases)));
+    }
+  }
+
+  // Post tasks to every sequence with bound Mojo bindings, telling each to
+  // pause random Mojo bindings bound on the respective sequence.
+  void PauseRandomBindingStateBases() {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(runner_for_pauses_sequence_checker);
+
+    auto map_it = binding_state_base_map_.begin();
+    while (map_it != binding_state_base_map_.end()) {
+      // Tell sequence to randomly pause some of its bindings.
+      map_it->first->PostTask(
+          FROM_HERE,
+          base::BindOnce(
+              &RandomMojoDelays::PauseRandomBindingStateBasesOnTaskRunner,
+              base::Unretained(this), std::move(map_it->second)));
+      // Erase the current key-value pair (it will be re-added if necessary
+      // after resuming the bindings--for now, drop the reference to the
+      // SequencedTaskRunner).
+      map_it = binding_state_base_map_.erase(map_it);
+    }
+    // Post delayed task, instead of using a RepeatingTimer, to avoid
+    // overwhelming the task scheduling.
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::BindOnce(&RandomMojoDelays::PauseRandomBindingStateBases,
+                       base::Unretained(this)),
+        kPauseBindingsFrequency);
+  }
+
+  scoped_refptr<base::SequencedTaskRunner> runner_for_pauses_;
+  std::map<scoped_refptr<base::SequencedTaskRunner>, BindingList>
+      binding_state_base_map_;
+  SEQUENCE_CHECKER(runner_for_pauses_sequence_checker);
+};
+
+RandomMojoDelays& GetRandomMojoDelays() {
+  static base::NoDestructor<RandomMojoDelays> random_mojo_delays;
+  return *random_mojo_delays;
+}
+
+void MakeBindingRandomlyPaused(
+    scoped_refptr<base::SequencedTaskRunner> runner,
+    base::WeakPtr<BindingStateBase> binding_state_base) {
+  GetRandomMojoDelays().AddBindingStateBase(std::move(runner),
+                                            binding_state_base);
+}
+
+}  // namespace internal
+
+void BeginRandomMojoDelays() {
+  internal::GetRandomMojoDelays().Start();
+}
+
+}  // namespace mojo

mojo/public/cpp/bindings/lib/test_random_mojo_delays.h

+// Copyright 2018 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.
+
+#ifndef MOJO_PUBLIC_CPP_BINDINGS_LIB_TEST_RANDOM_MOJO_DELAYS_H_
+#define MOJO_PUBLIC_CPP_BINDINGS_LIB_TEST_RANDOM_MOJO_DELAYS_H_
+
+#include "base/memory/weak_ptr.h"
+#include "base/sequenced_task_runner.h"
+#include "mojo/public/cpp/bindings/bindings_export.h"
+
+namespace mojo {
+// Begins issuing temporary pauses on randomly selected Mojo bindings for
+// debugging purposes.
+MOJO_CPP_BINDINGS_EXPORT void BeginRandomMojoDelays();
+namespace internal {
+class BindingStateBase;
+// Adds a binding state base to make it eligible for pausing.
+MOJO_CPP_BINDINGS_EXPORT void MakeBindingRandomlyPaused(
+    scoped_refptr<base::SequencedTaskRunner>,
+    base::WeakPtr<BindingStateBase>);
+}  // namespace internal
+}  // namespace mojo
+
+#endif  // MOJO_PUBLIC_CPP_BINDINGS_LIB_TEST_RANDOM_MOJO_DELAYS_H_