Fix race condition with SCOPED_MAY_LOAD_LIBRARY_AT_BACKGROUND_PRIORITY

This CL is fixing a race condition that can happen when two threads
are trying to load a DLL at the same time. The problem was that the
priority boost may happen on the wrong thread
(see: https://crbug.com/1124759).

The solution is to add a std::atomic_bool already_loaded and set the
flag when exiting the scope. It is fine that two threads enter the
scope and try to load the DLL at the same time. Both thread will have
and increased thread priority.

R=gab@chromium.org, fdoray@chromium.org

Bug: 1124759
Change-Id: I650ff99f56f1abeba6175024909cf344cad6bc61
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2392746Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805083}

base/threading/scoped_thread_priority.cc

@@ -12,14 +12,19 @@ namespace base {
 namespace internal {
 
 ScopedMayLoadLibraryAtBackgroundPriority::
-    ScopedMayLoadLibraryAtBackgroundPriority(const Location& from_here) {
+    ScopedMayLoadLibraryAtBackgroundPriority(const Location& from_here,
+                                             std::atomic_bool* already_loaded)
+#if defined(OS_WIN)
+    : already_loaded_(already_loaded)
+#endif  // OS_WIN
+{
   TRACE_EVENT_BEGIN2("base", "ScopedMayLoadLibraryAtBackgroundPriority",
                      "file_name", from_here.file_name(), "function_name",
                      from_here.function_name());
-}
-
-bool ScopedMayLoadLibraryAtBackgroundPriority::OnScopeEntered() {
 #if defined(OS_WIN)
+  if (already_loaded_ && already_loaded_->load(std::memory_order_relaxed))
+    return;
+
   const base::ThreadPriority priority =
       PlatformThread::GetCurrentThreadPriority();
   if (priority == base::ThreadPriority::BACKGROUND) {
@@ -31,8 +36,6 @@ bool ScopedMayLoadLibraryAtBackgroundPriority::OnScopeEntered() {
         "ScopedMayLoadLibraryAtBackgroundPriority : Priority Increased");
   }
 #endif  // OS_WIN
-
-  return true;
 }
 
 ScopedMayLoadLibraryAtBackgroundPriority::
@@ -46,6 +49,9 @@ ScopedMayLoadLibraryAtBackgroundPriority::
         "ScopedMayLoadLibraryAtBackgroundPriority : Priority Increased");
     PlatformThread::SetCurrentThreadPriority(original_thread_priority_.value());
   }
+
+  if (already_loaded_)
+    already_loaded_->store(true, std::memory_order_relaxed);
 #endif  // OS_WIN
   TRACE_EVENT_END0("base", "ScopedMayLoadLibraryAtBackgroundPriority");
 }

base/threading/scoped_thread_priority.h

@@ -5,6 +5,8 @@
 #ifndef BASE_THREADING_SCOPED_THREAD_PRIORITY_H_
 #define BASE_THREADING_SCOPED_THREAD_PRIORITY_H_
 
+#include <atomic>
+
 #include "base/base_export.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
@@ -38,24 +40,19 @@ enum class ThreadPriority : int;
 //   }
 //   Bar();
 //
-// The macro raises the thread priority to NORMAL for the scope when first
-// encountered. On Windows, loading a DLL on a background thread can lead to a
-// priority inversion on the loader lock and cause huge janks.
+// The macro raises the thread priority to NORMAL for the scope if no other
+// thread has completed the current scope already (multiple threads can racily
+// begin the initialization and will all be boosted for it). On Windows, loading
+// a DLL on a background thread can lead to a priority inversion on the loader
+// lock and cause huge janks.
 #define SCOPED_MAY_LOAD_LIBRARY_AT_BACKGROUND_PRIORITY()               \
+  static std::atomic_bool INTERNAL_SCOPED_THREAD_PRIORITY_APPEND_LINE( \
+      already_loaded){false};                                          \
   base::internal::ScopedMayLoadLibraryAtBackgroundPriority             \
       INTERNAL_SCOPED_THREAD_PRIORITY_APPEND_LINE(                     \
-          scoped_may_load_library_at_background_priority)(FROM_HERE);      \
-  {                                                                        \
-    /* Thread-safe static local variable initialization ensures that */    \
-    /* OnScopeEntered() is only invoked the first time that this is */     \
-    /* encountered. */                                                     \
-    static bool INTERNAL_SCOPED_THREAD_PRIORITY_APPEND_LINE(invoke_once) = \
-        INTERNAL_SCOPED_THREAD_PRIORITY_APPEND_LINE(                       \
-            scoped_may_load_library_at_background_priority)                \
-            .OnScopeEntered();                                             \
-    ALLOW_UNUSED_LOCAL(                                                    \
-        INTERNAL_SCOPED_THREAD_PRIORITY_APPEND_LINE(invoke_once));         \
-  }
+          scoped_may_load_library_at_background_priority)(             \
+          FROM_HERE,                                                   \
+          &INTERNAL_SCOPED_THREAD_PRIORITY_APPEND_LINE(already_loaded));
 
 // Like SCOPED_MAY_LOAD_LIBRARY_AT_BACKGROUND_PRIORITY, but raises the thread
 // priority every time the scope is entered. Use this around code that may
@@ -64,30 +61,24 @@ enum class ThreadPriority : int;
 #define SCOPED_MAY_LOAD_LIBRARY_AT_BACKGROUND_PRIORITY_REPEATEDLY() \
   base::internal::ScopedMayLoadLibraryAtBackgroundPriority          \
       INTERNAL_SCOPED_THREAD_PRIORITY_APPEND_LINE(                  \
-          scoped_may_load_library_at_background_priority)(FROM_HERE); \
-  {                                                                   \
-    INTERNAL_SCOPED_THREAD_PRIORITY_APPEND_LINE(                      \
-        scoped_may_load_library_at_background_priority)               \
-        .OnScopeEntered();                                            \
-  }
+          scoped_may_load_library_at_background_priority)(FROM_HERE, nullptr);
 
 namespace internal {
 
 class BASE_EXPORT ScopedMayLoadLibraryAtBackgroundPriority {
  public:
-  explicit ScopedMayLoadLibraryAtBackgroundPriority(const Location& from_here);
+  // Boosts thread priority to NORMAL within its scope if |already_loaded| is
+  // nullptr or set to false.
+  explicit ScopedMayLoadLibraryAtBackgroundPriority(
+      const Location& from_here,
+      std::atomic_bool* already_loaded);
   ~ScopedMayLoadLibraryAtBackgroundPriority();
 
-  // The SCOPED_MAY_LOAD_LIBRARY_AT_BACKGROUND_PRIORITY() macro invokes this
-  // the first time that it is encountered. The
-  // SCOPED_MAY_LOAD_LIBRARY_AT_BACKGROUND_PRIORITY_REPEATEDLY() macro invokes
-  // this every time it is encountered.
-  bool OnScopeEntered();
-
  private:
 #if defined(OS_WIN)
-  // The original priority when invoking OnScopeEntered().
+  // The original priority when invoking entering the scope().
   base::Optional<ThreadPriority> original_thread_priority_;
+  std::atomic_bool* const already_loaded_;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(ScopedMayLoadLibraryAtBackgroundPriority);