From 4754796b384da5d11037aa2f625cec90a00e1912 Mon Sep 17 00:00:00 2001 From: Tom Callaway Date: Wed, 17 Jun 2020 10:27:35 -0400 Subject: [PATCH] revert last change, I already had that patch applied --- chromium-83-gcc-r766770.patch | 134 ---------------------------------- chromium.spec | 9 +-- 2 files changed, 4 insertions(+), 139 deletions(-) delete mode 100644 chromium-83-gcc-r766770.patch diff --git a/chromium-83-gcc-r766770.patch b/chromium-83-gcc-r766770.patch deleted file mode 100644 index 2b63ccf..0000000 --- a/chromium-83-gcc-r766770.patch +++ /dev/null @@ -1,134 +0,0 @@ -From bd59ce32629ef684624821419c43967b73d2989e Mon Sep 17 00:00:00 2001 -From: Hiroki Nakagawa -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>. - -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 -Commit-Queue: Hiroki Nakagawa -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 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 helper_; - scoped_refptr 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(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.spec b/chromium.spec index 0183449..0696d67 100644 --- a/chromium.spec +++ b/chromium.spec @@ -167,7 +167,7 @@ Name: chromium%{chromium_channel}%{nsuffix} Name: chromium%{chromium_channel} %endif Version: %{majorversion}.0.4103.97 -Release: 5%{?dist} +Release: 6%{?dist} %if %{?freeworld} %if %{?shared} # chromium-libs-media-freeworld @@ -269,9 +269,6 @@ Patch88: chromium-83-gcc-ozone-wayland.patch Patch90: chromium-83-gcc-compatibility.patch # Fix skia's handling of no_sanitize attributes to work with gcc Patch91: chromium-83.0.4103.97-skia-gcc-no_sanitize-fixes.patch -# https://chromium-review.googlesource.com/c/chromium/src/+/2094496 -# https://chromium.googlesource.com/chromium/src/+/bd59ce32629ef684624821419c43967b73d2989e -Patch92: chromium-83-gcc-r766770.patch # Use lstdc++ on EPEL7 only @@ -856,7 +853,6 @@ udev. %patch88 -p1 -b .gcc-ozone-wayland %patch90 -p1 -b .gcc-compatibility %patch91 -p1 -b .gcc-no_sanitize -%patch92 -p1 -b .gcc-r766770 # Fedora branded user agent %if 0%{?fedora} @@ -1814,6 +1810,9 @@ getent group chrome-remote-desktop >/dev/null || groupadd -r chrome-remote-deskt %changelog +* Wed Jun 17 2020 Tom Callaway - 83.0.4103.97-6 +- revert last change, I already had it. + * Tue Jun 16 2020 Tom Callaway - 83.0.4103.97-5 - add ServiceWorker fix