lacros: Fix potential deadlocks involving lacros-chrome-service.

lacros-chrome is allowed to make synchronous mojo IPC calls to
ash-chrome. To avoid deadlocks, the sequence responsible for handling
lacros-chrome mojo IPC must never block. Prior to this CL, the class
LacrosChromeServiceImpl was affine to the main-thread, which could and
did block. See bug for a concrete example of a deadlock.

This CL create a new private class
LacrosChromeServiceNeverBlockingState. This class is affine to a
never-blocking sequence. Most state has been moved from
LacrosChromeServiceImpl to the private class. The exception is the public
endpoint mojo::Remote<crosapi::mojom::SelectFile>, which is affine to
the main thread.

Bug: 1103765
Change-Id: I462311f7b2b68db5eeaab799a83d2bb6391dc575
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2293011
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Auto-Submit: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788810}

chromeos/lacros/README.md

+This directory contains the low-level system interfaces that lacros-chrome needs
+to function properly. This includes both device interaction (e.g. querying
+displays) and system-provided UI components (e.g. file picker).
+
+The closest equivalent on other operating systems would be a client-side library
+that provides OS-specific functionality, such as Win32 or Cocoa.

chromeos/lacros/browser/lacros_chrome_service_impl.cc

@@ -4,18 +4,119 @@
 
 #include "chromeos/lacros/browser/lacros_chrome_service_impl.h"
 
