PKGBUILDs/extra/chromium/avoid-double-destruction-of-ServiceWorkerObjectHost.patch

From bd59ce32629ef684624821419c43967b73d2989e Mon Sep 17 00:00:00 2001
From: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Fri, 8 May 2020 08:25:31 +0000
Subject: [PATCH] ServiceWorker: Avoid double destruction of
 ServiceWorkerObjectHost on connection error

This CL avoids the case where ServiceWorkerObjectHost is destroyed twice
on ServiceWorkerObjectHost::OnConnectionError() when Chromium is built
with the GCC build toolchain.

> How does the issue happen?

ServiceWorkerObjectHost has a cyclic reference like this:

ServiceWorkerObjectHost
  --([1] scoped_refptr)--> ServiceWorkerVersion
    --([2] std::unique_ptr)--> ServiceWorkerProviderHost
      --([3] std::unique_ptr)--> ServiceWorkerContainerHost
        --([4] std::unique_ptr)--> ServiceWorkerObjectHost

Note that ServiceWorkerContainerHost manages ServiceWorkerObjectHost in
map<int64_t version_id, std::unique_ptr<ServiceWorkerObjectHost>>.

When ServiceWorkerObjectHost::OnConnectionError() is called, the
function removes the reference [4] from the map, and destroys
ServiceWorkerObjectHost. If the object host has the last reference [1]
to ServiceWorkerVersion, the destruction also cuts off the references
[2] and [3], and destroys ServiceWorkerProviderHost and
ServiceWorkerContainerHost.

This seems to work well on the Chromium's default toolchain, but not
work on the GCC toolchain. According to the report, destruction of
ServiceWorkerContainerHost happens while the map owned by the container
host is erasing the ServiceWorkerObjectHost, and this results in crash
due to double destruction of the object host.

I don't know the reason why this happens only on the GCC toolchain, but
I suspect the order of object destruction on std::map::erase() could be
different depending on the toolchains.

> How does this CL fix this?

The ideal fix is to redesign the ownership model of
ServiceWorkerVersion, but it's not feasible in the short term.

Instead, this CL avoids destruction of ServiceWorkerObjectHost on
std::map::erase(). The new code takes the ownership of the object host
from the map first, and then erases the entry from the map. This
separates timings to erase the map entry and to destroy the object host,
so the crash should no longer happen.

Bug: 1056598
Change-Id: Id30654cb575bc557c42044d6f0c6f1f9bfaed613
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2094496
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#766770}
---
 .../service_worker_container_host.cc          | 10 +++++
 .../service_worker_object_host_unittest.cc    | 38 +++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/content/browser/service_worker/service_worker_container_host.cc b/content/browser/service_worker/service_worker_container_host.cc
index ec7fb1449af..98c62093b0e 100644
--- a/content/browser/service_worker/service_worker_container_host.cc
+++ b/content/browser/service_worker/service_worker_container_host.cc
@@ -669,6 +669,16 @@ void ServiceWorkerContainerHost::RemoveServiceWorkerObjectHost(
     int64_t version_id) {
   DCHECK_CURRENTLY_ON(ServiceWorkerContext::GetCoreThreadId());
   DCHECK(base::Contains(service_worker_object_hosts_, version_id));
+
+  // ServiceWorkerObjectHost to be deleted may have the last reference to
+  // ServiceWorkerVersion that indirectly owns this ServiceWorkerContainerHost.
+  // If we erase the object host directly from the map, |this| could be deleted
+  // during the map operation and may crash. To avoid the case, we take the
+  // ownership of the object host from the map first, and then erase the entry
+  // from the map. See https://crbug.com/1056598 for details.
+  std::unique_ptr<ServiceWorkerObjectHost> to_be_deleted =
+      std::move(service_worker_object_hosts_[version_id]);
+  DCHECK(to_be_deleted);
   service_worker_object_hosts_.erase(version_id);
 }
 
diff --git a/content/browser/service_worker/service_worker_object_host_unittest.cc b/content/browser/service_worker/service_worker_object_host_unittest.cc
index 408d7c1f9d1..6eab59040ab 100644
--- a/content/browser/service_worker/service_worker_object_host_unittest.cc
+++ b/content/browser/service_worker/service_worker_object_host_unittest.cc
@@ -200,6 +200,19 @@ class ServiceWorkerObjectHostTest : public testing::Test {
     return registration_info;
   }
 
+  void CallOnConnectionError(ServiceWorkerContainerHost* container_host,
+                             int64_t version_id) {
+    // ServiceWorkerObjectHost has the last reference to the version.
+    ServiceWorkerObjectHost* object_host =
+        GetServiceWorkerObjectHost(container_host, version_id);
+    EXPECT_TRUE(object_host->version_->HasOneRef());
+
+    // Make sure that OnConnectionError induces destruction of the version and
+    // the object host.
+    object_host->receivers_.Clear();
+    object_host->OnConnectionError();
+  }
+
   BrowserTaskEnvironment task_environment_;
   std::unique_ptr<EmbeddedWorkerTestHelper> helper_;
   scoped_refptr<ServiceWorkerRegistration> registration_;
@@ -409,5 +422,30 @@ TEST_F(ServiceWorkerObjectHostTest, DispatchExtendableMessageEvent_FromClient) {
             events[0]->source_info_for_client->client_type);
 }
 
+// This is a regression test for https://crbug.com/1056598.
+TEST_F(ServiceWorkerObjectHostTest, OnConnectionError) {
+  const GURL scope("https://www.example.com/");
+  const GURL script_url("https://www.example.com/service_worker.js");
+  Initialize(std::make_unique<EmbeddedWorkerTestHelper>(base::FilePath()));
+  SetUpRegistration(scope, script_url);
+
+  // Create the provider host.
+  ASSERT_EQ(blink::ServiceWorkerStatusCode::kOk,
+            StartServiceWorker(version_.get()));
+
+  // Set up the case where the last reference to the version is owned by the
+  // service worker object host.
+  ServiceWorkerContainerHost* container_host =
+      version_->provider_host()->container_host();
+  ServiceWorkerVersion* version_rawptr = version_.get();
+  version_ = nullptr;
+  ASSERT_TRUE(version_rawptr->HasOneRef());
+
+  // Simulate the connection error that induces the object host destruction.
+  // This shouldn't crash.
+  CallOnConnectionError(container_host, version_rawptr->version_id());
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace service_worker_object_host_unittest
 }  // namespace content