Auto merge of #13472 - asajeffrey:util-remutex-dont-log-while-mutating-lock, r=jdm

Don't log in the middle of mutating a reentrant lock

<!-- Please describe your changes on the following line: -->

Moved assertion that we are the lock owner to after the lock release. The problem is that Servo uses a reentrant lock for logging, so logging in the middle of mutating the lock is A Bad Idea.

While I was at it, I tidied up the reentrant lock code.

cc @jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #13463.
- [X] These changes do not require tests because they fix an existing intermittent.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13472)
<!-- Reviewable:end -->

2 files changed, 27 insertions, 9 deletions

diff --git a/components/util/remutex.rs b/components/util/remutex.rs
index 02dcd884bcd..a574de9cfb3 100644
--- a/components/util/remutex.rs
+++ b/components/util/remutex.rs
@@ -12,7 +12,6 @@
 
 use core::nonzero::NonZero;
 use std::cell::{Cell, UnsafeCell};
-use std::mem;
 use std::ops::Deref;
 use std::sync::{LockResult, Mutex, MutexGuard, PoisonError, TryLockError, TryLockResult};
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -87,13 +86,34 @@ impl HandOverHandMutex {
         }
     }
     #[allow(unsafe_code)]
-    pub fn lock(&self) -> LockResult<()> {
+    unsafe fn set_guard_and_owner<'a>(&'a self, guard: MutexGuard<'a, ()>) {
+        // The following two lines allow us to unsafely store
+        // Some(guard): Option<MutexGuard<'a, ()>
+        // in self.guard, even though its contents are Option<MutexGuard<'static, ()>>,
+        // that is the lifetime is 'a not 'static.
+        let guard_ptr = &mut *(self.guard.get() as *mut u8 as *mut Option<MutexGuard<'a, ()>>);
+        *guard_ptr = Some(guard);
+        self.owner.store(Some(ThreadId::current()), Ordering::Relaxed);
+    }
+    #[allow(unsafe_code)]
+    unsafe fn unset_guard_and_owner(&self) {
+        let guard_ptr = &mut *self.guard.get();
+        let old_owner = self.owner();
+        self.owner.store(None, Ordering::Relaxed);
+        // Make sure we release the lock before checking the assertions.
+        // We protect logging by a re-entrant lock, so we don't want
+        // to do any incidental logging while we the lock is held.
+        drop(guard_ptr.take());
+        // Now we have released the lock, it's okay to use logging.
+        assert_eq!(old_owner, Some(ThreadId::current()));
+    }
+    #[allow(unsafe_code)]
+    pub fn lock<'a>(&'a self) -> LockResult<()> {
         let (guard, result) = match self.mutex.lock() {
             Ok(guard) => (guard, Ok(())),
             Err(err) => (err.into_inner(), Err(PoisonError::new(()))),
         };
-        unsafe { *self.guard.get().as_mut().unwrap() = mem::transmute(guard) };
-        self.owner.store(Some(ThreadId::current()), Ordering::Relaxed);
+        unsafe { self.set_guard_and_owner(guard); }
         result
     }
     #[allow(unsafe_code)]
@@ -103,15 +123,12 @@ impl HandOverHandMutex {
             Err(TryLockError::WouldBlock) => return Err(TryLockError::WouldBlock),
             Err(TryLockError::Poisoned(err)) => (err.into_inner(), Err(TryLockError::Poisoned(PoisonError::new(())))),
         };
-        unsafe { *self.guard.get().as_mut().unwrap() = mem::transmute(guard) };
-        self.owner.store(Some(ThreadId::current()), Ordering::Relaxed);
+        unsafe { self.set_guard_and_owner(guard); }
         result
     }
     #[allow(unsafe_code)]
     pub fn unlock(&self) {
-        assert_eq!(Some(ThreadId::current()), self.owner.load(Ordering::Relaxed));
-        self.owner.store(None, Ordering::Relaxed);
-        unsafe { *self.guard.get().as_mut().unwrap() = None; }
+        unsafe { self.unset_guard_and_owner(); }
     }
     pub fn owner(&self) -> Option<ThreadId> {
         self.owner.load(Ordering::Relaxed)
diff --git a/etc/ci/former_intermittents_css.txt b/etc/ci/former_intermittents_css.txt
index 19abe956cb8..e380032bfa4 100644
--- a/etc/ci/former_intermittents_css.txt
+++ b/etc/ci/former_intermittents_css.txt
@@ -1,3 +1,4 @@
+/css-flexbox-1_dev/html/flexbox-basic-iframe-vert-001.htm
 /css-transforms-1_dev/html/transform-table-007.htm
 /css-transforms-1_dev/html/transform-abspos-002.htm
 /css-transforms-1_dev/html/transform-abspos-007.htm