Bug 1641715 - Normalize permission transmission. r=rpl

### Process Reuse

Prior to this patch, the logic was unintentionally specifying that
existing processes shouldn't be reused, which could lead to growing
the size of a process pool that we did not actually desire to grow.
We now pass the flag to request reuse of a process, which helps
compensate for the process selection decision process happening on
PBackground in a manner which inherently does not have as full an
understanding of what processes are in the act of being created on
the main thread.

See the comments in the code for more informationm.

### Permission Transmission

Prior to this patch, permissions would be transmitted when either:
1. The choice was made to use an existing process.
2. A new process was launched to be the home for a worker.

This seems like it covers all the bases but the actual mechanism by
which a remote worker would be launched for a new process is when its
RemoteWorkerServiceParent actor is registered.  The above would be
sufficient if the only way for processes to come into existence was at
the request of the RemoteWorkerService, but obviously this is not the
case.  When that happened, it would be possible for a remote worker to
be placed in a new process that was not the process we were
transmitting permissions for.

The changes in this patch normalize permission transmission so that
they are transmitted at the same time we attempt to spawn the worker
via the selected RemoteWorkerServiceParent.  This code path is used for
both an existing process and when a new process is spawned so there's
no longer any gap.

Differential Revision: https://phabricator.services.mozilla.com/D99753
This commit is contained in:
Andrew Sutherland 2020-12-18 16:27:52 +00:00
parent 75dc272ce2
commit dd7728004b

View File

@ -445,6 +445,26 @@ void RemoteWorkerManager::LaunchInternal(
MOZ_ASSERT(aTargetActor == mParentActor ||
mChildActors.Contains(aTargetActor));
// We need to send permissions to content processes, but not if we're spawning
// the worker here in the parent process.
if (aTargetActor != mParentActor) {
RefPtr<ContentParent> contentParent =
BackgroundParent::GetContentParent(aTargetActor->Manager());
// This won't cause any race conditions because the content process
// should wait for the permissions to be received before executing the
// Service Worker.
nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
__func__, [contentParent = std::move(contentParent),
principalInfo = aData.principalInfo()] {
TransmitPermissionsAndBlobURLsForPrincipalInfo(contentParent,
principalInfo);
});
MOZ_ALWAYS_SUCCEEDS(
SchedulerGroup::Dispatch(TaskCategory::Other, r.forget()));
}
RemoteWorkerParent* workerActor = static_cast<RemoteWorkerParent*>(
aTargetActor->Manager()->SendPRemoteWorkerConstructor(aData));
if (NS_WARN_IF(!workerActor)) {
@ -572,19 +592,6 @@ RemoteWorkerServiceParent* RemoteWorkerManager::SelectTargetActorInternal(
(aActor->OtherPid() == aProcessId || !actor)) {
++lock->mCount;
// This won't cause any race conditions because the content process
// should wait for the permissions to be received before executing the
// Service Worker.
nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
__func__, [contentParent = std::move(aContentParent),
principalInfo = aData.principalInfo()] {
TransmitPermissionsAndBlobURLsForPrincipalInfo(contentParent,
principalInfo);
});
MOZ_ALWAYS_SUCCEEDS(
SchedulerGroup::Dispatch(TaskCategory::Other, r.forget()));
actor = aActor;
return false;
}
@ -656,8 +663,6 @@ void RemoteWorkerManager::LaunchNewContentProcess(
const nsCString& remoteType) mutable {
if (aValue.IsResolve()) {
LOG(("LaunchNewContentProcess: successfully got child process"));
TransmitPermissionsAndBlobURLsForPrincipalInfo(aValue.ResolveValue(),
principalInfo);
// The failure callback won't run, and we're on the main thread, so
// we need to properly release the thread-unsafe RemoteWorkerManager.
@ -697,8 +702,23 @@ void RemoteWorkerManager::LaunchNewContentProcess(
auto remoteType =
workerRemoteType.IsEmpty() ? DEFAULT_REMOTE_TYPE : workerRemoteType;
// Request a process making sure to specify aPreferUsed=true. For a
// given remoteType there's a pool size limit. If we pass aPreferUsed
// here, then if there's any process in the pool already, we will use
// that. If we pass false (which is the default if omitted), then this
// call will spawn a new process if the pool isn't at its limit yet.
//
// (Our intent is never to grow the pool size here. Our logic gets here
// because our current logic on PBackground is only aware of
// RemoteWorkerServiceParent actors that have registered themselves,
// which is fundamentally unaware of processes that will match in the
// future when they register. So we absolutely are fine with and want
// any existing processes.)
ContentParent::GetNewOrUsedBrowserProcessAsync(
/* aRemoteType = */ remoteType)
/* aRemoteType = */ remoteType,
/* aGroup */ nullptr,
hal::ProcessPriority::PROCESS_PRIORITY_FOREGROUND,
/* aPreferUsed */ true)
->Then(GetCurrentSerialEventTarget(), __func__,
[callback = std::move(callback),
remoteType](const CallbackParamType& aValue) mutable {