From 093600fa79603e1b0b8a59cd402ae967128e8c19 Mon Sep 17 00:00:00 2001 From: Justin Lebar Date: Thu, 14 Feb 2013 15:41:30 -0500 Subject: [PATCH] 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 | 8 ---- dom/ipc/ContentParent.cpp | 77 ++++++++++++++++++++---------- dom/ipc/ContentParent.h | 11 +++-- 3 files changed, 59 insertions(+), 37 deletions(-) diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp index 85347fce1ad..a8f9145031c 100644 --- a/content/base/src/nsFrameLoader.cpp +++ b/content/base/src/nsFrameLoader.cpp @@ -2054,14 +2054,6 @@ nsFrameLoader::TryRemoteBrowser() nsCOMPtr 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 rootItem; parentAsItem->GetRootTreeItem(getter_AddRefs(rootItem)); nsCOMPtr rootWin = do_GetInterface(rootItem); diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp index 5610a17a825..efa7e356bce 100644 --- a/dom/ipc/ContentParent.cpp +++ b/dom/ipc/ContentParent.cpp @@ -267,8 +267,8 @@ ContentParent::MaybeTakePreallocatedAppProcess(const nsAString& aAppManifestURL, 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(); @@ -492,10 +492,19 @@ ContentParent::CreateBrowserOrApp(const TabContext& aContext, return nullptr; } - nsRefPtr p = gAppContentParents->Get(manifestURL); - if (!p) { - ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement); + ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement); + nsRefPtr 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) { ChildPrivileges privs = PrivilegesForApp(ownApp); p = MaybeTakePreallocatedAppProcess(manifestURL, privs, initialPriority); @@ -508,14 +517,27 @@ ContentParent::CreateBrowserOrApp(const TabContext& aContext, gAppContentParents->Put(manifestURL, p); } - p->MaybeTakeCPUWakeLock(aFrameElement); - nsRefPtr 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 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(browser); } @@ -673,14 +695,14 @@ NS_IMPL_ISUPPORTS1(SystemMessageHandledListener, } // 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 @@ -708,25 +730,16 @@ ContentParent::MaybeTakeCPUWakeLock(nsIDOMElement* aFrameElement) } 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(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()); @@ -735,6 +748,18 @@ ContentParent::TransformPreallocatedIntoApp(const nsAString& aAppManifestURL, } #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(mAppManifestURL) = aAppManifestURL; + return SendSetProcessPrivileges(aPrivs); } @@ -1071,7 +1096,7 @@ ContentParent::ContentParent(const nsAString& aAppManifestURL, // 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()); diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h index bb3c61d6752..3f18b580e50 100644 --- a/dom/ipc/ContentParent.h +++ b/dom/ipc/ContentParent.h @@ -187,7 +187,7 @@ private: // 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 @@ -195,12 +195,17 @@ private: // 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*().