+#include <atomic>
 #include <utility>
 
 #include "base/logging.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
 #include "chromeos/lacros/browser/lacros_chrome_service_delegate.h"
 
 namespace chromeos {
 namespace {
 
-LacrosChromeServiceImpl* g_instance = nullptr;
+// We use a std::atomic here rather than a base::NoDestructor because we want to
+// allow instances of LacrosChromeServiceImpl to be destroyed to facilitate
+// testing.
+std::atomic<LacrosChromeServiceImpl*> g_instance = {nullptr};
 
 }  // namespace
 
+// This class that holds all state that is affine to a single, never-blocking
+// sequence. The sequence must be never-blocking to avoid deadlocks, see
+// https://crbug.com/1103765.
+class LacrosChromeServiceNeverBlockingState
+    : public crosapi::mojom::LacrosChromeService {
+ public:
+  LacrosChromeServiceNeverBlockingState(
+      scoped_refptr<base::SequencedTaskRunner> owner_sequence,
+      base::WeakPtr<LacrosChromeServiceImpl> owner)
+      : owner_sequence_(owner_sequence), owner_(owner) {
+    DETACH_FROM_SEQUENCE(sequence_checker_);
+  }
+  ~LacrosChromeServiceNeverBlockingState() override {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  }
+
+  // crosapi::mojom::LacrosChromeService:
+  void RequestAshChromeServiceReceiver(
+      RequestAshChromeServiceReceiverCallback callback) override {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    // TODO(hidehiko): Remove non-error logging from here.
+    LOG(WARNING) << "AshChromeServiceReceiver requested.";
+    std::move(callback).Run(std::move(pending_ash_chrome_service_receiver_));
+  }
+
+  void NewWindow(NewWindowCallback callback) override {
+    owner_sequence_->PostTaskAndReply(
+        FROM_HERE,
+        base::BindOnce(&LacrosChromeServiceImpl::NewWindowAffineSequence,
+                       owner_),
+        std::move(callback));
+  }
+
+  // AshChromeService is the interface that lacros-chrome uses to message
+  // ash-chrome. This method binds the remote, which allows queuing of message
+  // to ash-chrome. The messages will not go through until
+  // RequestAshChromeServiceReceiver() is invoked.
+  void BindAshChromeServiceRemote() {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    pending_ash_chrome_service_receiver_ =
+        ash_chrome_service_.BindNewPipeAndPassReceiver();
+  }
+
+  // LacrosChromeService is the interface that ash-chrome uses to message
+  // lacros-chrome. This handles and routes all incoming messages from
+  // ash-chrome.
+  void BindLacrosChromeServiceReceiver(
+      mojo::PendingReceiver<crosapi::mojom::LacrosChromeService> receiver) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    receiver_.Bind(std::move(receiver));
+  }
+
+  // These methods pass the receiver end of a mojo message pipe to ash-chrome.
+  // This effectively allows ash-chrome to receive messages sent on these
+  // message pipes.
+  void BindSelectFileReceiver(
+      mojo::PendingReceiver<crosapi::mojom::SelectFile> pending_receiver) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    ash_chrome_service_->BindSelectFile(std::move(pending_receiver));
+  }
+
+  void BindScreenManagerReceiver(
+      mojo::PendingReceiver<crosapi::mojom::ScreenManager> pending_receiver) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    ash_chrome_service_->BindScreenManager(std::move(pending_receiver));
+  }
+
+  base::WeakPtr<LacrosChromeServiceNeverBlockingState> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
+ private:
+  // Receives and routes messages from ash-chrome.
+  mojo::Receiver<crosapi::mojom::LacrosChromeService> receiver_{this};
+
+  // This remote allows lacros-chrome to send messages to ash-chrome.
+  mojo::Remote<crosapi::mojom::AshChromeService> ash_chrome_service_;
+
+  // This class holds onto the receiver for AshChromeService until ash-chrome
+  // is ready to bind it.
+  mojo::PendingReceiver<crosapi::mojom::AshChromeService>
+      pending_ash_chrome_service_receiver_;
+
+  // This allows LacrosChromeServiceNeverBlockingState to route IPC messages
+  // back to the affine thread on LacrosChromeServiceImpl. |owner_| is affine to
+  // |owner_sequence_|.
+  scoped_refptr<base::SequencedTaskRunner> owner_sequence_;
+  base::WeakPtr<LacrosChromeServiceImpl> owner_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  base::WeakPtrFactory<LacrosChromeServiceNeverBlockingState> weak_factory_{
+      this};
+};
+
 // static
 LacrosChromeServiceImpl* LacrosChromeServiceImpl::Get() {
   return g_instance;
@@ -24,42 +125,69 @@ LacrosChromeServiceImpl* LacrosChromeServiceImpl::Get() {
 LacrosChromeServiceImpl::LacrosChromeServiceImpl(
     std::unique_ptr<LacrosChromeServiceDelegate> delegate)
     : delegate_(std::move(delegate)),
-      pending_ash_chrome_service_receiver_(
-          ash_chrome_service_.BindNewPipeAndPassReceiver()) {
-  // Bind remote interfaces in ash-chrome. These remote interfaces can be used
-  // immediately. Outgoing calls will be queued.
-  ash_chrome_service_->BindSelectFile(
-      select_file_remote_.BindNewPipeAndPassReceiver());
+      sequenced_state_(nullptr, base::OnTaskRunnerDeleter(nullptr)) {
+  // The sequence on which this object was constructed, and thus affine to.
+  scoped_refptr<base::SequencedTaskRunner> affine_sequence =
+      base::SequencedTaskRunnerHandle::Get();
+
+  never_blocking_sequence_ = base::ThreadPool::CreateSequencedTaskRunner(
+      {base::TaskPriority::USER_BLOCKING,
+       base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN});
+
+  sequenced_state_ = std::unique_ptr<LacrosChromeServiceNeverBlockingState,
+                                     base::OnTaskRunnerDeleter>(
+      new LacrosChromeServiceNeverBlockingState(affine_sequence,
+                                                weak_factory_.GetWeakPtr()),
+      base::OnTaskRunnerDeleter(never_blocking_sequence_));
+  weak_sequenced_state_ = sequenced_state_->GetWeakPtr();
+
+  never_blocking_sequence_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &LacrosChromeServiceNeverBlockingState::BindAshChromeServiceRemote,
+          weak_sequenced_state_));
+
+  // Bind the remote for SelectFile on the current thread, and then pass the
+  // receiver to the never_blocking_sequence_.
+  mojo::PendingReceiver<crosapi::mojom::SelectFile>
+      select_file_pending_receiver =
+          select_file_remote_.BindNewPipeAndPassReceiver();
+  never_blocking_sequence_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &LacrosChromeServiceNeverBlockingState::BindSelectFileReceiver,
+          weak_sequenced_state_, std::move(select_file_pending_receiver)));
 
   DCHECK(!g_instance);
   g_instance = this;
 }
 
 LacrosChromeServiceImpl::~LacrosChromeServiceImpl() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
   DCHECK_EQ(this, g_instance);
   g_instance = nullptr;
 }
 
 void LacrosChromeServiceImpl::BindReceiver(
     mojo::PendingReceiver<crosapi::mojom::LacrosChromeService> receiver) {
-  receiver_.Bind(std::move(receiver));
+  never_blocking_sequence_->PostTask(
+      FROM_HERE, base::BindOnce(&LacrosChromeServiceNeverBlockingState::
+                                    BindLacrosChromeServiceReceiver,
+                                weak_sequenced_state_, std::move(receiver)));
 }
 
 void LacrosChromeServiceImpl::BindScreenManagerReceiver(
     mojo::PendingReceiver<crosapi::mojom::ScreenManager> pending_receiver) {
-  ash_chrome_service_->BindScreenManager(std::move(pending_receiver));
-}
-
-void LacrosChromeServiceImpl::RequestAshChromeServiceReceiver(
-    RequestAshChromeServiceReceiverCallback callback) {
-  // TODO(hidehiko): Remove non-error logging from here.
-  LOG(WARNING) << "AshChromeServiceReceiver requested.";
-  std::move(callback).Run(std::move(pending_ash_chrome_service_receiver_));
+  never_blocking_sequence_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &LacrosChromeServiceNeverBlockingState::BindScreenManagerReceiver,
+          weak_sequenced_state_, std::move(pending_receiver)));
 }
 
