Remove MojoGetProperty and EDK SetProperty APIs

These have only ever been used to support bindings-level state. There's
no need for the EDK to support this, and no need for the stable ABI to
expose a GetProperty call.

Bug: 819046
Change-Id: I94bdca21045590350f83914fa0429f8a96d7851d
Reviewed-on: https://chromium-review.googlesource.com/1036936Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Commit-Queue: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555245}

content/browser/browser_main_loop.cc

@@ -119,6 +119,7 @@
 #include "media/mojo/buildflags.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/scoped_ipc_support.h"
+#include "mojo/public/cpp/bindings/sync_call_restrictions.h"
 #include "net/base/network_change_notifier.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/ssl/ssl_config_service.h"
@@ -1510,10 +1511,7 @@ void BrowserMainLoop::InitializeMojo() {
     // Disallow mojo sync calls in the browser process. Note that we allow sync
     // calls in single-process mode since renderer IPCs are made from a browser
     // thread.
-    bool sync_call_allowed = false;
-    MojoResult result = mojo::edk::SetProperty(
-        MOJO_PROPERTY_TYPE_SYNC_CALL_ALLOWED, &sync_call_allowed);
-    DCHECK_EQ(MOJO_RESULT_OK, result);
+    mojo::SyncCallRestrictions::DisallowSyncCall();
   }
 
   mojo_ipc_support_.reset(new mojo::edk::ScopedIPCSupport(

mojo/edk/embedder/README.md

@@ -323,13 +323,3 @@ handles, as everything beyond this point - including the passing of other
 interface pipes - can be handled eloquently using
 [public bindings APIs](/mojo#High_Level-Bindings-APIs).
 
-## Setting System Properties
-
-The public Mojo C System API exposes a
-[**`MojoGetProperty`**](/mojo/public/c/system#MojoGetProperty) function for
-querying global, embedder-defined property values. These can be set by calling:
-
-```
-mojo::edk::SetProperty(MojoPropertyType type, const void* value)
-```
-

mojo/edk/embedder/embedder.cc

@@ -64,10 +64,6 @@ MojoResult PassWrappedPlatformHandle(MojoHandle platform_handle_wrapper_handle,
                                                 platform_handle);
 }
 
-MojoResult SetProperty(MojoPropertyType type, const void* value) {
-  return Core::Get()->SetProperty(type, value);
-}
-
 scoped_refptr<base::TaskRunner> GetIOTaskRunner() {
   return Core::Get()->GetNodeController()->io_task_runner();
 }

mojo/edk/embedder/embedder.h

@@ -70,15 +70,6 @@ MOJO_SYSTEM_IMPL_EXPORT MojoResult
 PassWrappedPlatformHandle(MojoHandle platform_handle_wrapper_handle,
                           ScopedPlatformHandle* platform_handle);
 
-// Sets system properties that can be read by the MojoGetProperty() API. See the
-// documentation for MojoPropertyType for supported property types and their
-// corresponding value type.
-//
-// Default property values:
-//   |MOJO_PROPERTY_TYPE_SYNC_CALL_ALLOWED| - true
-MOJO_SYSTEM_IMPL_EXPORT MojoResult SetProperty(MojoPropertyType type,
-                                               const void* value);
-
 // Initialialization/shutdown for interprocess communication (IPC) -------------
 
 // Retrieves the TaskRunner used for IPC I/O, as set by ScopedIPCSupport.

mojo/edk/embedder/entrypoints.cc

@@ -266,10 +266,6 @@ MojoResult MojoNotifyBadMessageImpl(MojoMessageHandle message,
   return g_core->NotifyBadMessage(message, error, error_num_bytes);
 }
 
-MojoResult MojoGetPropertyImpl(MojoPropertyType type, void* value) {
-  return g_core->GetProperty(type, value);
-}
-
 }  // extern "C"
 
 MojoSystemThunks g_thunks = {sizeof(MojoSystemThunks),
@@ -308,8 +304,7 @@ MojoSystemThunks g_thunks = {sizeof(MojoSystemThunks),
                              MojoUnwrapPlatformHandleImpl,
                              MojoWrapPlatformSharedBufferHandleImpl,
                              MojoUnwrapPlatformSharedBufferHandleImpl,
-                             MojoNotifyBadMessageImpl,
-                             MojoGetPropertyImpl};
+                             MojoNotifyBadMessageImpl};
 
 }  // namespace
 

mojo/edk/system/core.cc

@@ -330,17 +330,6 @@ MojoHandle Core::ExtractMessagePipeFromInvitation(const std::string& name) {
   return handle;
 }
 
-MojoResult Core::SetProperty(MojoPropertyType type, const void* value) {
-  base::AutoLock locker(property_lock_);
-  switch (type) {
-    case MOJO_PROPERTY_TYPE_SYNC_CALL_ALLOWED:
-      property_sync_call_allowed_ = *static_cast<const bool*>(value);
-      return MOJO_RESULT_OK;
-    default:
-      return MOJO_RESULT_INVALID_ARGUMENT;
-  }
-}
-
 MojoTimeTicks Core::GetTimeTicksNow() {
   return base::TimeTicks::Now().ToInternalValue();
 }
@@ -583,17 +572,6 @@ MojoResult Core::GetMessageContext(MojoMessageHandle message_handle,
   return MOJO_RESULT_OK;
 }
 
-MojoResult Core::GetProperty(MojoPropertyType type, void* value) {
-  base::AutoLock locker(property_lock_);
-  switch (type) {
-    case MOJO_PROPERTY_TYPE_SYNC_CALL_ALLOWED:
-      *static_cast<bool*>(value) = property_sync_call_allowed_;
-      return MOJO_RESULT_OK;
-    default:
-      return MOJO_RESULT_INVALID_ARGUMENT;
-  }
-}
-
 MojoResult Core::CreateMessagePipe(const MojoCreateMessagePipeOptions* options,
                                    MojoHandle* message_pipe_handle0,
                                    MojoHandle* message_pipe_handle1) {

mojo/edk/system/core.h

@@ -153,8 +153,6 @@ class MOJO_SYSTEM_IMPL_EXPORT Core {
   // may be called from any thread. Beware!
   void RequestShutdown(const base::Closure& callback);
 
-  MojoResult SetProperty(MojoPropertyType type, const void* value);
-
   // ---------------------------------------------------------------------------
 
   // The following methods are essentially implementations of the Mojo Core
@@ -213,7 +211,6 @@ class MOJO_SYSTEM_IMPL_EXPORT Core {
   MojoResult GetMessageContext(MojoMessageHandle message_handle,
                                const MojoGetMessageContextOptions* options,
                                uintptr_t* context);
-  MojoResult GetProperty(MojoPropertyType type, void* value);
 
   // These methods correspond to the API functions defined in
   // "mojo/public/c/system/message_pipe.h":
@@ -329,10 +326,6 @@ class MOJO_SYSTEM_IMPL_EXPORT Core {
       std::unordered_map<void*, std::unique_ptr<PlatformSharedMemoryMapping>>;
   MappingTable mapping_table_;
 
-  base::Lock property_lock_;
-  // Properties that can be read using the MojoGetProperty() API.
-  bool property_sync_call_allowed_ = true;
-
   DISALLOW_COPY_AND_ASSIGN(Core);
 };
 

mojo/public/c/system/functions.h

@@ -77,16 +77,6 @@ MOJO_SYSTEM_EXPORT MojoResult
 MojoQueryHandleSignalsState(MojoHandle handle,
                             struct MojoHandleSignalsState* signals_state);
 
-// Retrieves system properties. See the documentation for |MojoPropertyType| for
-// supported property types and their corresponding output value type.
-//
-// Returns:
-//     |MOJO_RESULT_OK| on success.
-//     |MOJO_RESULT_INVALID_ARGUMENT| if |type| is not recognized. In this case,
-//         |value| is untouched.
-MOJO_SYSTEM_EXPORT MojoResult MojoGetProperty(MojoPropertyType type,
-                                              void* value);
-
 #ifdef __cplusplus
 }  // extern "C"
 #endif

mojo/public/c/system/thunks.cc

@@ -340,10 +340,6 @@ MojoResult MojoNotifyBadMessage(MojoMessageHandle message,
   return g_thunks.NotifyBadMessage(message, error, error_num_bytes);
 }
 
-MojoResult MojoGetProperty(MojoPropertyType type, void* value) {
-  return g_thunks.GetProperty(type, value);
-}
-
 }  // extern "C"
 
 void MojoEmbedderSetSystemThunks(const MojoSystemThunks* thunks) {

mojo/public/c/system/thunks.h

@@ -146,7 +146,6 @@ struct MojoSystemThunks {
   MojoResult (*NotifyBadMessage)(MojoMessageHandle message,
                                  const char* error,
                                  size_t error_num_bytes);
-  MojoResult (*GetProperty)(MojoPropertyType type, void* value);
 };
 #pragma pack(pop)
 

mojo/public/c/system/types.h

@@ -206,20 +206,6 @@ struct MOJO_ALIGNAS(4) MojoHandleSignalsState {
 MOJO_STATIC_ASSERT(sizeof(MojoHandleSignalsState) == 8,
                    "MojoHandleSignalsState has wrong size");
 
-// |MojoPropertyType|: Property types that can be passed to |MojoGetProperty()|
-// to retrieve system properties. May take the following values:
-//   |MOJO_PROPERTY_TYPE_SYNC_CALL_ALLOWED| - Whether making synchronous calls
-//       (i.e., blocking to wait for a response to an outbound message) is
-//       allowed. The property value is of boolean type. If the value is true,
-//       users should refrain from making sync calls.
-typedef uint32_t MojoPropertyType;
-
-#ifdef __cplusplus
-const MojoPropertyType MOJO_PROPERTY_TYPE_SYNC_CALL_ALLOWED = 0;
-#else
-#define MOJO_PROPERTY_TYPE_SYNC_CALL_ALLOWED ((MojoPropertyType)0)
-#endif
-
 // TODO(https://crbug.com/819046): Remove these aliases.
 #define MOJO_WATCH_CONDITION_SATISFIED MOJO_TRIGGER_CONDITION_SIGNALS_SATISFIED
 #define MOJO_WATCH_CONDITION_NOT_SATISFIED \

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

@@ -7,85 +7,79 @@
 #if ENABLE_SYNC_CALL_RESTRICTIONS
 
 #include "base/debug/leak_annotations.h"
-#include "base/lazy_instance.h"
 #include "base/logging.h"
-#include "base/threading/thread_local.h"
+#include "base/macros.h"
+#include "base/no_destructor.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/sequence_local_storage_slot.h"
 #include "mojo/public/c/system/core.h"
 
 namespace mojo {
 
 namespace {
 
-class SyncCallSettings {
+class GlobalSyncCallSettings {
  public:
-  static SyncCallSettings* current();
+  GlobalSyncCallSettings() = default;
+  ~GlobalSyncCallSettings() = default;
 
-  bool allowed() const {
-    return scoped_allow_count_ > 0 || system_defined_value_;
+  bool sync_call_allowed_by_default() const {
+    base::AutoLock lock(lock_);
+    return sync_call_allowed_by_default_;
   }
 
-  void IncreaseScopedAllowCount() { scoped_allow_count_++; }
-  void DecreaseScopedAllowCount() {
-    DCHECK_LT(0u, scoped_allow_count_);
-    scoped_allow_count_--;
+  void DisallowSyncCallByDefault() {
+    base::AutoLock lock(lock_);
+    sync_call_allowed_by_default_ = false;
   }
 
  private:
-  SyncCallSettings();
-  ~SyncCallSettings();
+  mutable base::Lock lock_;
+  bool sync_call_allowed_by_default_ = true;
 
-  bool system_defined_value_ = true;
-  size_t scoped_allow_count_ = 0;
+  DISALLOW_COPY_AND_ASSIGN(GlobalSyncCallSettings);
 };
 
-base::LazyInstance<base::ThreadLocalPointer<SyncCallSettings>>::Leaky
-    g_sync_call_settings = LAZY_INSTANCE_INITIALIZER;
-
-// static
-SyncCallSettings* SyncCallSettings::current() {
-  SyncCallSettings* result = g_sync_call_settings.Pointer()->Get();
-  if (!result) {
-    result = new SyncCallSettings();
-    ANNOTATE_LEAKING_OBJECT_PTR(result);
-    DCHECK_EQ(result, g_sync_call_settings.Pointer()->Get());
-  }
-  return result;
-}
-
-SyncCallSettings::SyncCallSettings() {
-  MojoResult result = MojoGetProperty(MOJO_PROPERTY_TYPE_SYNC_CALL_ALLOWED,
-                                      &system_defined_value_);
-  DCHECK_EQ(MOJO_RESULT_OK, result);
-
-  DCHECK(!g_sync_call_settings.Pointer()->Get());
-  g_sync_call_settings.Pointer()->Set(this);
+GlobalSyncCallSettings& GetGlobalSettings() {
+  static base::NoDestructor<GlobalSyncCallSettings> global_settings;
+  return *global_settings;
 }
 
-SyncCallSettings::~SyncCallSettings() {
-  g_sync_call_settings.Pointer()->Set(nullptr);
+size_t& GetSequenceLocalScopedAllowCount() {
+  static base::NoDestructor<base::SequenceLocalStorageSlot<size_t>> count;
+  return count->Get();
 }
 
 }  // namespace
 
 // static
 void SyncCallRestrictions::AssertSyncCallAllowed() {
-  if (!SyncCallSettings::current()->allowed()) {
-      LOG(FATAL) << "Mojo sync calls are not allowed in this process because "
-                 << "they can lead to jank and deadlock. If you must make an "
-                 << "exception, please see "
-                 << "SyncCallRestrictions::ScopedAllowSyncCall and consult "
-                 << "mojo/OWNERS.";
-  }
+  if (GetGlobalSettings().sync_call_allowed_by_default())
+    return;
+  if (GetSequenceLocalScopedAllowCount() > 0)
+    return;
+
+  LOG(FATAL) << "Mojo sync calls are not allowed in this process because "
+             << "they can lead to jank and deadlock. If you must make an "
+             << "exception, please see "
+             << "SyncCallRestrictions::ScopedAllowSyncCall and consult "
+             << "mojo/OWNERS.";
+}
+
+// static
+void SyncCallRestrictions::DisallowSyncCall() {
+  GetGlobalSettings().DisallowSyncCallByDefault();
 }
 
 // static
 void SyncCallRestrictions::IncreaseScopedAllowCount() {
-  SyncCallSettings::current()->IncreaseScopedAllowCount();
+  ++GetSequenceLocalScopedAllowCount();
 }
 
 // static
 void SyncCallRestrictions::DecreaseScopedAllowCount() {
-  SyncCallSettings::current()->DecreaseScopedAllowCount();
+  DCHECK_GT(GetSequenceLocalScopedAllowCount(), 0u);
+  --GetSequenceLocalScopedAllowCount();
 }
 
 }  // namespace mojo

mojo/public/cpp/bindings/sync_call_restrictions.h

@@ -55,20 +55,29 @@ class ScopedAllowSyncCallForTesting;
 //
 // Before processing a sync call, the bindings call
 // SyncCallRestrictions::AssertSyncCallAllowed() to check whether sync calls are
-// allowed. By default, it is determined by the mojo system property
-// MOJO_PROPERTY_TYPE_SYNC_CALL_ALLOWED. If the default setting says no but you
-// have a very compelling reason to disregard that (which should be very very
-// rare), you can override it by constructing a ScopedAllowSyncCall object,
-// which allows making sync calls on the current sequence during its lifetime.
+// allowed. By default sync calls are allowed but they may be globally
+// disallowed within a process by calling DisallowSyncCall().
+//
+// If globally disallowed but you but you have a very compelling reason to
+// disregard that (which should be very very rare), you can override it by
+// constructing a ScopedAllowSyncCall object which allows making sync calls on
+// the current sequence during its lifetime.
 class MOJO_CPP_BINDINGS_EXPORT SyncCallRestrictions {
  public:
 #if ENABLE_SYNC_CALL_RESTRICTIONS
   // Checks whether the current sequence is allowed to make sync calls, and
   // causes a DCHECK if not.
   static void AssertSyncCallAllowed();
+
+  // Disables sync calls within the calling process. Any caller who wishes to
+  // make sync calls once this has been invoked must do so within the extent of
+  // a ScopedAllowSyncCall or ScopedAllowSyncCallForTesting.
+  static void DisallowSyncCall();
+
 #else
   // Inline the empty definitions of functions so that they can be compiled out.
   static void AssertSyncCallAllowed() {}
+  static void DisallowSyncCall() {}
 #endif
 
  private: