Bug 1423833: Give WinWindowOcclusionTracker safe shutdown timeout semantics. r=rkraesig

This follows similar shutdown timeout logic as used in
WinCompositorWindowThread. It waits a reasonable amount of time, but will
leak memory "safely" if timeout occurs, without crashing. This behavior is
difficult to trigger in testing, but modifying the
WindowOcclusionCalculator::Shutdown() method to include a long sleep
confirms that the browser will still shutdown without hanging, and will
leak the expected memory.

Differential Revision: https://phabricator.services.mozilla.com/D167464

widget/windows/WinWindowOcclusionTracker.cpp

@@ -18,7 +18,6 @@
 #include "nsThreadUtils.h"
 #include "mozilla/DataMutex.h"
 #include "mozilla/gfx/Logging.h"
-#include "mozilla/layers/SynchronousTask.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/Logging.h"
 #include "mozilla/StaticPrefs_widget.h"
@@ -328,6 +327,9 @@ StaticRefPtr<WinWindowOcclusionTracker> WinWindowOcclusionTracker::sTracker;
 /* static */
 WinWindowOcclusionTracker* WinWindowOcclusionTracker::Get() {
   MOZ_ASSERT(NS_IsMainThread());
+  if (!sTracker || sTracker->mHasAttemptedShutdown) {
+    return nullptr;
+  }
   return sTracker;
 }
 