-void LacrosChromeServiceImpl::NewWindow(NewWindowCallback callback) {
+void LacrosChromeServiceImpl::NewWindowAffineSequence() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
   delegate_->NewWindow();
-  std::move(callback).Run();
 }
 
 }  // namespace chromeos

chromeos/lacros/browser/lacros_chrome_service_impl.h

@@ -8,6 +8,10 @@
 #include <memory>
 
 #include "base/component_export.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/memory/weak_ptr.h"
+#include "base/sequence_checker.h"
+#include "base/sequenced_task_runner.h"
 #include "chromeos/crosapi/mojom/crosapi.mojom.h"
 #include "chromeos/crosapi/mojom/screen_manager.mojom.h"
 #include "chromeos/crosapi/mojom/select_file.mojom.h"
@@ -19,54 +23,92 @@ namespace chromeos {
 
 class LacrosChromeServiceDelegate;
 
-// Implements LacrosChromeService, which owns the mojo remote connection to
-// ash-chrome.
-// This class is not thread safe. It can only be used on the main thread.
-class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl
-    : public crosapi::mojom::LacrosChromeService {
+// Forward declaration for class defined in .cc file that holds most of the
+// business logic of this class.
+class LacrosChromeServiceNeverBlockingState;
+
+// This class is responsible for receiving and routing mojo messages from
+// ash-chrome via the mojo::Receiver |sequenced_state_.receiver_|. This class is
+// responsible for sending and routing messages to ash-chrome via the
+// mojo::Remote |sequenced_state_.ash_chrome_service_|. Messages are sent and
+// received on a dedicated, never-blocking sequence to avoid deadlocks.
+//
+// This object is constructed, destroyed, and mostly used on an "affine
+// sequence". For most intents and purposes, this is the main/UI thread.
+//
+// This class is a singleton but is not thread safe. Each method is individually
+// documented with threading requirements.
+class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
  public:
+  // The getter is safe to call from all threads.
+  //
+  // This method returns nullptr very early or late in the application
+  // lifecycle. We've chosen to have precise constructor/destructor timings
+  // rather than rely on a lazy initializer and no destructor to allow for
+  // more precise testing.
+  //
+  // If this is accessed on a thread other than the affine sequence, the caller
+  // must invalidate or destroy the pointer before shutdown. Attempting to use
+  // this pointer during shutdown can result in UaF.
   static LacrosChromeServiceImpl* Get();
 
+  // This class is expected to be constructed and destroyed on the same
+  // sequence.
   explicit LacrosChromeServiceImpl(
       std::unique_ptr<LacrosChromeServiceDelegate> delegate);
-  ~LacrosChromeServiceImpl() override;
+  ~LacrosChromeServiceImpl();
 
+  // This can be called on any thread. This call allows LacrosChromeServiceImpl
+  // to start receiving messages from ash-chrome.
   void BindReceiver(
       mojo::PendingReceiver<crosapi::mojom::LacrosChromeService> receiver);
 
+  // This must be called on the affine sequence. It exposes a remote that can
+  // be used to show a select-file dialog.
   mojo::Remote<crosapi::mojom::SelectFile>& select_file_remote() {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
     return select_file_remote_;
   }
 
+  // This may be called on any thread.
   void BindScreenManagerReceiver(
       mojo::PendingReceiver<crosapi::mojom::ScreenManager> pending_receiver);
 
-  // crosapi::mojom::LacrosChromeService:
-  void RequestAshChromeServiceReceiver(
-      RequestAshChromeServiceReceiverCallback callback) override;
-  void NewWindow(NewWindowCallback callback) override;
-
  private:
-  // Delegate instance to inject Chrome dependent code.
+  // LacrosChromeServiceNeverBlockingState is an implementation detail of this
+  // class.
+  friend class LacrosChromeServiceNeverBlockingState;
+
+  // Creates a new window on the affine sequence.
+  void NewWindowAffineSequence();
+
+  // Delegate instance to inject Chrome dependent code. Must only be used on the
+  // affine sequence.
   std::unique_ptr<LacrosChromeServiceDelegate> delegate_;
 
-  mojo::Receiver<crosapi::mojom::LacrosChromeService> receiver_{this};
+  // This member allows lacros-chrome to use the SelectFile interface. This
+  // member is affine to the affine sequence. It is initialized in the
+  // constructor and it is immediately available for use.
+  mojo::Remote<crosapi::mojom::SelectFile> select_file_remote_;
 
-  // Proxy to AshChromeService in ash-chrome.
-  mojo::Remote<crosapi::mojom::AshChromeService> ash_chrome_service_;
+  // This member is instantiated on the affine sequence alongside the
+  // constructor. All subsequent invocations of this member, including
+  // destruction, happen on the |never_blocking_sequence_|.
+  std::unique_ptr<LacrosChromeServiceNeverBlockingState,
+                  base::OnTaskRunnerDeleter>
+      sequenced_state_;
 
-  // Pending receiver of AshChromeService.
-  // AshChromeService is bound to mojo::Remote on construction, then
-  // when AshChromeService requests via RequestAshChromeServiceReceiver,
-  // its PendingReceiver is returned.
-  // This member holds the PendingReceiver between them. Note that even
-  // during the period, calling a method on AshChromeService via Remote
-  // should be available.
-  mojo::PendingReceiver<crosapi::mojom::AshChromeService>
-      pending_ash_chrome_service_receiver_;
+  // This member is instantiated on the affine sequence, but only ever
+  // dereferenced on the |never_blocking_sequence_|.
+  base::WeakPtr<LacrosChromeServiceNeverBlockingState> weak_sequenced_state_;
 
-  // Proxy to SelectFile interface in ash-chrome.
-  mojo::Remote<crosapi::mojom::SelectFile> select_file_remote_;
+  // A sequence that is guaranteed to never block.
+  scoped_refptr<base::SequencedTaskRunner> never_blocking_sequence_;
+
+  // Checks that the method is called on the affine sequence.
+  SEQUENCE_CHECKER(affine_sequence_checker_);
+
+  base::WeakPtrFactory<LacrosChromeServiceImpl> weak_factory_{this};
 };
 
 }  // namespace chromeos

