Implement the Mac prefs lock by polling ownership of a Mach service.

If the service has a matching port with a receive right in userspace,
the task with the receive right owns the lock. If the service is not
assigned, the lock is free.

This implementation polls to attempt to acquire the lock, using a
constant service name and constant polling interval. Further development
could fetch these values from another source of truth (a file of
constants, compiler macros, options loaded at runtime, etc.) or use
bootstrap_look_up and mach_port_request_notification to find the owner
of the lock and wait for the lock to be released.

This change includes two broad changes to preexisting unit tests, due
to issues revealed with those tests while developing this CL:

* app_server_unittest.cc included tests where the AppServer lived
  in the same scope as test code that generates a reference to the
  Global Prefs Object, which requires the prefs lock. This results in
  deadlock with a non-reentrant implementation of a prefs lock, since
  the AppServer owns that lock at that time. This CL introduces scopes
  to deallocate the AppServer instance before the test itself grabs
  the global prefs lock.
* prefs_unittest.cc marked out all the lock tests as Windows-only. Now
  that there is a Mac lock, this limitation does not need to exist.
  However, one of its tests explicitly checked that the lock supports
  reentrancy within a thread (while another verified that it bans
  reentrancy across threads). This test has been deleted, after
  discussion with teammates where we decided that a non-reentrant lock
  is desired.


Change-Id: I513cb67d1c3a12b2b0c01b532a9af10ae2233530
Bug: 1135692
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2453800Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Adam Norberg <norberg@google.com>
Cr-Commit-Position: refs/heads/master@{#815825}

chrome/updater/app/app_server_unittest.cc

@@ -122,28 +122,32 @@ TEST_F(AppServerTestCase, SelfPromote) {
     local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
-  auto app = base::MakeRefCounted<AppServerTest>();
+  {
+    auto app = base::MakeRefCounted<AppServerTest>();
 
-  // Expect the app to SwapRpcInterfaces and then ActiveDuty then Shutdown(0).
-  EXPECT_CALL(*app, ActiveDuty).Times(1);
-  EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(true));
-  EXPECT_CALL(*app, UninstallSelf).Times(0);
-  EXPECT_EQ(app->Run(), 0);
+    // Expect the app to SwapRpcInterfaces and then ActiveDuty then Shutdown(0).
+    EXPECT_CALL(*app, ActiveDuty).Times(1);
+    EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(true));
+    EXPECT_CALL(*app, UninstallSelf).Times(0);
+    EXPECT_EQ(app->Run(), 0);
+  }
   std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
   EXPECT_FALSE(global_prefs->GetSwapping());
   EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
 }
 
 TEST_F(AppServerTestCase, InstallAutoPromotes) {
-  auto app = base::MakeRefCounted<AppServerTest>();
-
-  // Expect the app to SwapRpcInterfaces and then ActiveDuty then Shutdown(0).
-  // In this case it bypasses qualification.
-  EXPECT_CALL(*app, ActiveDuty).Times(1);
-  EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(true));
-  EXPECT_CALL(*app, UninstallSelf).Times(0);
-  EXPECT_EQ(app->Run(), 0);
-  EXPECT_FALSE(CreateLocalPrefs()->GetQualified());
+  {
+    auto app = base::MakeRefCounted<AppServerTest>();
+
+    // Expect the app to SwapRpcInterfaces and then ActiveDuty then Shutdown(0).
+    // In this case it bypasses qualification.
+    EXPECT_CALL(*app, ActiveDuty).Times(1);
+    EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(true));
+    EXPECT_CALL(*app, UninstallSelf).Times(0);
+    EXPECT_EQ(app->Run(), 0);
+    EXPECT_FALSE(CreateLocalPrefs()->GetQualified());
+  }
   std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
   EXPECT_FALSE(global_prefs->GetSwapping());
   EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
