Skip to content

GitLab

T
tangled
Commit c7079780 authored by Matt Falkenhagen's avatar Matt Falkenhagen Committed by Commit Bot
Browse files
Options

service worker: Remove the dispatcher host check in start worker. 

According to UMA this doesn't happen since 65. The original bug about a
reused/crashed process host was probably fixed when we removed the extra
thread hops after allocating a process. Then the remaining bug issue
781313 was fixed in 65, which was causing a process from the wrong
storage partition to be allocated.

Also simplify SendStartWorker() by removing the return value.

Change-Id: I0dddd731c4c512e21b4644b19b074b8bcbdfcd5b
Reviewed-on: https://chromium-review.googlesource.com/1037048Reviewed-by: default avatarHiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556373}
parent 488e5e08
Hide whitespace changes
Inline Side-by-side
Showing with 16 additions and 67 deletions
content/browser/service_worker/embedded_worker_instance.cc
View file @ c7079780
......@@ -417,13 +417,16 @@ class EmbeddedWorkerInstance::StartTask {
network::mojom::URLLoaderFactoryPtrInfo non_network_loader_factory_info) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
// We allocated a process but will not use it. Tell the process manager to
// release the process, by making a WorkerProcessHandle and immediately
// destroying it.
if (status == SERVICE_WORKER_OK && !instance_->context_) {
WorkerProcessHandle(process_manager, instance_->embedded_worker_id(),
process_info->process_id);
status = SERVICE_WORKER_ERROR_ABORT;
std::unique_ptr<WorkerProcessHandle> process_handle;
if (status == SERVICE_WORKER_OK) {
// If we allocated a process, WorkerProcessHandle has to be created before
// returning to ensure the process is eventually released.
process_handle = std::make_unique<WorkerProcessHandle>(
process_manager, instance_->embedded_worker_id(),
process_info->process_id);
if (!instance_->context_)
status = SERVICE_WORKER_ERROR_ABORT;
}
if (status != SERVICE_WORKER_OK) {
......@@ -452,11 +455,7 @@ class EmbeddedWorkerInstance::StartTask {
// Notify the instance that a process is allocated.
state_ = ProcessAllocationState::ALLOCATED;
instance_->OnProcessAllocated(
std::make_unique<WorkerProcessHandle>(process_manager,
instance_->embedded_worker_id(),
process_info->process_id),
start_situation);
instance_->OnProcessAllocated(std::move(process_handle), start_situation);
// Notify the instance that it is registered to the DevTools manager.
instance_->OnRegisteredToDevToolsManager(std::move(devtools_proxy),
......@@ -464,14 +463,9 @@ class EmbeddedWorkerInstance::StartTask {
network::mojom::URLLoaderFactoryPtr non_network_loader_factory(
std::move(non_network_loader_factory_info));
status = instance_->SendStartWorker(std::move(params),
std::move(non_network_loader_factory));
if (status != SERVICE_WORKER_OK) {
StatusCallback callback = std::move(start_callback_);
start_callback_.Reset();
instance_->OnStartFailed(std::move(callback), status);
// |this| may be destroyed.
}
instance_->SendStartWorker(std::move(params),
std::move(non_network_loader_factory));
TRACE_EVENT_NESTABLE_ASYNC_BEGIN0("ServiceWorker",
"INITIALIZING_ON_RENDERER", this);
// |this|'s work is done here, but |instance_| still uses its state until
......@@ -642,23 +636,10 @@ void EmbeddedWorkerInstance::OnRegisteredToDevToolsManager(
observer.OnRegisteredToDevToolsManager();
}
ServiceWorkerStatusCode EmbeddedWorkerInstance::SendStartWorker(
void EmbeddedWorkerInstance::SendStartWorker(
mojom::EmbeddedWorkerStartParamsPtr params,
network::mojom::URLLoaderFactoryPtr non_network_loader_factory) {
DCHECK(context_);
if (!context_->GetDispatcherHost(process_id())) {
// Check if there's a dispatcher host, which is a good sign the process is
// still alive. It's possible that previously the process crashed, and the
// Mojo connection error via |client_| detected it and this instance was
// detached, but on restart ServiceWorkerProcessManager assigned us the
// process again before RenderProcessHostImpl itself or
// ServiceWorkerProcessManager knew it crashed, and by the time we get here
// RenderProcessHostImpl::EnableSendQueue may have been called in
// anticipation of the RPHI being reused for another renderer process, so
// Mojo doesn't consider it an error. See https://crbug.com/732729.
return SERVICE_WORKER_ERROR_IPC_FAILED;
}
DCHECK(params->dispatcher_request.is_pending());
DCHECK(params->controller_request.is_pending());
DCHECK(params->service_worker_host.is_valid());
......@@ -677,12 +658,7 @@ ServiceWorkerStatusCode EmbeddedWorkerInstance::SendStartWorker(
.Run(process_id(), std::move(non_network_loader_factory));
client_->StartWorker(std::move(params));
registry_->BindWorkerToProcess(process_id(), embedded_worker_id());
OnStartWorkerMessageSent(is_script_streaming);
return SERVICE_WORKER_OK;
}
void EmbeddedWorkerInstance::OnStartWorkerMessageSent(
bool is_script_streaming) {
if (!step_time_.is_null()) {
base::TimeDelta duration = UpdateStepTime();
if (inflight_start_task_->is_installed()) {
......
content/browser/service_worker/embedded_worker_instance.h
View file @ c7079780
......@@ -244,13 +244,10 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
// |non_network_loader_factory| is non-null when the service worker script URL
// has a non-http(s) scheme. In that case, it is used to load the script since
// the usual network factory can't be used.
ServiceWorkerStatusCode SendStartWorker(
void SendStartWorker(
mojom::EmbeddedWorkerStartParamsPtr params,
network::mojom::URLLoaderFactoryPtr non_network_loader_factory);
// Called back from StartTask after a start worker message is sent.
void OnStartWorkerMessageSent(bool is_script_streaming);
// Implements mojom::EmbeddedWorkerInstanceHost.
// These functions all run on the IO thread.
void RequestTermination() override;
......
content/browser/service_worker/embedded_worker_instance_unittest.cc
View file @ c7079780
......@@ -234,13 +234,6 @@ class EmbeddedWorkerInstanceTest : public testing::Test,
worker->status_ = status;
}
ServiceWorkerStatusCode SimulateSendStartWorker(
EmbeddedWorkerInstance* worker,
mojom::EmbeddedWorkerStartParamsPtr params) {
return worker->SendStartWorker(std::move(params),
nullptr /* non_network_loader_factory */);
}
blink::mojom::ServiceWorkerInstalledScriptsInfoPtr
GetInstalledScriptsInfoPtr() {
installed_scripts_managers_.emplace_back();
......@@ -976,21 +969,4 @@ TEST_F(EmbeddedWorkerInstanceTest, AddMessageToConsole) {
EXPECT_EQ(EmbeddedWorkerStatus::STOPPED, worker->status());
}
// Test that SendStartWorker checks if dispatcher host exists.
TEST_F(EmbeddedWorkerInstanceTest, NoDispatcherHost) {
const GURL scope("http://example.com/");
const GURL url("http://example.com/worker.js");
RegistrationAndVersionPair pair = PrepareRegistrationAndVersion(scope, url);
std::unique_ptr<EmbeddedWorkerInstance> worker =
embedded_worker_registry()->CreateWorker(pair.second.get());
SetWorkerStatus(worker.get(), EmbeddedWorkerStatus::STARTING);
auto params = mojom::EmbeddedWorkerStartParams::New();
ServiceWorkerStatusCode result =
SimulateSendStartWorker(worker.get(), std::move(params));
EXPECT_EQ(SERVICE_WORKER_ERROR_IPC_FAILED, result);
// Set to STOPPED because EWInstance's destructor DCHECKs status.
SetWorkerStatus(worker.get(), EmbeddedWorkerStatus::STOPPED);
}
} // namespace content
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment