Bug 836654 - Part 9: During app-frame creation, take the CPU wake lock only after sending down the mozapptype. r=cjones
authorJustin Lebar <justin.lebar@gmail.com>
Thu, 14 Feb 2013 15:41:30 -0500
changeset 131845 5e64eb3c6c96abb94707cde25115066c9d2bed2a
parent 131844 942cec35b0e443315aac86812fbe7de2a8245522
child 131846 832ac179e3d0ce85b476c1a7daf9fe231b77de0b
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
bugs836654
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 836654 - Part 9: During app-frame creation, take the CPU wake lock only after sending down the mozapptype. r=cjones This prevents the child process from downgrading its CPU priority when it notices that it has the CPU wake lock but no "critical" frames. In this patch we also reduce the scope of a race condition which occurs when we don't launch a new process for an app. In this case, we still need to set the child process's priority and then check that the process is still alive. Because this isn't a brand-new process, it's possible that the process will downgrade its priority (e.g. due to a timer) after we check that it's still alive and before it notices that we've acquired the CPU wake lock on its behalf. This patch does not resolve that race.
content/base/src/nsFrameLoader.cpp
dom/ipc/ContentParent.cpp
dom/ipc/ContentParent.h
--- a/content/base/src/nsFrameLoader.cpp
+++ b/content/base/src/nsFrameLoader.cpp
@@ -2049,24 +2049,16 @@ nsFrameLoader::TryRemoteBrowser()
   } else if (OwnerIsBrowserFrame()) {
     // The |else| above is unnecessary; OwnerIsBrowserFrame() implies !ownApp.
     context.SetTabContextForBrowserFrame(containingApp, scrollingBehavior);
   }
 
   nsCOMPtr<nsIDOMElement> ownerElement = do_QueryInterface(mOwnerContent);
   mRemoteBrowser = ContentParent::CreateBrowserOrApp(context, ownerElement);
   if (mRemoteBrowser) {
-    // If we're an app, send the frame element's mozapptype down to the child
-    // process.  This ends up in TabChild::GetAppType().
-    if (ownApp) {
-      nsAutoString appType;
-      mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::mozapptype, appType);
-      mRemoteBrowser->SendSetAppType(appType);
-    }
-
     nsCOMPtr<nsIDocShellTreeItem> rootItem;
     parentAsItem->GetRootTreeItem(getter_AddRefs(rootItem));
     nsCOMPtr<nsIDOMWindow> rootWin = do_GetInterface(rootItem);
     nsCOMPtr<nsIDOMChromeWindow> rootChromeWin = do_QueryInterface(rootWin);
     NS_ABORT_IF_FALSE(rootChromeWin, "How did we not get a chrome window here?");
 
     nsCOMPtr<nsIBrowserDOMWindow> browserDOMWin;
     rootChromeWin->GetBrowserDOMWindow(getter_AddRefs(browserDOMWin));
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -262,18 +262,18 @@ ContentParent::MaybeTakePreallocatedAppP
 {
     nsRefPtr<ContentParent> process = sPreallocatedAppProcess.get();
     sPreallocatedAppProcess = nullptr;
 
     if (!process) {
         return nullptr;
     }
 
-    if (!process->TransformPreallocatedIntoApp(aAppManifestURL, aPrivs,
-                                               aInitialPriority)) {
+    if (!process->SetPriorityAndCheckIsAlive(aInitialPriority) ||
+        !process->TransformPreallocatedIntoApp(aAppManifestURL, aPrivs)) {
         // 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();
 }
@@ -487,40 +487,62 @@ ContentParent::CreateBrowserOrApp(const 
 
     // Each app gets its own ContentParent instance.
     nsAutoString manifestURL;
     if (NS_FAILED(ownApp->GetManifestURL(manifestURL))) {
         NS_ERROR("Failed to get manifest URL");
         return nullptr;
     }
 
+    ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement);
+
     nsRefPtr<ContentParent> p = gAppContentParents->Get(manifestURL);
+    if (p) {
+        // Check that the process is still alive and set its priority.
+        // Hopefully the process won't die after this point, if this call
+        // succeeds.
+        if (!p->SetPriorityAndCheckIsAlive(initialPriority)) {
+            p = nullptr;
+        }
+    }
+
     if (!p) {
-        ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement);
-
         ChildPrivileges privs = PrivilegesForApp(ownApp);
         p = MaybeTakePreallocatedAppProcess(manifestURL, privs,
                                             initialPriority);
         if (!p) {
             NS_WARNING("Unable to use pre-allocated app process");
             p = new ContentParent(manifestURL, /* isBrowserElement = */ false,
                                   privs, initialPriority);
             p->Init();
         }
         gAppContentParents->Put(manifestURL, p);
     }
 
-    p->MaybeTakeCPUWakeLock(aFrameElement);
-
     nsRefPtr<TabParent> tp = new TabParent(aContext);
     tp->SetOwnerElement(aFrameElement);
     PBrowserParent* browser = p->SendPBrowserConstructor(
         tp.forget().get(), // DeallocPBrowserParent() releases this ref.
         aContext.AsIPCTabContext(),
         /* chromeFlags */ 0);
+
+    // Send the frame element's mozapptype down to the child process.  This ends
+    // up in TabChild::GetAppType().  We have to do this /before/ we acquire the
+    // CPU wake lock for this process, because if the child sees that it has a
+    // CPU wake lock but its TabChild doesn't have the right mozapptype, it
+    // might downgrade its process priority.
+    nsCOMPtr<Element> frameElement = do_QueryInterface(aFrameElement);
+    if (frameElement) {
+      nsAutoString appType;
+      frameElement->GetAttr(kNameSpaceID_None, nsGkAtoms::mozapptype, appType);
+      unused << browser->SendSetAppType(appType);
+    }
+
+    p->MaybeTakeCPUWakeLock(aFrameElement);
+
     return static_cast<TabParent*>(browser);
 }
 
 static PLDHashOperator
 AppendToTArray(const nsAString& aKey, ContentParent* aValue, void* aArray)
 {
     nsTArray<ContentParent*> *array =
         static_cast<nsTArray<ContentParent*>*>(aArray);
@@ -668,24 +690,24 @@ StaticAutoPtr<LinkedList<SystemMessageHa
     SystemMessageHandledListener::sListeners;
 
 NS_IMPL_ISUPPORTS1(SystemMessageHandledListener,
                    nsITimerCallback)
 
 } // anonymous namespace
 
 void
