Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

www-client/chromium: beta channel bump & drop system-icu mask #15591

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions profiles/base/package.use.mask
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ www-apps/nikola ghpages
# Bug #713590, bug #717744.
<sys-apps/man-pages-5.05-r1 l10n_de l10n_fr l10n_nl l10n_pl

# Stephan Hartmann <stha09@googlemail.com> (2020-04-11)
# Requires unreleased >=dev-libs/icu-67
>=www-client/chromium-83.0.4103.7 system-icu

# Ulrich Müller <ulm@gentoo.org> (2020-04-08)
# Old versions of libjpeg-turbo have known security issues.
# Use the bundled lib on your own risk. Bug #715106.
Expand Down
2 changes: 1 addition & 1 deletion www-client/chromium/Manifest
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
DIST chromium-81.0.4044.129.tar.xz 785978416 BLAKE2B 5af9ab9e17bbc237f5a254b98cb27b998021d5c95b5da4d6de25c3fb234fea609f8a9173f3ac75eee208b8c88c5d39d9cf1ec39ebc8d436cf8aafee31e8f32c1 SHA512 93dfc5c1050bc226b836721d422a8d98a183fff81e91f55477dce0c650d35a95aeb89c810bea6e07ffb948ee62e8e150c8b8c5bad4658fcc215de05a681b064a
DIST chromium-83.0.4103.23.tar.xz 802566932 BLAKE2B 00c9105a9b5e9cebc8adeb8a61b4491f0ca7cc95da9595506c77d556c0ee07074da0da506831bec1b390a39c7535220ab5c6d06ce784e6b20d2694f94d3b0a76 SHA512 08a50372570aa9da5fb9ccc6dfc3d949e84cb401d62d650132aa4edea2b1658fb56a81854fde11690d040e3b36b9d20da1ad7f83e47ffca77893ab57620c2a3e
DIST chromium-83.0.4103.34.tar.xz 802525184 BLAKE2B d5e47c96642fb9344fe43a582d035f507b714565c01b6bacbf1cd4fdb3537db28ae0e54a47bbd7f47ad4d00960ee9e40d9a10522262cba4063f95501225fce75 SHA512 467006d3b3093b078569c0ead9203e66cfd83ad14ed95a07b5f83e49451a0e9f4506b3ce35c97106b4540b55484d6cd33afbacf92385ace261e78d5c1cc0188e
DIST setuptools-44.1.0.zip 858569 BLAKE2B f59f154e121502a731e51294ccd293d60ffccadacf51e23b53bf7ceba38858948b86783238061136c827ac3373ea7ea8e6253d4bb53f3f1dd69284568ec65a68 SHA512 4dfb0f42d334b835758e865a26ecd1e725711fa2b9c38ddc273b8b3849fba04527bc97436d11ba1e98f1a42922aa0f0b9032e32998273c705fac6e10735eacbf
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,8 @@ BDEPEND="
)
"

: ${CHROMIUM_FORCE_CLANG=yes}
: ${CHROMIUM_FORCE_LIBCXX=yes}
: ${CHROMIUM_FORCE_CLANG=no}
: ${CHROMIUM_FORCE_LIBCXX=no}

if [[ ${CHROMIUM_FORCE_CLANG} == yes ]]; then
BDEPEND+=" >=sys-devel/clang-9"
Expand All @@ -123,7 +123,7 @@ else
dev-libs/libxslt:=
>=dev-libs/re2-0.2019.08.01:=
>=media-libs/openh264-1.6.0:=
system-icu? ( >=dev-libs/icu-65:= )
system-icu? ( >=dev-libs/icu-67.1:= )
"
RDEPEND+="${COMMON_DEPEND}"
DEPEND+="${COMMON_DEPEND}"
Expand Down Expand Up @@ -178,7 +178,9 @@ PATCHES=(
"${FILESDIR}/chromium-83-gcc-include.patch"
"${FILESDIR}/chromium-83-gcc-permissive.patch"
"${FILESDIR}/chromium-83-gcc-iterator.patch"
"${FILESDIR}/chromium-83-gcc-serviceworker.patch"
"${FILESDIR}/chromium-83-gcc-10.patch"
"${FILESDIR}/chromium-83-icu67.patch"
)

pre_build_checks() {
Expand Down
130 changes: 130 additions & 0 deletions www-client/chromium/files/chromium-83-gcc-serviceworker.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
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
170 changes: 170 additions & 0 deletions www-client/chromium/files/chromium-83-icu67.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
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;