From 8e93ce8929a77b8d25b1dec32686b772a3f49b49 Mon Sep 17 00:00:00 2001
From: Tyler Veness <calcmogul@gmail.com>
Date: Sat, 3 Aug 2019 14:58:10 -0700
Subject: [PATCH] Fix PIDControllerRunner member destruction order (#1801)

The mutexes in PIDControllerRunner are declared after the Notifier, and
when the PIDControllerRunner object is destructed, the member object
destructors are called in the reverse order in which they are declared.
The mutexes are destructed first, then the Notifier destructor is called
which stops the Notifier.

There's a window between those destructor calls during which the
Notifier can run the callable and attempt to lock a mutex that no longer
exists.

Declaring the Notifier after all the variables its callable uses fixes
this issue, as it ensures the Notifier is destructed first.
---
 .../native/include/frc/controller/PIDControllerRunner.h    | 7 ++++++-
 .../wpi/first/wpilibj/controller/PIDControllerRunner.java  | 7 ++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/wpilibc/src/main/native/include/frc/controller/PIDControllerRunner.h b/wpilibc/src/main/native/include/frc/controller/PIDControllerRunner.h
index aa47ca73a0..bcfacb9aab 100644
--- a/wpilibc/src/main/native/include/frc/controller/PIDControllerRunner.h
+++ b/wpilibc/src/main/native/include/frc/controller/PIDControllerRunner.h
@@ -55,7 +55,6 @@ class PIDControllerRunner : SendableBase {
   void InitSendable(SendableBuilder& builder) override;
 
  private:
-  Notifier m_notifier{&PIDControllerRunner::Run, this};
   frc2::PIDController& m_controller;
   std::function<double(void)> m_measurementSource;
   std::function<void(double)> m_controllerOutput;
@@ -67,6 +66,12 @@ class PIDControllerRunner : SendableBase {
   // Controller::Update() is already running at that time.
   mutable wpi::mutex m_outputMutex;
 
+  // This is declared after all other member variables so that during
+  // PIDControllerRunner destruction, the Notifier is stopped before any member
+  // variables its callable uses are destructed. This avoids use-after-free
+  // bugs like crashes when locking is attempted on deallocated mutexes.
+  Notifier m_notifier{&PIDControllerRunner::Run, this};
+
   void Run();
 };
 
diff --git a/wpilibj/src/main/java/edu/wpi/first/wpilibj/controller/PIDControllerRunner.java b/wpilibj/src/main/java/edu/wpi/first/wpilibj/controller/PIDControllerRunner.java
index 7d6edb866d..fa4b22a37c 100644
--- a/wpilibj/src/main/java/edu/wpi/first/wpilibj/controller/PIDControllerRunner.java
+++ b/wpilibj/src/main/java/edu/wpi/first/wpilibj/controller/PIDControllerRunner.java
@@ -16,7 +16,6 @@ import edu.wpi.first.wpilibj.SendableBase;
 import edu.wpi.first.wpilibj.smartdashboard.SendableBuilder;
 
 public class PIDControllerRunner extends SendableBase {
-  private final Notifier m_notifier = new Notifier(this::run);
   private final PIDController m_controller;
   private final DoubleConsumer m_controllerOutput;
   private final DoubleSupplier m_measurementSource;
@@ -28,6 +27,12 @@ public class PIDControllerRunner extends SendableBase {
   // Controller.update() is already running at that time.
   private final ReentrantLock m_outputMutex = new ReentrantLock();
 
+  // This is declared after all other member variables so that during
+  // PIDControllerRunner destruction, the Notifier is stopped before any member
+  // variables its callable uses are destructed. This avoids use-after-free
+  // bugs like crashes when locking is attempted on deallocated mutexes.
+  private final Notifier m_notifier = new Notifier(this::run);
+
   /**
    * Allocates a PIDControllerRunner.
    *