@@ -155,13 +159,15 @@ TEST_F(AppServerTestCase, SelfPromoteFails) {
     local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
-  auto app = base::MakeRefCounted<AppServerTest>();
+  {
+    auto app = base::MakeRefCounted<AppServerTest>();
 
-  // Expect the app to SwapRpcInterfaces and then Shutdown(2).
-  EXPECT_CALL(*app, ActiveDuty).Times(0);
-  EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(false));
-  EXPECT_CALL(*app, UninstallSelf).Times(0);
-  EXPECT_EQ(app->Run(), 2);
+    // Expect the app to SwapRpcInterfaces and then Shutdown(2).
+    EXPECT_CALL(*app, ActiveDuty).Times(0);
+    EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(false));
+    EXPECT_CALL(*app, UninstallSelf).Times(0);
+    EXPECT_EQ(app->Run(), 2);
+  }
   std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
   EXPECT_TRUE(global_prefs->GetSwapping());
   EXPECT_EQ(global_prefs->GetActiveVersion(), "0");
@@ -176,13 +182,15 @@ TEST_F(AppServerTestCase, ActiveDutyAlready) {
     local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
-  auto app = base::MakeRefCounted<AppServerTest>();
+  {
+    auto app = base::MakeRefCounted<AppServerTest>();
 
-  // Expect the app to ActiveDuty and then Shutdown(0).
-  EXPECT_CALL(*app, ActiveDuty).Times(1);
-  EXPECT_CALL(*app, SwapRPCInterfaces).Times(0);
-  EXPECT_CALL(*app, UninstallSelf).Times(0);
-  EXPECT_EQ(app->Run(), 0);
+    // Expect the app to ActiveDuty and then Shutdown(0).
+    EXPECT_CALL(*app, ActiveDuty).Times(1);
+    EXPECT_CALL(*app, SwapRPCInterfaces).Times(0);
+    EXPECT_CALL(*app, UninstallSelf).Times(0);
+    EXPECT_EQ(app->Run(), 0);
+  }
   std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
   EXPECT_FALSE(global_prefs->GetSwapping());
   EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
@@ -198,14 +206,16 @@ TEST_F(AppServerTestCase, StateDirty) {
     local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
-  auto app = base::MakeRefCounted<AppServerTest>();
-
-  // Expect the app to SwapRpcInterfaces and then ActiveDuty and then
-  // Shutdown(0).
-  EXPECT_CALL(*app, ActiveDuty).Times(1);
-  EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(true));
-  EXPECT_CALL(*app, UninstallSelf).Times(0);
-  EXPECT_EQ(app->Run(), 0);
+  {
+    auto app = base::MakeRefCounted<AppServerTest>();
+
+    // Expect the app to SwapRpcInterfaces and then ActiveDuty and then
+    // Shutdown(0).
+    EXPECT_CALL(*app, ActiveDuty).Times(1);
+    EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(true));
+    EXPECT_CALL(*app, UninstallSelf).Times(0);
+    EXPECT_EQ(app->Run(), 0);
+  }
   std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
   EXPECT_FALSE(global_prefs->GetSwapping());
   EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
@@ -221,13 +231,15 @@ TEST_F(AppServerTestCase, StateDirtySwapFails) {
     local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
-  auto app = base::MakeRefCounted<AppServerTest>();
+  {
+    auto app = base::MakeRefCounted<AppServerTest>();
 
-  // Expect the app to SwapRpcInterfaces and Shutdown(2).
-  EXPECT_CALL(*app, ActiveDuty).Times(0);
-  EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(false));
-  EXPECT_CALL(*app, UninstallSelf).Times(0);
-  EXPECT_EQ(app->Run(), 2);
+    // Expect the app to SwapRpcInterfaces and Shutdown(2).
+    EXPECT_CALL(*app, ActiveDuty).Times(0);
+    EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(false));
+    EXPECT_CALL(*app, UninstallSelf).Times(0);
+    EXPECT_EQ(app->Run(), 2);
+  }
   std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
   EXPECT_TRUE(global_prefs->GetSwapping());
   EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);

chrome/updater/prefs_mac.mm

