--- /dev/null
+From 0914a38252f205fc04fa50e858b24fa5f535ab11 Mon Sep 17 00:00:00 2001
+From: Hiroki Nakagawa <nhiroki@chromium.org>
+Date: Wed, 29 Apr 2020 11:46:54 +0900
+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
+---
+
+diff --git a/content/browser/service_worker/service_worker_container_host.cc b/content/browser/service_worker/service_worker_container_host.cc
+index c631bcd..ff917f8 100644
+--- a/content/browser/service_worker/service_worker_container_host.cc
++++ b/content/browser/service_worker/service_worker_container_host.cc
+@@ -717,6 +717,16 @@
+ 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 238cb8b..f60c7a2 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 @@
+ 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 @@
+ 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
--- /dev/null
+From 3f8dc4b2e5baf77b463334c769af85b79d8c1463 Mon Sep 17 00:00:00 2001
+From: Frank Tang <ftang@chromium.org>
+Date: Fri, 03 Apr 2020 23:13:54 -0700
+Subject: [PATCH] [intl] Remove soon-to-be removed getAllFieldPositions
+
+Needed to land ICU67.1 soon.
+
+Bug: v8:10393
+Change-Id: I3c7737ca600d6ccfdc46ffaddfb318ce60bc7618
+Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2136489
+Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
+Commit-Queue: Frank Tang <ftang@chromium.org>
+Cr-Commit-Position: refs/heads/master@{#67027}
+---
+
+diff --git a/v8/src/objects/js-number-format.cc b/v8/src/objects/js-number-format.cc
+index ad831c5..bcd4403 100644
+--- a/v8/src/objects/js-number-format.cc
++++ b/v8/src/objects/js-number-format.cc
+@@ -1241,44 +1241,33 @@
+ }
+
+ namespace {
+-Maybe<icu::UnicodeString> IcuFormatNumber(
++Maybe<bool> IcuFormatNumber(
+ Isolate* isolate,
+ const icu::number::LocalizedNumberFormatter& number_format,
+- Handle<Object> numeric_obj, icu::FieldPositionIterator* fp_iter) {
++ Handle<Object> numeric_obj, icu::number::FormattedNumber* formatted) {
+ // If it is BigInt, handle it differently.
+ UErrorCode status = U_ZERO_ERROR;
+- icu::number::FormattedNumber formatted;
+ if (numeric_obj->IsBigInt()) {
+ Handle<BigInt> big_int = Handle<BigInt>::cast(numeric_obj);
+ Handle<String> big_int_string;
+ ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, big_int_string,
+ BigInt::ToString(isolate, big_int),
+- Nothing<icu::UnicodeString>());
+- formatted = number_format.formatDecimal(
++ Nothing<bool>());
++ *formatted = number_format.formatDecimal(
+ {big_int_string->ToCString().get(), big_int_string->length()}, status);
+ } else {
+ double number = numeric_obj->IsNaN()
+ ? std::numeric_limits<double>::quiet_NaN()
+ : numeric_obj->Number();
+- formatted = number_format.formatDouble(number, status);
++ *formatted = number_format.formatDouble(number, status);
+ }
+ if (U_FAILURE(status)) {
+ // This happen because of icu data trimming trim out "unit".
+ // See https://bugs.chromium.org/p/v8/issues/detail?id=8641
+- THROW_NEW_ERROR_RETURN_VALUE(isolate,
+- NewTypeError(MessageTemplate::kIcuError),
+- Nothing<icu::UnicodeString>());
++ THROW_NEW_ERROR_RETURN_VALUE(
++ isolate, NewTypeError(MessageTemplate::kIcuError), Nothing<bool>());
+ }
+- if (fp_iter) {
+- formatted.getAllFieldPositions(*fp_iter, status);
+- }
+- icu::UnicodeString result = formatted.toString(status);
+- if (U_FAILURE(status)) {
+- THROW_NEW_ERROR_RETURN_VALUE(isolate,
+- NewTypeError(MessageTemplate::kIcuError),
+- Nothing<icu::UnicodeString>());
+- }
+- return Just(result);
++ return Just(true);
+ }
+
+ } // namespace
+@@ -1289,10 +1278,16 @@
+ Handle<Object> numeric_obj) {
+ DCHECK(numeric_obj->IsNumeric());
+
+- Maybe<icu::UnicodeString> maybe_format =
+- IcuFormatNumber(isolate, number_format, numeric_obj, nullptr);
++ icu::number::FormattedNumber formatted;
++ Maybe<bool> maybe_format =
++ IcuFormatNumber(isolate, number_format, numeric_obj, &formatted);
+ MAYBE_RETURN(maybe_format, Handle<String>());
+- return Intl::ToString(isolate, maybe_format.FromJust());
++ UErrorCode status = U_ZERO_ERROR;
++ icu::UnicodeString result = formatted.toString(status);
++ if (U_FAILURE(status)) {
++ THROW_NEW_ERROR(isolate, NewTypeError(MessageTemplate::kIcuError), String);
++ }
++ return Intl::ToString(isolate, result);
+ }
+
+ namespace {
+@@ -1405,12 +1400,18 @@
+ }
+
+ namespace {
+-Maybe<int> ConstructParts(Isolate* isolate, const icu::UnicodeString& formatted,
+- icu::FieldPositionIterator* fp_iter,
++Maybe<int> ConstructParts(Isolate* isolate,
++ icu::number::FormattedNumber* formatted,
+ Handle<JSArray> result, int start_index,
+ Handle<Object> numeric_obj, bool style_is_unit) {
++ UErrorCode status = U_ZERO_ERROR;
++ icu::UnicodeString formatted_text = formatted->toString(status);
++ if (U_FAILURE(status)) {
++ THROW_NEW_ERROR_RETURN_VALUE(
++ isolate, NewTypeError(MessageTemplate::kIcuError), Nothing<int>());
++ }
+ DCHECK(numeric_obj->IsNumeric());
+- int32_t length = formatted.length();
++ int32_t length = formatted_text.length();
+ int index = start_index;
+ if (length == 0) return Just(index);
+
+@@ -1419,13 +1420,14 @@
+ // other region covers some part of the formatted string. It's possible
+ // there's another field with exactly the same begin and end as this backdrop,
+ // in which case the backdrop's field_id of -1 will give it lower priority.
+- regions.push_back(NumberFormatSpan(-1, 0, formatted.length()));
++ regions.push_back(NumberFormatSpan(-1, 0, formatted_text.length()));
+
+ {
+- icu::FieldPosition fp;
+- while (fp_iter->next(fp)) {
+- regions.push_back(NumberFormatSpan(fp.getField(), fp.getBeginIndex(),
+- fp.getEndIndex()));
++ icu::ConstrainedFieldPosition cfp;
++ cfp.constrainCategory(UFIELD_CATEGORY_NUMBER);
++ while (formatted->nextPosition(cfp, status)) {
++ regions.push_back(
++ NumberFormatSpan(cfp.getField(), cfp.getStart(), cfp.getLimit()));
+ }
+ }
+
+@@ -1447,7 +1449,7 @@
+ Handle<String> substring;
+ ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+ isolate, substring,
+- Intl::ToString(isolate, formatted, part.begin_pos, part.end_pos),
++ Intl::ToString(isolate, formatted_text, part.begin_pos, part.end_pos),
+ Nothing<int>());
+ Intl::AddElement(isolate, result, index, field_type_string, substring);
+ ++index;
+@@ -1467,20 +1469,19 @@
+ number_format->icu_number_formatter().raw();
+ CHECK_NOT_NULL(fmt);
+
+- icu::FieldPositionIterator fp_iter;
+- Maybe<icu::UnicodeString> maybe_format =
+- IcuFormatNumber(isolate, *fmt, numeric_obj, &fp_iter);
++ icu::number::FormattedNumber formatted;
++ Maybe<bool> maybe_format =
++ IcuFormatNumber(isolate, *fmt, numeric_obj, &formatted);
+ MAYBE_RETURN(maybe_format, Handle<JSArray>());
+-
+ UErrorCode status = U_ZERO_ERROR;
++
+ bool style_is_unit =
+ Style::UNIT == StyleFromSkeleton(fmt->toSkeleton(status));
+ CHECK(U_SUCCESS(status));
+
+ Handle<JSArray> result = factory->NewJSArray(0);
+- Maybe<int> maybe_format_to_parts =
+- ConstructParts(isolate, maybe_format.FromJust(), &fp_iter, result, 0,
+- numeric_obj, style_is_unit);
++ Maybe<int> maybe_format_to_parts = ConstructParts(
++ isolate, &formatted, result, 0, numeric_obj, style_is_unit);
+ MAYBE_RETURN(maybe_format_to_parts, Handle<JSArray>());
+
+ return result;
projects / gentoo.git / commitdiff