-ContentParent::SetProcessInitialPriority(ProcessPriority aInitialPriority)
+ContentParent::SetProcessPriority(ProcessPriority aPriority)
 {
     if (!Preferences::GetBool("dom.ipc.processPriorityManager.enabled")) {
         return;
     }
 
-    SetProcessPriority(base::GetProcId(mSubprocess->GetChildProcessHandle()),
-                       aInitialPriority);
+    hal::SetProcessPriority(base::GetProcId(mSubprocess->GetChildProcessHandle()),
+                            aPriority);
 }
 
 void
 ContentParent::MaybeTakeCPUWakeLock(nsIDOMElement* aFrameElement)
 {
     // Take the CPU wake lock on behalf of this processs if it's expecting a
     // system message.  We'll release the CPU lock once the message is
     // delivered, or after some period of time, which ever comes first.
@@ -703,43 +725,46 @@ ContentParent::MaybeTakeCPUWakeLock(nsID
 
     // This object's Init() function keeps it alive.
     nsRefPtr<SystemMessageHandledListener> listener =
         new SystemMessageHandledListener();
     listener->Init(lock);
 }
 
 bool
-ContentParent::TransformPreallocatedIntoApp(const nsAString& aAppManifestURL,
-                                            ChildPrivileges aPrivs,
-                                            ProcessPriority aInitialPriority)
+ContentParent::SetPriorityAndCheckIsAlive(ProcessPriority aPriority)
 {
-    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;
+    SetProcessPriority(aPriority);
 
-    SetProcessInitialPriority(aInitialPriority);
-
-    // Now that we've increased the process's priority from BACKGROUND (where
-    // the preallocated app sits) to something higher, check whether the process
-    // is still alive.  Hopefully the process won't unexpectedly crash after
-    // this point!
+    // Now that we've set this process's priority, check whether the process is
+    // still alive.  Hopefully we've set the priority to FOREGROUND*, so the
+    // process won't unexpectedly crash after this point!
     //
     // It's not legal to call DidProcessCrash on Windows if the process has not
-    // terminated yet, so we have to skip this check there.
-
+    // terminated yet, so we have to skip this check here.
 #ifndef XP_WIN
     bool exited = false;
     base::DidProcessCrash(&exited, mSubprocess->GetChildProcessHandle());
     if (exited) {
         return false;
     }
 #endif
 
+    return true;
+}
+
+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;
+
     return SendSetProcessPrivileges(aPrivs);
 }
 
 void
 ContentParent::ShutDownProcess()
 {
   if (!mIsDestroyed) {
     const InfallibleTArray<PIndexedDBParent*>& idbParents =
@@ -1066,17 +1091,17 @@ ContentParent::ContentParent(const nsASt
     mSubprocess = new GeckoChildProcessHost(GeckoProcessType_Content,
                                             aOSPrivileges);
 
     mSubprocess->LaunchAndWaitForProcessHandle();
 
     // Set the subprocess's priority.  We do this first because we're likely
     // /lowering/ its CPU and memory priority, which it has inherited from this
     // process.
-    SetProcessInitialPriority(aInitialPriority);
+    SetProcessPriority(aInitialPriority);
 
     Open(mSubprocess->GetChannel(), mSubprocess->GetChildProcessHandle());
 
     // NB: internally, this will send an IPC message to the child
     // process to get it to create the CompositorChild.  This
     // message goes through the regular IPC queue for this
     // channel, so delivery will happen-before any other messages
     // we send.  The CompositorChild must be created before any
--- a/dom/ipc/ContentParent.h
+++ b/dom/ipc/ContentParent.h
@@ -182,30 +182,35 @@ private:
                   ChildPrivileges aOSPrivileges = base::PRIVILEGES_DEFAULT,
                   hal::ProcessPriority aInitialPriority = hal::PROCESS_PRIORITY_FOREGROUND);
     virtual ~ContentParent();
 
     void Init();
 
     // Set the child process's priority.  Once the child starts up, it will
     // manage its own priority via the ProcessPriorityManager.
-    void SetProcessInitialPriority(hal::ProcessPriority aInitialPriority);
+    void SetProcessPriority(hal::ProcessPriority aInitialPriority);
 
     // If the frame element indicates that the child process is "critical" and
     // has a pending system message, this function acquires the CPU wake lock on
     // behalf of the child.  We'll release the lock when the system message is
     // handled or after a timeout, whichever comes first.
     void MaybeTakeCPUWakeLock(nsIDOMElement* aFrameElement);
 
+    // Set the child process's priority and then check whether the child is
+    // still alive.  Returns true if the process is still alive, and false
+    // otherwise.  If you pass a FOREGROUND* priority here, it's (hopefully)
+    // unlikely that the process will be killed after this point.
+    bool SetPriorityAndCheckIsAlive(hal::ProcessPriority aPriority);
+
     // Transform a pre-allocated app process into a "real" app
     // process, for the specified manifest URL.  If this returns false, the
     // child process has died.
     bool TransformPreallocatedIntoApp(const nsAString& aAppManifestURL,
-                                      ChildPrivileges aPrivs,
-                                      hal::ProcessPriority aInitialPriority);
+                                      ChildPrivileges aPrivs);
 
     /**
      * Mark this ContentParent as dead for the purposes of Get*().
      * This method is idempotent.
      */
     void MarkAsDead();
 
     /**