Bug 891035: Provide mutual exclusion in Plugin Hang UI to protect against concurrent shutdown and user response. r=bsmedberg

--HG--
extra : rebase_source : c7960c2d74af8af57aa7e97867df61a975f4d7fe

dom/plugins/ipc/PluginHangUIParent.cpp

@@ -71,7 +71,8 @@ namespace plugins {
 PluginHangUIParent::PluginHangUIParent(PluginModuleParent* aModule,
                                        const int32_t aHangUITimeoutPref,
                                        const int32_t aChildTimeoutPref)
-  : mModule(aModule),
+  : mMutex("mozilla::plugins::PluginHangUIParent::mMutex"),
+    mModule(aModule),
     mTimeoutPrefMs(static_cast<uint32_t>(aHangUITimeoutPref) * 1000U),
     mIPCTimeoutMs(static_cast<uint32_t>(aChildTimeoutPref) * 1000U),
     mMainThreadMessageLoop(MessageLoop::current()),
@@ -88,8 +89,9 @@ PluginHangUIParent::PluginHangUIParent(PluginModuleParent* aModule,
 
 PluginHangUIParent::~PluginHangUIParent()
 {
-  if (mRegWait) {
-    ::UnregisterWaitEx(mRegWait, INVALID_HANDLE_VALUE);
+  { // Scope for lock
+    MutexAutoLock lock(mMutex);
+    UnwatchHangUIChildProcess(true);
   }
   if (mShowEvent) {
     ::CloseHandle(mShowEvent);
@@ -227,6 +229,7 @@ PluginHangUIParent::Init(const nsString& aPluginName)
   }
   mShowEvent = showEvent.Get();
 
+  MutexAutoLock lock(mMutex);
   STARTUPINFO startupInfo = { sizeof(STARTUPINFO) };
   PROCESS_INFORMATION processInfo = { NULL };
   BOOL isProcessCreated = ::CreateProcess(exePath.value().c_str(),
@@ -266,6 +269,7 @@ VOID CALLBACK PluginHangUIParent::SOnHangUIProcessExit(PVOID aContext,
                                                        BOOLEAN aIsTimer)
 {
   PluginHangUIParent* object = static_cast<PluginHangUIParent*>(aContext);
+  MutexAutoLock lock(object->mMutex);
   // If the Hang UI child process died unexpectedly, act as if the UI cancelled
   if (object->IsShowing()) {
     object->RecvUserResponse(HANGUI_USER_RESPONSE_CANCEL);
@@ -275,9 +279,44 @@ VOID CALLBACK PluginHangUIParent::SOnHangUIProcessExit(PVOID aContext,
   }
 }
 
+// A precondition for this function is that the caller has locked mMutex
+bool
+PluginHangUIParent::UnwatchHangUIChildProcess(bool aWait)
+{
+  mMutex.AssertCurrentThreadOwns();
+  if (mRegWait) {
+    // If aWait is false then we want to pass a NULL (i.e. default constructor)
+    // completionEvent
+    ScopedHandle completionEvent;
+    if (aWait) {
+      completionEvent.Set(::CreateEvent(NULL, FALSE, FALSE, NULL));
+      if (!completionEvent.IsValid()) {
+        return false;
+      }
+    }
+
+    // if aWait == false and UnregisterWaitEx fails with ERROR_IO_PENDING,
+    // it is okay to clear mRegWait; Windows is telling us that the wait's
+    // callback is running but will be cleaned up once the callback returns.
+    if (::UnregisterWaitEx(mRegWait, completionEvent) ||
+        !aWait && ::GetLastError() == ERROR_IO_PENDING) {
+      mRegWait = NULL;
+      if (aWait) {
+        // We must temporarily unlock mMutex while waiting for the registered
+        // wait callback to complete, or else we could deadlock.
+        MutexAutoUnlock unlock(mMutex);
+        ::WaitForSingleObject(completionEvent, INFINITE);
+      }
+      return true;
+    }
+  }
+  return false;
+}
+
 bool
 PluginHangUIParent::Cancel()
 {
+  MutexAutoLock lock(mMutex);
   bool result = mIsShowing && SendCancel();
   if (result) {
     mIsShowing = false;
@@ -297,15 +336,17 @@ PluginHangUIParent::SendCancel()
   return NS_SUCCEEDED(mMiniShm.Send());
 }
 
+// A precondition for this function is that the caller has locked mMutex
 bool
 PluginHangUIParent::RecvUserResponse(const unsigned int& aResponse)
 {
+  mMutex.AssertCurrentThreadOwns();
   if (!mIsShowing && !(aResponse & HANGUI_USER_RESPONSE_CANCEL)) {
     // Don't process a user response if a cancellation is already pending
     return true;
   }
   mLastUserResponse = aResponse;
-  mResponseTicks = GetTickCount();
+  mResponseTicks = ::GetTickCount();
   mIsShowing = false;
   // responseCode: 1 = Stop, 2 = Continue, 3 = Cancel
   int responseCode;
@@ -370,10 +411,8 @@ PluginHangUIParent::OnMiniShmEvent(MiniShmBase *aMiniShmObj)
   if (NS_SUCCEEDED(rv)) {
     // The child process has returned a response so we shouldn't worry about 
     // its state anymore.
-    if (::UnregisterWaitEx(mRegWait, NULL)) {
-      mRegWait = NULL;
-    }
-
+    MutexAutoLock lock(mMutex);
+    UnwatchHangUIChildProcess(false);
     RecvUserResponse(response->mResponseBits);
   }
 }
@@ -390,7 +429,7 @@ PluginHangUIParent::OnMiniShmConnect(MiniShmBase* aMiniShmObj)
   }
   cmd->mCode = PluginHangUICommand::HANGUI_CMD_SHOW;
   if (NS_SUCCEEDED(aMiniShmObj->Send())) {
-    mShowTicks = GetTickCount();
+    mShowTicks = ::GetTickCount();
   }
   ::SetEvent(mShowEvent);
 }

dom/plugins/ipc/PluginHangUIParent.h

@@ -12,6 +12,7 @@
 #include "base/process.h"
 #include "base/process_util.h"
 
+#include "mozilla/Mutex.h"
 #include "mozilla/plugins/PluginMessageUtils.h"
 
 #include "MiniShmParent.h"
@@ -126,15 +127,19 @@ private:
   bool
   RecvUserResponse(const unsigned int& aResponse);
 
+  bool
+  UnwatchHangUIChildProcess(bool aWait);
+
   static
   VOID CALLBACK SOnHangUIProcessExit(PVOID aContext, BOOLEAN aIsTimer);
 
 private:
+  Mutex mMutex;
   PluginModuleParent* mModule;
   const uint32_t mTimeoutPrefMs;
   const uint32_t mIPCTimeoutMs;
   MessageLoop* mMainThreadMessageLoop;
-  volatile bool mIsShowing;
+  bool mIsShowing;
   unsigned int mLastUserResponse;
   base::ProcessHandle mHangUIProcessHandle;
   NativeWindowHandle mMainWindowHandle;