@@ -336,21 +338,31 @@ void WinWindowOcclusionTracker::Ensure() {
   MOZ_ASSERT(NS_IsMainThread());
   LOG(LogLevel::Info, "WinWindowOcclusionTracker::Ensure()");
 
-  if (sTracker) {
-    return;
-  }
-
-  base::Thread* thread = new base::Thread("WinWindowOcclusionCalc");
-
   base::Thread::Options options;
   options.message_loop_type = MessageLoop::TYPE_UI;
 
+  if (sTracker) {
+    // Try to reuse the thread, which involves stopping and restarting it.
+    sTracker->mThread->Stop();
+    if (sTracker->mThread->StartWithOptions(options)) {
+      // Success!
+      sTracker->mHasAttemptedShutdown = false;
+      return;
+    }
+    // Restart failed, so null out our sTracker and try again with a new
+    // thread. This will cause the old singleton instance to be deallocated,
+    // which will destroy its mThread as well.
+    sTracker = nullptr;
+  }
+
+  UniquePtr<base::Thread> thread =
+      MakeUnique<base::Thread>("WinWindowOcclusionCalc");
+
   if (!thread->StartWithOptions(options)) {
-    delete thread;
     return;
   }
 
-  sTracker = new WinWindowOcclusionTracker(thread);
+  sTracker = new WinWindowOcclusionTracker(std::move(thread));
   WindowOcclusionCalculator::CreateInstance();
 
   RefPtr<Runnable> runnable =
@@ -369,27 +381,50 @@ void WinWindowOcclusionTracker::ShutDown() {
   MOZ_ASSERT(NS_IsMainThread());
   LOG(LogLevel::Info, "WinWindowOcclusionTracker::ShutDown()");
 
+  sTracker->mHasAttemptedShutdown = true;
   sTracker->Destroy();
 
-  // Our shutdown task could hang. Since we're shutting down,
-  // that's not a critical problem. We set a reasonable amount
-  // of time to wait for shutdown, and if it succeeds within
-  // that time, we correctly stop our tracker thread. If it
-  // times out, we just leak the memory and proceed.
-  static const PRIntervalTime TIMEOUT = PR_TicksPerSecond() * 2;
-  layers::SynchronousTask task("WinWindowOcclusionTracker");
-  RefPtr<Runnable> runnable =
-      WrapRunnable(RefPtr<WindowOcclusionCalculator>(
-                       WindowOcclusionCalculator::GetInstance()),
-                   &WindowOcclusionCalculator::Shutdown, &task);
-  OcclusionCalculatorLoop()->PostTask(runnable.forget());
-  nsresult rv = task.Wait(TIMEOUT);
-  if (rv == NS_OK) {
-    sTracker->mThread->Stop();
+  // Our thread could hang while we're waiting for it to stop.
+  // Since we're shutting down, that's not a critical problem.
+  // We set a reasonable amount of time to wait for shutdown,
+  // and if it succeeds within that time, we correctly stop
+  // our thread by nulling out the refptr, which will cause it
+  // to be deallocated and join the thread. If it times out,
+  // we do nothing, which means that the thread will not be
+  // joined and sTracker memory will leak.
+  CVStatus status;
+  {
+    // It's important to hold the lock before posting the
+    // runnable. This ensures that the runnable can't begin
+    // until we've started our Wait, which prevents us from
+    // Waiting on a monitor that has already been notified.
+    MonitorAutoLock lock(sTracker->mMonitor);
+
+    static const TimeDuration TIMEOUT = TimeDuration::FromSeconds(2.0);
+    RefPtr<Runnable> runnable =
+        WrapRunnable(RefPtr<WindowOcclusionCalculator>(
+                         WindowOcclusionCalculator::GetInstance()),
+                     &WindowOcclusionCalculator::Shutdown);
+    OcclusionCalculatorLoop()->PostTask(runnable.forget());
+
+    // Monitor uses SleepConditionVariableSRW, which can have
+    // spurious wakeups which are reported as timeouts, so we
+    // check timestamps to ensure that we've waited as long we
+    // intended to. If we wake early, we don't bother calculating
+    // a precise amount for the next wait; we just wait the same
+    // amount of time. This means timeout might happen after as
+    // much as 2x the TIMEOUT time.
+    TimeStamp timeStart = TimeStamp::NowLoRes();
+    do {
+      status = sTracker->mMonitor.Wait(TIMEOUT);
+    } while ((status == CVStatus::Timeout) &&
+             ((TimeStamp::NowLoRes() - timeStart) < TIMEOUT));
   }
 
-  WindowOcclusionCalculator::ClearInstance();
-  sTracker = nullptr;
+  if (status == CVStatus::NoTimeout) {
+    WindowOcclusionCalculator::ClearInstance();
+    sTracker = nullptr;
+  }
 }
 
 void WinWindowOcclusionTracker::Destroy() {
@@ -486,8 +521,9 @@ void WinWindowOcclusionTracker::OnWindowVisibilityChanged(nsBaseWidget* aWindow,
   mSerializedTaskDispatcher->PostTaskToCalculator(runnable.forget());
 }
 
-WinWindowOcclusionTracker::WinWindowOcclusionTracker(base::Thread* aThread)
-    : mThread(aThread) {
+WinWindowOcclusionTracker::WinWindowOcclusionTracker(
+    UniquePtr<base::Thread> aThread)
+    : mThread(std::move(aThread)), mMonitor("WinWindowOcclusionTracker") {
   MOZ_ASSERT(NS_IsMainThread());
   LOG(LogLevel::Info, "WinWindowOcclusionTracker::WinWindowOcclusionTracker()");
 
@@ -506,7 +542,6 @@ WinWindowOcclusionTracker::~WinWindowOcclusionTracker() {
   MOZ_ASSERT(NS_IsMainThread());
   LOG(LogLevel::Info,
       "WinWindowOcclusionTracker::~WinWindowOcclusionTracker()");
-  delete mThread;
 }
 
 // static
@@ -815,7 +850,8 @@ StaticRefPtr<WinWindowOcclusionTracker::WindowOcclusionCalculator>
     WinWindowOcclusionTracker::WindowOcclusionCalculator::sCalculator;
 
 WinWindowOcclusionTracker::WindowOcclusionCalculator::
-    WindowOcclusionCalculator() {
+    WindowOcclusionCalculator()
+    : mMonitor(WinWindowOcclusionTracker::Get()->mMonitor) {
   MOZ_ASSERT(NS_IsMainThread());
   LOG(LogLevel::Info, "WindowOcclusionCalculator()");
 
@@ -859,19 +895,20 @@ void WinWindowOcclusionTracker::WindowOcclusionCalculator::Initialize() {
 #endif
 }
 
-void WinWindowOcclusionTracker::WindowOcclusionCalculator::Shutdown(
-    layers::SynchronousTask* aTask) {
+void WinWindowOcclusionTracker::WindowOcclusionCalculator::Shutdown() {
+  MonitorAutoLock lock(mMonitor);
+
   MOZ_ASSERT(IsInWinWindowOcclusionThread());
   CALC_LOG(LogLevel::Info, "Shutdown()");
 
-  layers::AutoCompleteTask complete(aTask);
-
   UnregisterEventHooks();
   if (mOcclusionUpdateRunnable) {
     mOcclusionUpdateRunnable->Cancel();
     mOcclusionUpdateRunnable = nullptr;
   }
   mVirtualDesktopManager = nullptr;
+
+  mMonitor.NotifyAll();
 }
 
 void WinWindowOcclusionTracker::WindowOcclusionCalculator::

widget/windows/WinWindowOcclusionTracker.h

@@ -13,6 +13,7 @@
 #include <vector>
 
 #include "nsIWeakReferenceUtils.h"
+#include "mozilla/Monitor.h"
 #include "mozilla/ThreadSafeWeakPtr.h"
 #include "mozilla/widget/WindowOcclusionState.h"
 #include "mozilla/widget/WinEventObserver.h"
@@ -28,10 +29,6 @@ class Thread;
 
 namespace mozilla {
 
-namespace layers {
-class SynchronousTask;
-}
-
 namespace widget {
 
 class OcclusionUpdateRunnable;
@@ -89,7 +86,7 @@ class WinWindowOcclusionTracker final : public DisplayStatusListener,
   friend class ::WinWindowOcclusionTrackerTest;
   friend class ::WinWindowOcclusionTrackerInteractiveTest;
 
-  explicit WinWindowOcclusionTracker(base::Thread* aThread);
+  explicit WinWindowOcclusionTracker(UniquePtr<base::Thread> aThread);
   virtual ~WinWindowOcclusionTracker();
 
   // This class computes the occlusion state of the tracked windows.
@@ -108,7 +105,7 @@ class WinWindowOcclusionTracker final : public DisplayStatusListener,
     static WindowOcclusionCalculator* GetInstance() { return sCalculator; }
 
     void Initialize();
-    void Shutdown(layers::SynchronousTask* aTask);
+    void Shutdown();
 
     void EnableOcclusionTrackingForWindow(HWND hwnd);
     void DisableOcclusionTrackingForWindow(HWND hwnd);
@@ -252,6 +249,10 @@ class WinWindowOcclusionTracker final : public DisplayStatusListener,
     // Used to serialize tasks related to mRootWindowHwndsOcclusionState.
     RefPtr<SerializedTaskDispatcher> mSerializedTaskDispatcher;
 
+    // This is an alias to the singleton WinWindowOcclusionTracker mMonitor,
+    // and is used in ShutDown().
+    Monitor& mMonitor;
+
     friend class OcclusionUpdateRunnable;
   };
 
@@ -290,7 +291,12 @@ class WinWindowOcclusionTracker final : public DisplayStatusListener,
   static StaticRefPtr<WinWindowOcclusionTracker> sTracker;
 
   // "WinWindowOcclusionCalc" thread.
-  base::Thread* const mThread;
+  UniquePtr<base::Thread> mThread;
+  Monitor mMonitor;
+
+  // Has ShutDown been called on us? We might have survived if our thread join
+  // timed out.
+  bool mHasAttemptedShutdown = false;
 
   // Map of HWND to widget. Maintained on main thread, and used to send
   // occlusion state notifications to Windows from