@@ -4,18 +4,107 @@
 
 #include "chrome/updater/prefs_impl.h"
 
+#include <mach/mach.h>
+#include <servers/bootstrap.h>
+
+#include "base/logging.h"
+#include "base/mac/mach_logging.h"
+#include "base/threading/platform_thread.h"
 #include "base/time/time.h"
+#include "chrome/updater/updater_version.h"
+
+namespace {
+
+// Mach service name for the global prefs lock.
+constexpr char kPrefsLockMachServiceName[] =
+    MAC_BUNDLE_IDENTIFIER_STRING ".lock";
+
+// Interval to poll for lock availability if it is not immediately available.
+// Final interval will be truncated to fit the available timeout.
+constexpr base::TimeDelta kPrefsLockPollingInterval =
+    base::TimeDelta::FromSeconds(3);
+
+//
+// Attempts to acquire the receive right to a named Mach service.
+// Single attempt, no retries. Logs errors other than "permission denied",
+// since "permission denied" typically means the service receive rights have
+// already been assigned.
+//
+// Returns a mach_port_t representing the receive right if the right was
+// successfully acquired. If the right cannot be acquired for any reason,
+// returns MACH_PORT_NULL instead.
+mach_port_t TryAcquireReceive(const char* service_name) {
+  mach_port_t target_port = MACH_PORT_NULL;
+  kern_return_t check_in_result =
+      bootstrap_check_in(bootstrap_port, service_name, &target_port);
+  if (check_in_result != KERN_SUCCESS) {
+    // Log error reports for all errors other than BOOTSTRAP_NOT_PRIVILEGED.
+    // BOOTSTRAP_NOT_PRIVILEGED is not logged because it just means that another
+    // process has acquired the receive rights for this service.
+    if (check_in_result != BOOTSTRAP_NOT_PRIVILEGED) {
+      BOOTSTRAP_LOG(ERROR, check_in_result)
+          << "bootstrap_check_in to acquire prefs lock: "
+          << kPrefsLockMachServiceName;
+    } else {
+      BOOTSTRAP_VLOG(2, check_in_result)
+          << "Prefs lock already held: " << kPrefsLockMachServiceName;
+    }
+    return MACH_PORT_NULL;
+  }
+  return target_port;
+}
+
+// Sleep until the lock should be retried, up to an approximate maximum of
+// max_wait (within the tolerances of timing, scheduling, etc.).
+void WaitToRetryLock(base::TimeDelta max_wait) {
+  // This is a polling implementation of Mach service locking.
+  // TODO(1135787): replace with a non-polling Mach notification approach.
+  const base::TimeDelta wait_time = max_wait < kPrefsLockPollingInterval
+                                        ? max_wait
+                                        : kPrefsLockPollingInterval;
+  base::PlatformThread::Sleep(wait_time);
+}
+
+}  // anonymous namespace
 
 namespace updater {
 
 class ScopedPrefsLockImpl {
  public:
-  ScopedPrefsLockImpl() = default;
+  // Constructs a ScopedPrefsLockImpl from a receive right.
+  explicit ScopedPrefsLockImpl(mach_port_t receive_port);
+
+  // Releases the receive right (and therefore releases the lock).
+  ~ScopedPrefsLockImpl();
+
   ScopedPrefsLockImpl(const ScopedPrefsLockImpl&) = delete;
   ScopedPrefsLockImpl& operator=(const ScopedPrefsLockImpl&) = delete;
-  ~ScopedPrefsLockImpl() = default;
+
+ private:
+  // The Mach port representing the held lock itself. We only care about
+  // service ownership; no messages are transfered with this port.
+  mach_port_t receive_port_;
 };
 
+ScopedPrefsLockImpl::ScopedPrefsLockImpl(mach_port_t receive_port)
+    : receive_port_(receive_port) {
+  // Verify the validity of the lock.
+  mach_port_type_t port_type = 0;
+  kern_return_t port_check_result =
+      mach_port_type(mach_task_self(), receive_port, &port_type);
+  MACH_CHECK(port_check_result == KERN_SUCCESS, port_check_result)
+      << "ScopedPrefsLockImpl could not verify lock port";
+  CHECK(port_type & MACH_PORT_TYPE_RECEIVE)
+      << "ScopedPrefsLockImpl given port without receive right";
+}
+
+ScopedPrefsLockImpl::~ScopedPrefsLockImpl() {
+  kern_return_t destroy_result =
+      mach_port_destroy(mach_task_self(), receive_port_);
+  MACH_LOG_IF(ERROR, destroy_result != KERN_SUCCESS, destroy_result)
+      << "ScopedPrefsLockImpl could not destroy port";
+}
+
 ScopedPrefsLock::ScopedPrefsLock(std::unique_ptr<ScopedPrefsLockImpl> impl)
     : impl_(std::move(impl)) {}
 
@@ -23,9 +112,30 @@ ScopedPrefsLock::~ScopedPrefsLock() = default;
 
 std::unique_ptr<ScopedPrefsLock> AcquireGlobalPrefsLock(
     base::TimeDelta timeout) {
-  // TODO(crbug.com/1092936): implement the actual mutex.
-  return std::make_unique<ScopedPrefsLock>(
-      std::make_unique<ScopedPrefsLockImpl>());
+  // First, try to acquire the lock. If the timeout is zero or negative,
+  // this is the only attempt we will make.
+  mach_port_t lock_port = TryAcquireReceive(kPrefsLockMachServiceName);
+
+  // Set up time limits.
+  base::TimeTicks deadline = base::TimeTicks::Now() + timeout;
+  constexpr base::TimeDelta kDeltaZero;
+
+  // Loop until we acquire the lock or time out. Note that we have already
+  // tried once to acquire the lock, so this loop will execute zero times
+  // if we have already succeeded (or if the timeout was zero). Therefore,
+  // this loop waits for retry *before* attempting to acquire the lock;
+  // the special case of the first try was handled outside the loop.
+  for (base::TimeDelta remain = deadline - base::TimeTicks::Now();
+       lock_port == MACH_PORT_NULL && remain > kDeltaZero;
+       remain = deadline - base::TimeTicks::Now()) {
+    WaitToRetryLock(remain);
+    lock_port = TryAcquireReceive(kPrefsLockMachServiceName);
+  }
+
+  return lock_port == MACH_PORT_NULL
+             ? nullptr
+             : std::make_unique<ScopedPrefsLock>(
+                   std::make_unique<ScopedPrefsLockImpl>(lock_port));
 }
 
 }  // namespace updater