content/browser/media/capture/desktop_capturer_lacros.cc

@@ -4,13 +4,11 @@
 
 #include "content/browser/media/capture/desktop_capturer_lacros.h"
 
-#include "base/debug/stack_trace.h"
-#include "base/logging.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
 
 #include "chromeos/crosapi/cpp/window_snapshot.h"
 #include "chromeos/lacros/browser/lacros_chrome_service_impl.h"
-#include "content/public/browser/browser_task_traits.h"
-#include "content/public/browser/browser_thread.h"
 
 namespace content {
 
@@ -21,9 +19,12 @@ DesktopCapturerLacros::DesktopCapturerLacros(
   mojo::PendingRemote<crosapi::mojom::ScreenManager> pending_screen_manager;
   mojo::PendingReceiver<crosapi::mojom::ScreenManager> pending_receiver =
       pending_screen_manager.InitWithNewPipeAndPassReceiver();
-  content::GetUIThreadTaskRunner({})->PostTask(
-      FROM_HERE, base::BindOnce(&DesktopCapturerLacros::BindReceiverMainThread,
-                                std::move(pending_receiver)));
+
+  // The lacros chrome service exists at all times except during early start-up
+  // and late shut-down. This class should never be used in those two times.
+  auto* lacros_chrome_service = chromeos::LacrosChromeServiceImpl::Get();
+  DCHECK(lacros_chrome_service);
+  lacros_chrome_service->BindScreenManagerReceiver(std::move(pending_receiver));
 
   // We create a SharedRemote that binds the underlying Remote onto a
   // dedicated sequence.
@@ -103,19 +104,6 @@ void DesktopCapturerLacros::SetSharedMemoryFactory(
 
 void DesktopCapturerLacros::SetExcludedWindow(webrtc::WindowId window) {}
 
-// static
-void DesktopCapturerLacros::BindReceiverMainThread(
-    mojo::PendingReceiver<crosapi::mojom::ScreenManager> receiver) {
-  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
-  // The lacros chrome service must exist at all points in time for the lacros
-  // browser.
-  auto* lacros_chrome_service = chromeos::LacrosChromeServiceImpl::Get();
-  DCHECK(lacros_chrome_service);
-
-  lacros_chrome_service->BindScreenManagerReceiver(std::move(receiver));
-}
-
 void DesktopCapturerLacros::DidTakeSnapshot(
     bool success,
     const crosapi::WindowSnapshot& snapshot) {

content/browser/media/capture/desktop_capturer_lacros.h

@@ -44,9 +44,6 @@ class DesktopCapturerLacros : public webrtc::DesktopCapturer {
   void SetExcludedWindow(webrtc::WindowId window) override;
 
  private:
-  static void BindReceiverMainThread(
-      mojo::PendingReceiver<crosapi::mojom::ScreenManager> receiver);
-
   // Callback for when ash-chrome returns a snapshot of the screen or window as
   // a bitmap.
   void DidTakeSnapshot(bool success, const crosapi::WindowSnapshot& snapshot);