Thread: Cleanup m_blocker handling

The only two places we set m_blocker now are Thread::set_state(), and
Thread::block(). set_state is mostly just an issue of clarity: we don't
want to end up with state() != Blocked with an m_blocker, because that's
weird. It's also possible: if we yield, someone else may set_state() us.

We also now set_state() and set m_blocker under lock in block(), rather
than unlocking which might allow someone else to mess with our internals
while we're in the process of trying to block.

This seems to fix sending STOP & CONT causing a panic.

My guess as to what was happening is this:

    thread A blocks in select(): Blocking & m_blocker != nullptr
    thread B sends SIGSTOP: Stopped & m_blocker != nullptr
    thread B sends SIGCONT: we continue execution. Runnable & m_blocker != nullptr
    thread A tries to block in select() again:
        * sets m_blocker
        * unlocks (in block_helper)
        * someone else tries to unblock us? maybe from the old m_blocker? unclear -- clears m_blocker
        * sets Blocked (while unlocked!)

So, thread A is left with state Blocked & m_blocker == nullptr, leading
to the scheduler assert (m_blocker != nullptr) failing.

Long story short, let's do all our data management with the lock _held_.

Kernel/Scheduler.cpp

@@ -222,10 +222,8 @@ void Thread::consider_unblock(time_t now_sec, long now_usec)
         return;
     case Thread::Blocked:
         ASSERT(m_blocker);
-        if (m_blocker->should_unblock(*this, now_sec, now_usec)) {
+        if (m_blocker->should_unblock(*this, now_sec, now_usec))
             unblock();
-            m_blocker = nullptr;
-        }
         return;
     case Thread::Skip1SchedulerPass:
         set_state(Thread::Skip0SchedulerPasses);

Kernel/Thread.cpp

@@ -97,7 +97,6 @@ Thread::~Thread()
 
 void Thread::unblock()
 {
-    m_blocker = nullptr;
     if (current == this) {
         set_state(Thread::Running);
         return;
@@ -108,9 +107,11 @@ void Thread::unblock()
 
 void Thread::block_helper()
 {
+    // This function mostly exists to avoid circular header dependencies. If
+    // anything needs adding, think carefully about whether it belongs in
+    // block() instead. Remember that we're unlocking here, so be very careful
+    // about altering any state once we're unlocked!
     bool did_unlock = process().big_lock().unlock_if_locked();
-    ASSERT(state() == Thread::Running);
-    set_state(Thread::Blocked);
     Scheduler::yield();
     if (did_unlock)
         process().big_lock().lock();
@@ -160,8 +161,6 @@ void Thread::finalize()
     dbgprintf("Finalizing Thread %u in %s(%u)\n", tid(), m_process.name().characters(), pid());
     set_state(Thread::State::Dead);
 
-    m_blocker = nullptr;
-
     if (this == &m_process.main_thread())
         m_process.finalize();
 }
@@ -539,6 +538,11 @@ bool Thread::is_thread(void* ptr)
 void Thread::set_state(State new_state)
 {
     InterruptDisabler disabler;
+    if (new_state == Blocked) {
+        // we should always have an m_blocker while blocked
+        ASSERT(m_blocker != nullptr);
+    }
+
     m_state = new_state;
     if (m_process.pid() != 0) {
         SchedulerThreadList* list = nullptr;

Kernel/Thread.h

@@ -217,10 +217,19 @@ public:
     template <typename T, class... Args>
     [[nodiscard]] BlockResult block(Args&& ... args)
     {
-        ASSERT(!m_blocker);
+        // If this is triggered, state has gotten messed: we should never have a
+        // blocker already set. That means we're re-entering somehow, which is
+        // bad.
+        ASSERT(m_blocker == nullptr);
+
+        // We should never be blocking a blocked (or otherwise non-active) thread.
+        ASSERT(state() == Thread::Running);
         T t(AK::forward<Args>(args)...);
         m_blocker = &t;
-        block_helper();
+        set_state(Thread::Blocked);
+        block_helper(); // this will unlock, yield, and eventually unblock us to return here.
+        ASSERT(state() != Thread::Blocked);
+        m_blocker = nullptr;
         if (t.was_interrupted_by_signal())
             return BlockResult::InterruptedBySignal;
         return BlockResult::WokeNormally;