chrome/updater/prefs_unittest.cc

@@ -33,21 +33,6 @@ TEST(PrefsTest, PrefsCommitPendingWrites) {
   PrefsCommitPendingWrites(pref.get());
 }
 
-#if defined(OS_WIN)
-
-TEST(PrefsTest, AcquireGlobalPrefsLock_LockAgainSameThread) {
-  base::test::TaskEnvironment task_environment(
-      base::test::SingleThreadTaskEnvironment::MainThreadType::UI);
-
-  std::unique_ptr<ScopedPrefsLock> lock =
-      AcquireGlobalPrefsLock(base::TimeDelta::FromSeconds(0));
-  EXPECT_TRUE(lock);
-
-  std::unique_ptr<ScopedPrefsLock> same_thread_lock =
-      AcquireGlobalPrefsLock(base::TimeDelta::FromSeconds(0));
-  EXPECT_TRUE(same_thread_lock);
-}
-
 TEST(PrefsTest, AcquireGlobalPrefsLock_LockThenTryLockInThreadFail) {
   base::test::TaskEnvironment task_environment(
       base::test::SingleThreadTaskEnvironment::MainThreadType::UI);
@@ -93,6 +78,4 @@ TEST(PrefsTest, AcquireGlobalPrefsLock_TryLockInThreadSuccess) {
   EXPECT_TRUE(lock);
 }
 
-#endif
-
 }  // namespace updater