Bug 1161169: Clean up usage of mContentParent and clearly identify it as specifically for async plugin init; r=billm,jimm
authorAaron Klotz <aklotz@mozilla.com>
Thu, 03 Mar 2016 00:24:36 -0700
changeset 325066 3438e0228ed7167f0f331a541cee59616c824c74
parent 325065 a8e1ac47ba0a421ae69512374caea9ad87aa716e
child 325067 23589403e7759d51a879ece223f6730656aa9081
push id1128
push userjlund@mozilla.com
push dateWed, 01 Jun 2016 01:31:59 +0000
treeherdermozilla-release@fe0d30de989d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbillm, jimm
bugs1161169
milestone47.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1161169: Clean up usage of mContentParent and clearly identify it as specifically for async plugin init; r=billm,jimm mContentParent is really just to be used while handling a synchronous ContentParent::RecvLoadPlugin call when async plugin init turned on. In any other context, using it will be unsafe. This patch adds comments and assertions to ensure that this value isn't set otherwise, and converts the one use of mContentParent outside of async plugin init to use an alternative mechanism for identifying the content process. MozReview-Commit-ID: Esgt1kj0MCt
dom/ipc/PProcessHangMonitor.ipdl
dom/ipc/ProcessHangMonitor.cpp
dom/plugins/ipc/PluginBridge.h
dom/plugins/ipc/PluginHangUIParent.cpp
dom/plugins/ipc/PluginModuleParent.cpp
dom/plugins/ipc/PluginModuleParent.h
--- a/dom/ipc/PProcessHangMonitor.ipdl
+++ b/dom/ipc/PProcessHangMonitor.ipdl
@@ -1,29 +1,31 @@
 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
  * vim: sw=2 ts=8 et :
  */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+using base::ProcessId from "base/process.h";
 using mozilla::dom::TabId from "mozilla/dom/ipc/IdType.h";
 
 namespace mozilla {
 
 struct SlowScriptData
 {
   TabId tabId;
   nsCString filename;
   uint32_t lineno;
 };
 
 struct PluginHangData
 {
   uint32_t pluginId;
+  ProcessId contentProcessId;
 };
 
 union HangData
 {
   SlowScriptData;
   PluginHangData;
 };
 
--- a/dom/ipc/ProcessHangMonitor.cpp
+++ b/dom/ipc/ProcessHangMonitor.cpp
@@ -423,17 +423,18 @@ HangMonitorChild::NotifyPluginHang(uint3
 
 void
 HangMonitorChild::NotifyPluginHangAsync(uint32_t aPluginId)
 {
   MOZ_RELEASE_ASSERT(MessageLoop::current() == MonitorLoop());
 
   // bounce back to parent on background thread
   if (mIPCOpen) {
-    Unused << SendHangEvidence(PluginHangData(aPluginId));
+    Unused << SendHangEvidence(PluginHangData(aPluginId,
+                                              base::GetCurrentProcId()));
   }
 }
 
 void
 HangMonitorChild::ClearHang()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
@@ -814,17 +815,18 @@ HangMonitoredProcess::TerminatePlugin()
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   if (mHangData.type() != HangData::TPluginHangData) {
     return NS_ERROR_UNEXPECTED;
   }
 
   // generates a crash report that includes a browser report taken here
   // earlier, the content process, and any plugin process(es).
   uint32_t id = mHangData.get_PluginHangData().pluginId();
-  plugins::TerminatePlugin(id, NS_LITERAL_CSTRING("HangMonitor"),
+  base::ProcessId contentPid = mHangData.get_PluginHangData().contentProcessId();
+  plugins::TerminatePlugin(id, contentPid, NS_LITERAL_CSTRING("HangMonitor"),
                            mBrowserDumpId);
 
   if (mActor) {
     mActor->CleanupPluginHang(id, false);
   }
   return NS_OK;
 }
 
--- a/dom/plugins/ipc/PluginBridge.h
+++ b/dom/plugins/ipc/PluginBridge.h
@@ -2,16 +2,18 @@
  * vim: sw=2 ts=2 et :
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_plugins_PluginBridge_h
 #define mozilla_plugins_PluginBridge_h
 
+#include "base/process.h"
+
 namespace mozilla {
 
 namespace dom {
 class ContentParent;
 } // namespace dom
 
 namespace plugins {
 
@@ -21,15 +23,16 @@ SetupBridge(uint32_t aPluginId, dom::Con
 
 nsresult
 FindPluginsForContent(uint32_t aPluginEpoch,
                       nsTArray<PluginTag>* aPlugins,
                       uint32_t* aNewPluginEpoch);
 
 void
 TerminatePlugin(uint32_t aPluginId,
+                base::ProcessId aContentProcessId,
                 const nsCString& aMonitorDescription,
                 const nsAString& aBrowserDumpId);
 
 } // namespace plugins
 } // namespace mozilla
 
 #endif // mozilla_plugins_PluginBridge_h
--- a/dom/plugins/ipc/PluginHangUIParent.cpp
+++ b/dom/plugins/ipc/PluginHangUIParent.cpp
@@ -4,16 +4,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "PluginHangUI.h"
 
 #include "PluginHangUIParent.h"
 
 #include "mozilla/Telemetry.h"
+#include "mozilla/ipc/ProtocolUtils.h"
 #include "mozilla/plugins/PluginModuleParent.h"
 
 #include "nsContentUtils.h"
 #include "nsDirectoryServiceDefs.h"
 #include "nsIFile.h"
 #include "nsIProperties.h"
 #include "nsIWindowMediator.h"
 #include "nsIWinTaskbar.h"
@@ -349,16 +350,17 @@ PluginHangUIParent::RecvUserResponse(con
   mLastUserResponse = aResponse;
   mResponseTicks = ::GetTickCount();
   mIsShowing = false;
   // responseCode: 1 = Stop, 2 = Continue, 3 = Cancel
   int responseCode;
   if (aResponse & HANGUI_USER_RESPONSE_STOP) {
     // User clicked Stop
     mModule->TerminateChildProcess(mMainThreadMessageLoop,
+                                   mozilla::ipc::kInvalidProcessId,
                                    NS_LITERAL_CSTRING("ModalHangUI"),
                                    EmptyString());
     responseCode = 1;
   } else if(aResponse & HANGUI_USER_RESPONSE_CONTINUE) {
     mModule->OnHangUIContinue();
     // User clicked Continue
     responseCode = 2;
   } else {
--- a/dom/plugins/ipc/PluginModuleParent.cpp
+++ b/dom/plugins/ipc/PluginModuleParent.cpp
@@ -12,16 +12,17 @@
 
 #include "base/process_util.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/dom/ContentParent.h"
 #include "mozilla/dom/ContentChild.h"
 #include "mozilla/dom/PCrashReporterParent.h"
 #include "mozilla/ipc/GeckoChildProcessHost.h"
 #include "mozilla/ipc/MessageChannel.h"
+#include "mozilla/ipc/ProtocolUtils.h"
 #include "mozilla/plugins/BrowserStreamParent.h"
 #include "mozilla/plugins/PluginAsyncSurrogate.h"
 #include "mozilla/plugins/PluginBridge.h"
 #include "mozilla/plugins/PluginInstanceParent.h"
 #include "mozilla/Preferences.h"
 #ifdef MOZ_ENABLE_PROFILER_SPS
 #include "mozilla/ProfileGatherer.h"
 #endif
@@ -122,17 +123,19 @@ mozilla::plugins::SetupBridge(uint32_t a
     if (NS_FAILED(*rv)) {
         return true;
     }
     PluginModuleChromeParent* chromeParent = static_cast<PluginModuleChromeParent*>(plugin->GetLibrary());
     *rv = chromeParent->GetRunID(runID);
     if (NS_FAILED(*rv)) {
         return true;
     }
-    chromeParent->SetContentParent(aContentParent);
+    if (chromeParent->IsStartingAsync()) {
+        chromeParent->SetContentParent(aContentParent);
+    }
     if (!aForceBridgeNow && chromeParent->IsStartingAsync() &&
         PluginModuleChromeParent::DidInstantiate()) {
         // We'll handle the bridging asynchronously
         return true;
     }
     *rv = PPluginModule::Bridge(aContentParent, chromeParent);
     return true;
 }
@@ -353,30 +356,32 @@ PRCList PluginModuleMapping::sModuleList
     PR_INIT_STATIC_CLIST(&PluginModuleMapping::sModuleListHead);
 
 bool PluginModuleMapping::sIsLoadModuleOnStack = false;
 
 } // namespace
 
 void
 mozilla::plugins::TerminatePlugin(uint32_t aPluginId,
+                                  base::ProcessId aContentProcessId,
                                   const nsCString& aMonitorDescription,
                                   const nsAString& aBrowserDumpId)
 {
     MOZ_ASSERT(XRE_IsParentProcess());
 
     RefPtr<nsPluginHost> host = nsPluginHost::GetInst();
     nsPluginTag* pluginTag = host->PluginWithId(aPluginId);
     if (!pluginTag || !pluginTag->mPlugin) {
         return;
     }
     RefPtr<nsNPAPIPlugin> plugin = pluginTag->mPlugin;
     PluginModuleChromeParent* chromeParent =
         static_cast<PluginModuleChromeParent*>(plugin->GetLibrary());
     chromeParent->TerminateChildProcess(MessageLoop::current(),
+                                        aContentProcessId,
                                         aMonitorDescription,
                                         aBrowserDumpId);
 }
 
 /* static */ PluginLibrary*
 PluginModuleContentParent::LoadModule(uint32_t aPluginId,
                                       nsPluginTag* aPluginTag)
 {
@@ -468,17 +473,18 @@ PluginModuleContentParent::OnLoadPluginR
     MOZ_ASSERT(parent);
     parent->RecvNP_InitializeResult(aResult ? NPERR_NO_ERROR
                                             : NPERR_GENERIC_ERROR);
 }
 
 void
 PluginModuleChromeParent::SetContentParent(dom::ContentParent* aContentParent)
 {
-    MOZ_ASSERT(aContentParent);
+    // mContentParent is to be used ONLY during async plugin init!
+    MOZ_ASSERT(aContentParent && mIsStartingAsync);
     mContentParent = aContentParent;
 }
 
 bool
 PluginModuleChromeParent::SendAssociatePluginId()
 {
     MOZ_ASSERT(mContentParent);
     return mContentParent->SendAssociatePluginId(mPluginId, OtherPid());
@@ -708,17 +714,17 @@ PluginModuleChromeParent::PluginModuleCh
                                                    uint32_t aPluginId,
                                                    int32_t aSandboxLevel,
                                                    bool aAllowAsyncInit)
     : PluginModuleParent(true, aAllowAsyncInit)
     , mSubprocess(new PluginProcessParent(aFilePath))
     , mPluginId(aPluginId)
     , mChromeTaskFactory(this)
     , mHangAnnotationFlags(0)
-    , mHangAnnotatorMutex("PluginModuleChromeParent::mHangAnnotatorMutex")
+    , mProtocolCallStackMutex("PluginModuleChromeParent::mProtocolCallStackMutex")
 #ifdef XP_WIN
     , mPluginCpuUsageOnHang()
     , mHangUIParent(nullptr)
     , mHangUIEnabled(true)
     , mIsTimerReset(true)
 #ifdef MOZ_CRASHREPORTER
     , mCrashReporterMutex("PluginModuleChromeParent::mCrashReporterMutex")
     , mCrashReporter(nullptr)
@@ -980,41 +986,41 @@ GetProcessCpuUsage(const InfallibleTArra
 
 #endif // #ifdef XP_WIN
 
 void
 PluginModuleChromeParent::OnEnteredCall()
 {
     mozilla::ipc::IProtocol* protocol = GetInvokingProtocol();
     MOZ_ASSERT(protocol);
-    mozilla::MutexAutoLock lock(mHangAnnotatorMutex);
+    mozilla::MutexAutoLock lock(mProtocolCallStackMutex);
     mProtocolCallStack.AppendElement(protocol);
 }
 
 void
 PluginModuleChromeParent::OnExitedCall()
 {
-    mozilla::MutexAutoLock lock(mHangAnnotatorMutex);
+    mozilla::MutexAutoLock lock(mProtocolCallStackMutex);
     MOZ_ASSERT(!mProtocolCallStack.IsEmpty());
     mProtocolCallStack.RemoveElementAt(mProtocolCallStack.Length() - 1);
 }
 
 void
 PluginModuleChromeParent::OnEnteredSyncSend()
 {
     mozilla::ipc::IProtocol* protocol = GetInvokingProtocol();
     MOZ_ASSERT(protocol);
-    mozilla::MutexAutoLock lock(mHangAnnotatorMutex);
+    mozilla::MutexAutoLock lock(mProtocolCallStackMutex);
     mProtocolCallStack.AppendElement(protocol);
 }
 
 void
 PluginModuleChromeParent::OnExitedSyncSend()
 {
-    mozilla::MutexAutoLock lock(mHangAnnotatorMutex);
+    mozilla::MutexAutoLock lock(mProtocolCallStackMutex);
     MOZ_ASSERT(!mProtocolCallStack.IsEmpty());
     mProtocolCallStack.RemoveElementAt(mProtocolCallStack.Length() - 1);
 }
 
 /**
  * This function converts the topmost routing id on the call stack (as recorded
  * by the MessageChannel) into a pointer to a IProtocol object.
  */
@@ -1166,16 +1172,17 @@ PluginModuleChromeParent::ShouldContinue
     if (LaunchHangUI()) {
         return true;
     }
     // If LaunchHangUI returned false then we should proceed with the 
     // original plugin hang behaviour and kill the plugin container.
     FinishHangUI();
 #endif // XP_WIN
     TerminateChildProcess(MessageLoop::current(),
+                          mozilla::ipc::kInvalidProcessId,
                           NS_LITERAL_CSTRING("ModalHangUI"),
                           EmptyString());
     GetIPCChannel()->CloseWithTimeout();
     return false;
 }
 
 bool
 PluginModuleContentParent::ShouldContinueFromReplyTimeout()
@@ -1191,16 +1198,17 @@ PluginModuleContentParent::ShouldContinu
 void
 PluginModuleContentParent::OnExitedSyncSend()
 {
     ProcessHangMonitor::ClearHang();
 }
 
 void
 PluginModuleChromeParent::TerminateChildProcess(MessageLoop* aMsgLoop,
+                                                base::ProcessId aContentPid,
                                                 const nsCString& aMonitorDescription,
                                                 const nsAString& aBrowserDumpId)
 {
 #ifdef MOZ_CRASHREPORTER
 #ifdef XP_WIN
     mozilla::MutexAutoLock lock(mCrashReporterMutex);
     CrashReporterParent* crashReporter = mCrashReporter;
     if (!crashReporter) {
@@ -1276,19 +1284,19 @@ PluginModuleChromeParent::TerminateChild
                                      NS_LITERAL_CSTRING("flash1"))) {
                 additionalDumps.AppendLiteral(",flash1");
             }
             if (CreatePluginMinidump(mFlashProcess2, 0, pluginDumpFile,
                                      NS_LITERAL_CSTRING("flash2"))) {
                 additionalDumps.AppendLiteral(",flash2");
             }
 #endif
-            if (mContentParent) {
+            if (aContentPid != mozilla::ipc::kInvalidProcessId) {
                 // Include the content process minidump
-                if (CreatePluginMinidump(mContentParent->OtherPid(), 0,
+                if (CreatePluginMinidump(aContentPid, 0,
                                          pluginDumpFile,
                                          NS_LITERAL_CSTRING("content"))) {
                     additionalDumps.AppendLiteral(",content");
                 }
             }
         }
         crashReporter->AnnotateCrashReport(
             NS_LITERAL_CSTRING("additional_minidumps"),
@@ -2234,17 +2242,19 @@ PluginModuleChromeParent::RecvNP_Initial
     bool initOk = aError == NPERR_NO_ERROR;
     if (initOk) {
         SetPluginFuncs(mNPPIface);
         if (mIsStartingAsync && !SendAssociatePluginId()) {
             initOk = false;
         }
     }
     mNPInitialized = initOk;
-    return mContentParent->SendLoadPluginResult(mPluginId, initOk);
+    bool result = mContentParent->SendLoadPluginResult(mPluginId, initOk);
+    mContentParent = nullptr;
+    return result;
 }
 
 #else
 
 nsresult
 PluginModuleParent::NP_Initialize(NPNetscapeFuncs* bFuncs, NPError* error)
 {
     PLUGIN_LOG_DEBUG_METHOD;
@@ -2341,19 +2351,20 @@ PluginModuleParent::RecvNP_InitializeRes
 bool
 PluginModuleChromeParent::RecvNP_InitializeResult(const NPError& aError)
 {
     bool ok = true;
     if (mContentParent) {
         if ((ok = SendAssociatePluginId())) {
             ok = mContentParent->SendLoadPluginResult(mPluginId,
                                                       aError == NPERR_NO_ERROR);
+            mContentParent = nullptr;
         }
     } else if (aError == NPERR_NO_ERROR) {
-        // Initialization steps when e10s is disabled
+        // Initialization steps for (e10s && !asyncInit) || !e10s
 #if defined XP_WIN
         if (mIsStartingAsync) {
             SetPluginFuncs(mNPPIface);
         }
 
         // Send the info needed to join the chrome process's audio session to the
         // plugin process
         nsID id;
--- a/dom/plugins/ipc/PluginModuleParent.h
+++ b/dom/plugins/ipc/PluginModuleParent.h
@@ -413,25 +413,28 @@ class PluginModuleChromeParent
 
     /*
      * Terminates the plugin process associated with this plugin module. Also
      * generates appropriate crash reports. Takes ownership of the file
      * associated with aBrowserDumpId on success.
      *
      * @param aMsgLoop the main message pump associated with the module
      *   protocol.
+     * @param aContentPid PID of the e10s content process from which a hang was
+     *   reported. May be kInvalidProcessId if not applicable.
      * @param aMonitorDescription a string describing the hang monitor that
      *   is making this call. This string is added to the crash reporter
      *   annotations for the plugin process.
      * @param aBrowserDumpId (optional) previously taken browser dump id. If
      *   provided TerminateChildProcess will use this browser dump file in
      *   generating a multi-process crash report. If not provided a browser
      *   dump will be taken at the time of this call.
      */
     void TerminateChildProcess(MessageLoop* aMsgLoop,
+                               base::ProcessId aContentPid,
                                const nsCString& aMonitorDescription,
                                const nsAString& aBrowserDumpId);
 
 #ifdef XP_WIN
     /**
      * Called by Plugin Hang UI to notify that the user has clicked continue.
      * Used for chrome hang annotations.
      */
@@ -542,17 +545,17 @@ private:
     enum HangAnnotationFlags
     {
         kInPluginCall = (1u << 0),
         kHangUIShown = (1u << 1),
         kHangUIContinued = (1u << 2),
         kHangUIDontShow = (1u << 3)
     };
     Atomic<uint32_t> mHangAnnotationFlags;
-    mozilla::Mutex mHangAnnotatorMutex;
+    mozilla::Mutex mProtocolCallStackMutex;
     InfallibleTArray<mozilla::ipc::IProtocol*> mProtocolCallStack;
 #ifdef XP_WIN
     InfallibleTArray<float> mPluginCpuUsageOnHang;
     PluginHangUIParent *mHangUIParent;
     bool mHangUIEnabled;
     bool mIsTimerReset;
 #ifdef MOZ_CRASHREPORTER
     /**
@@ -620,16 +623,20 @@ private:
         PluginModuleChromeParent* mModule;
     };
 
     friend class LaunchedTask;
 
     bool                mInitOnAsyncConnect;
     nsresult            mAsyncInitRv;
     NPError             mAsyncInitError;
+    // mContentParent is to be used ONLY during the IPC dance that occurs
+    // when ContentParent::RecvLoadPlugin is called under async plugin init!
+    // In other contexts it is *unsafe*, as there might be multiple content
+    // processes in existence!
     dom::ContentParent* mContentParent;
     nsCOMPtr<nsIObserver> mOfflineObserver;
 #ifdef MOZ_ENABLE_PROFILER_SPS
     RefPtr<mozilla::ProfileGatherer> mGatherer;
 #endif
     nsCString mProfile;
     bool mIsBlocklisted;
     static bool sInstantiated;