[SAB] Document and tighten FutexEmulation `mutex_` scope

Document what pieces of data the global `FutexEmulation::mutex_`
mutex protects from concurrent access, and reduce the scope
in which said mutex is locked during `FutexEmulation::Wait()`
to match that description more closely.

Change-Id: I0764efabac06814d83ed5c4af4eb7da34af47cab
Reviewed-on: https://chromium-review.googlesource.com/1074689
Commit-Queue: Ben Smith <binji@chromium.org>
Reviewed-by: Ben Smith <binji@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53429}

src/futex-emulation.cc

@@ -81,8 +81,6 @@ Object* FutexEmulation::Wait(Isolate* isolate,
   int32_t* p =
       reinterpret_cast<int32_t*>(static_cast<int8_t*>(backing_store) + addr);
 
-  base::LockGuard<base::Mutex> lock_guard(mutex_.Pointer());
-
   if (*p != value) {
     return isolate->heap()->not_equal();
   }
@@ -116,10 +114,12 @@ Object* FutexEmulation::Wait(Isolate* isolate,
   base::TimeTicks timeout_time = start_time + rel_timeout;
   base::TimeTicks current_time = start_time;
 
-  wait_list_.Pointer()->AddNode(node);
-
   Object* result;
 
+  {
+    base::LockGuard<base::Mutex> lock_guard(mutex_.Pointer());
+    wait_list_.Pointer()->AddNode(node);
+
     while (true) {
       bool interrupted = node->interrupted_;
       node->interrupted_ = false;
@@ -135,8 +135,8 @@ Object* FutexEmulation::Wait(Isolate* isolate,
       // 2) After interrupted has been checked here, but before mutex_ is
       //    acquired: interrupted is checked again below, with mutex_ locked.
       //    Because the wakeup signal also acquires mutex_, we know it will not
-    //    be able to notify until mutex_ is released below, when waiting on the
-    //    condition variable.
+      //    be able to notify until mutex_ is released below, when waiting on
+      //    the condition variable.
       // 3) After the mutex is released in the call to WaitFor(): this
       // notification will wake up the condition variable. node->waiting() will
       // be false, so we'll loop and then check interrupts.
@@ -182,6 +182,8 @@ Object* FutexEmulation::Wait(Isolate* isolate,
     }
 
     wait_list_.Pointer()->RemoveNode(node);
+  }
+
   node->waiting_ = false;
 
   return result;

src/futex-emulation.h

@@ -52,10 +52,13 @@ class FutexWaitListNode {
   friend class FutexWaitList;
 
   base::ConditionVariable cond_;
+  // prev_ and next_ are protected by FutexEmulation::mutex_.
   FutexWaitListNode* prev_;
   FutexWaitListNode* next_;
   void* backing_store_;
   size_t wait_addr_;
+  // waiting_ and interrupted_ are protected by FutexEmulation::mutex_
+  // if this node is currently contained in FutexEmulation::wait_list_.
   bool waiting_;
   bool interrupted_;
 
@@ -110,6 +113,11 @@ class FutexEmulation : public AllStatic {
  private:
   friend class FutexWaitListNode;
 
+  // `mutex_` protects the composition of `wait_list_` (i.e. no elements may be
+  // added or removed without holding this mutex), as well as the `waiting_`
+  // and `interrupted_` fields for each individual list node that is currently
+  // part of the list. It must be the mutex used together with the `cond_`
+  // condition variable of such nodes.
   static base::LazyMutex mutex_;
   static base::LazyInstance<FutexWaitList>::type wait_list_;
 };