Bug 838616 - Eliminate a process-priority race condition around preallocated process creation. r=cjones
authorJustin Lebar <justin.lebar@gmail.com>
Fri, 08 Feb 2013 14:32:23 +0000
changeset 131204 4fb612f6279f5d42f3d572bb9b69e494e59852cf
parent 131203 b7b4cfecd9dc6a07a70723fa2185208211affead
child 131205 75899c659a0b5ca139e7b24d328f9697deadc64b
push id2323
push userbbajaj@mozilla.com
push dateMon, 01 Apr 2013 19:47:02 +0000
treeherdermozilla-beta@7712be144d91 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscjones
bugs838616
milestone21.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 838616 - Eliminate a process-priority race condition around preallocated process creation. r=cjones Previously, it was possible to return a dead preallocated process. This patch eliminates or at least significantly reduces the likelihood of this race.
dom/ipc/ContentParent.cpp
dom/ipc/ContentParent.h
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -246,20 +246,36 @@ ContentParent::ScheduleDelayedPreallocat
     }
     sPreallocateAppProcessTask =
         NewRunnableFunction(DelayedPreallocateAppProcess);
     MessageLoop::current()->PostDelayedTask(
         FROM_HERE, sPreallocateAppProcessTask, sPreallocateDelayMs);
 }
 
 /*static*/ already_AddRefed<ContentParent>
-ContentParent::MaybeTakePreallocatedAppProcess()
+ContentParent::MaybeTakePreallocatedAppProcess(const nsAString& aAppManifestURL,
+                                               ChildPrivileges aPrivs)
 {
     nsRefPtr<ContentParent> process = sPreallocatedAppProcess.get();
     sPreallocatedAppProcess = nullptr;
+
+    if (!process) {
+        return nullptr;
+    }
+
+    if (!process->TransformPreallocatedIntoApp(aAppManifestURL, aPrivs)) {
+        NS_WARNING("Can't TransformPrealocatedIntoApp.  Maybe "
+                   "the preallocated process died?");
+
+        // Kill the process just in case it's not actually dead; we don't want
+        // to "leak" this process!
+        process->KillHard();
+        return nullptr;
+    }
+
     return process.forget();
 }
 
 /*static*/ void
 ContentParent::FirstIdle(void)
 {
     // The parent has gone idle for the first time. This would be a good
     // time to preallocate an app process.
@@ -438,20 +454,18 @@ ContentParent::CreateBrowserOrApp(const 
     if (NS_FAILED(ownApp->GetManifestURL(manifestURL))) {
         NS_ERROR("Failed to get manifest URL");
         return nullptr;
     }
 
     nsRefPtr<ContentParent> p = gAppContentParents->Get(manifestURL);
     if (!p) {
         ChildPrivileges privs = PrivilegesForApp(ownApp);
-        p = MaybeTakePreallocatedAppProcess();
-        if (p) {
-            p->TransformPreallocatedIntoApp(manifestURL, privs);            
-        } else {
+        p = MaybeTakePreallocatedAppProcess(manifestURL, privs);
+        if (!p) {
             NS_WARNING("Unable to use pre-allocated app process");
             p = new ContentParent(manifestURL, /* isBrowserElement = */ false,
                                   privs);
             p->Init();
         }
         gAppContentParents->Put(manifestURL, p);
     }
 
@@ -527,17 +541,17 @@ ContentParent::Init()
         unused << SendActivateA11y();
     }
 #endif
 
     DebugOnly<FileUpdateDispatcher*> observer = FileUpdateDispatcher::GetSingleton();
     NS_ASSERTION(observer, "FileUpdateDispatcher is null");
 }
 
-void
+bool
 ContentParent::TransformPreallocatedIntoApp(const nsAString& aAppManifestURL,
                                             ChildPrivileges aPrivs)
 {
     MOZ_ASSERT(mAppManifestURL == MAGIC_PREALLOCATED_APP_MANIFEST_URL);
     // Clients should think of mAppManifestURL as const ... we're
     // bending the rules here just for the preallocation hack.
     const_cast<nsString&>(mAppManifestURL) = aAppManifestURL;
 
@@ -546,17 +560,17 @@ ContentParent::TransformPreallocatedInto
     // ContentChild::AllocPBrowser, but this happens earlier, thus reducing the
     // window in which the child might be killed due to low memory.
     if (Preferences::GetBool("dom.ipc.processPriorityManager.enabled")) {
         SetProcessPriority(base::GetProcId(mSubprocess->GetChildProcessHandle()),
                            PROCESS_PRIORITY_FOREGROUND);
     }
 
     // If this fails, the child process died.
-    unused << SendSetProcessPrivileges(aPrivs);
+    return SendSetProcessPrivileges(aPrivs);
 }
 
 void
 ContentParent::ShutDownProcess()
 {
   if (!mIsDestroyed) {
     const InfallibleTArray<PIndexedDBParent*>& idbParents =
       ManagedPIndexedDBParent();
--- a/dom/ipc/ContentParent.h
+++ b/dom/ipc/ContentParent.h
@@ -153,33 +153,41 @@ private:
     static nsTArray<ContentParent*>* gPrivateContent;
 
     static void JoinProcessesIOThread(const nsTArray<ContentParent*>* aProcesses,
                                       Monitor* aMonitor, bool* aDone);
 
     static void PreallocateAppProcess();
     static void DelayedPreallocateAppProcess();
     static void ScheduleDelayedPreallocateAppProcess();
-    static already_AddRefed<ContentParent> MaybeTakePreallocatedAppProcess();
+
+    // Take the preallocated process and transform it into a "real" app process,
+    // for the specified manifest URL.  If there is no preallocated process (or
+    // if it's dead), this returns false.
+    static already_AddRefed<ContentParent>
+    MaybeTakePreallocatedAppProcess(const nsAString& aAppManifestURL,
+                                    ChildPrivileges aPrivs);
+
     static void FirstIdle();
 
     // Hide the raw constructor methods since we don't want client code
     // using them.
     using PContentParent::SendPBrowserConstructor;
     using PContentParent::SendPTestShellConstructor;
 
     ContentParent(const nsAString& aAppManifestURL, bool aIsForBrowser,
                   base::ChildPrivileges aOSPrivileges = base::PRIVILEGES_DEFAULT);
     virtual ~ContentParent();
 
     void Init();
 
     // Transform a pre-allocated app process into a "real" app
-    // process, for the specified manifest URL.
-    void TransformPreallocatedIntoApp(const nsAString& aAppManifestURL,
+    // process, for the specified manifest URL.  If this returns false, the
+    // child process has died.
+    bool TransformPreallocatedIntoApp(const nsAString& aAppManifestURL,
                                       ChildPrivileges aPrivs);
 
     /**
      * Mark this ContentParent as dead for the purposes of Get*().
      * This method is idempotent.
      */
     void MarkAsDead();