9:19 a.m.
commit 6314d66137589969958360b4db4362c9fb8295d5
Author: qvint <dotqvint(a)gmail.com>
Date: Mon Jun 8 12:03:06 2020 +0300
Fix crash in ServiceWorker
See https://bugzilla.rpmfusion.org/show_bug.cgi?id=5671
chromium-83-gcc-r766770.patch | 134 ++++++++++++++++++++++++++++++++++++++++++
chromium-freeworld.spec | 6 +-
2 files changed, 139 insertions(+), 1 deletion(-)
---
diff --git a/chromium-83-gcc-r766770.patch b/chromium-83-gcc-r766770.patch
new file mode 100644
index 0000000..2b63ccf
--- /dev/null
+++ b/chromium-83-gcc-r766770.patch
@@ -0,0 +1,134 @@
+From bd59ce32629ef684624821419c43967b73d2989e Mon Sep 17 00:00:00 2001
+From: Hiroki Nakagawa <nhiroki(a)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(a)chromium.org>
+Commit-Queue: Hiroki Nakagawa <nhiroki(a)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(+)
+
+--- a/content/browser/service_worker/service_worker_container_host.cc
++++ b/content/browser/service_worker/service_worker_container_host.cc
+@@ -713,6 +713,16 @@ void ServiceWorkerContainerHost::RemoveS
+ 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);
+ }
+
+--- 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 : publ
+ 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, Disp
+ 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
diff --git a/chromium-freeworld.spec b/chromium-freeworld.spec
index 514cfdb..ad62344 100644
--- a/chromium-freeworld.spec
+++ b/chromium-freeworld.spec
@@ -70,7 +70,7 @@
##############################Package Definitions######################################
Name: chromium-freeworld
Version: 83.0.4103.97
-Release: 1%{?dist}
+Release: 2%{?dist}
Summary: Chromium web browser built with all freeworld codecs and VA-API support
License: BSD and LGPLv2+ and ASL 2.0 and IJG and MIT and GPLv2+ and ISC and
OpenSSL and (MPLv1.1 or GPLv2 or LGPLv2)
URL: https://www.chromium.org/Home
@@ -219,6 +219,7 @@ Patch153: chromium-83-gcc-r762806.patch
Patch154: chromium-83-gcc-10-r31184.patch
Patch155: chromium-83-gcc-10-r766427.patch
%endif
+Patch156: chromium-83-gcc-r766770.patch
# Gentoo patches (short-term fixes):
Patch250: chromium-83-gcc-include.patch
@@ -747,6 +748,9 @@ appstream-util validate-relax --nonet
"%{buildroot}%{_metainfodir}/%{name}.appda
%{chromiumdir}/swiftshader/libGLESv2.so
#########################################changelogs#################################################
%changelog
+* Mon Jun 08 2020 qvint <dotqvint(a)gmail.com> - 83.0.4103.97-2
+- Fix crash in ServiceWorker (rfbz#5671)
+
* Fri Jun 05 2020 qvint <dotqvint(a)gmail.com> - 83.0.4103.97-1
- Update to 83.0.4103.97