Mojo: Properly assert sync call restrictions

For off-thread sync IPC waits on [NoInterrupt] messages, we were
intentionally not asserting sync call restrictions. Instead we rely on
the implicit assertion that base sync primitives are allowed. This was
done due to the fact that some important GPU sync IPCs are sent during
thread teardown (not process shutdown) in renderers, and asserting sync
call restrictions relies on sequence-local storage which may already be
torn down by the time it's needed.

Ideally callers would not need to know about the distinction though,
and they should be able to rely on ScopedAllowSyncCall for all cases.

This CL addresses the issue by making Mojo's sync call restriction
state management and assertion operations silently do nothing when
sequence-local storage is no valid for the calling thread.

Bug: 1196476
Change-Id: I24c2a726662ba0b18faa2fb3b97f33e13422f854
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2856822
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Commit-Queue: Zhenyao Mo <zmo@chromium.org>
Auto-Submit: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#877506}

base/threading/sequence_local_storage_map.cc

@@ -34,6 +34,7 @@ ScopedSetSequenceLocalStorageMapForCurrentThread::
   tls_current_sequence_local_storage.Get().Set(nullptr);
 }
 
+// static
 SequenceLocalStorageMap& SequenceLocalStorageMap::GetForCurrentThread() {
   SequenceLocalStorageMap* current_sequence_local_storage =
       tls_current_sequence_local_storage.Get().Get();
@@ -47,6 +48,11 @@ SequenceLocalStorageMap& SequenceLocalStorageMap::GetForCurrentThread() {
   return *current_sequence_local_storage;
 }
 
+// static
+bool SequenceLocalStorageMap::IsSetForCurrentThread() {
+  return tls_current_sequence_local_storage.Get().Get() != nullptr;
+}
+
 void* SequenceLocalStorageMap::Get(int slot_id) {
   const auto it = sls_map_.find(slot_id);
   if (it == sls_map_.end())

base/threading/sequence_local_storage_map.h

@@ -31,6 +31,11 @@ class BASE_EXPORT SequenceLocalStorageMap {
   // ScopedSetSequenceLocalStorageForCurrentThread.
   static SequenceLocalStorageMap& GetForCurrentThread();
 
+  // Indicates whether the current thread has a SequenceLocalStorageMap
+  // available and thus whether it can safely call GetForCurrentThread and
+  // dereference SequenceLocalStorageSlots.
+  static bool IsSetForCurrentThread();
+
   // Holds a pointer to a value alongside a destructor for this pointer.
   // Calls the destructor on the value upon destruction.
   class BASE_EXPORT ValueDestructorPair {

gpu/ipc/client/gpu_channel_host.cc

@@ -175,7 +175,7 @@ void GpuChannelHost::VerifyFlush(uint32_t deferred_message_id) {
   InternalFlush(deferred_message_id);
 
   if (deferred_message_id > verified_deferred_message_id_) {
-    base::ScopedAllowBaseSyncPrimitivesOutsideBlockingScope allow_wait;
+    mojo::SyncCallRestrictions::ScopedAllowSyncCall allow_sync;
     GetGpuChannel().Flush();
     verified_deferred_message_id_ = flushed_deferred_message_id_;
   }

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

@@ -381,10 +381,11 @@ void ThreadSafeInterfaceEndpointClientProxy::SendMessageWithResponder(
     sync_calls->pending_responses.push_back(response.get());
   }
 
+  SyncCallRestrictions::AssertSyncCallAllowed();
+
   if (allow_interrupt) {
     // In the common case where interrupts are allowed, we watch cooperatively
     // with other potential endpoints on the same thread.
-    SyncCallRestrictions::AssertSyncCallAllowed();
     bool signaled = false;
     auto set_flag = [](bool* flag) { *flag = true; };
     SyncEventWatcher watcher(&response->event,

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

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "base/no_destructor.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/sequence_local_storage_map.h"
 #include "base/threading/sequence_local_storage_slot.h"
 #include "mojo/public/c/system/core.h"
 
@@ -50,12 +51,23 @@ size_t& GetSequenceLocalScopedAllowCount() {
   return count->GetOrCreateValue();
 }
 
+// Sometimes sync calls need to be made while sequence-local storage is not
+// initialized. In particular this can occur during thread tear-down while TLS
+// objects (including SequenceLocalStorageMap itself) are being destroyed. We
+// can't track a sequence-local policy in such cases, so we don't enforce one.
+bool SyncCallRestrictionsEnforceable() {
+  return base::internal::SequenceLocalStorageMap::IsSetForCurrentThread();
+}
+
 }  // namespace
 
 // static
 void SyncCallRestrictions::AssertSyncCallAllowed() {
-  if (GetGlobalSettings().sync_call_allowed_by_default())
+  if (GetGlobalSettings().sync_call_allowed_by_default() ||
+      !SyncCallRestrictionsEnforceable()) {
     return;
+  }
+
   if (GetSequenceLocalScopedAllowCount() > 0)
     return;
 
@@ -73,11 +85,17 @@ void SyncCallRestrictions::DisallowSyncCall() {
 
 // static
 void SyncCallRestrictions::IncreaseScopedAllowCount() {
+  if (!SyncCallRestrictionsEnforceable())
+    return;
+
   ++GetSequenceLocalScopedAllowCount();
 }
 
 // static
 void SyncCallRestrictions::DecreaseScopedAllowCount() {
+  if (!SyncCallRestrictionsEnforceable())
+    return;
+
   DCHECK_GT(GetSequenceLocalScopedAllowCount(), 0u);
   --GetSequenceLocalScopedAllowCount();
 }