Ignoring U_LB_COUNT being deprecated in 58.1 bug 1315986 enforces https://ssl.icu-project.org/trac/changeset/38753 baggage. If Unicode 9.0 is really required by Gecko, please, update build/autoconf/icu.m4 $ echo ac_add_options --with-system-icu >>.mozconfig $ ./mach build [...] In file included from objdir/intl/lwbrk/Unified_cpp_intl_lwbrk0.cpp:2: intl/lwbrk/nsJISx4051LineBreaker.cpp:557:3: error: static_assert failed "Gecko vs ICU LineBreak class mismatch" static_assert(U_LB_COUNT == mozilla::ArrayLength(sUnicodeLineBreakToClass), ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. $ pkg info -x icu icu-57.1,1 $ fgrep -r U_LB_COUNT /usr/local/include /usr/local/include/unicode/uchar.h: U_LB_COUNT = 40

Hmm... I'm not entirely sure which way to go here. We could modify the static_assert to use <= instead of ==, so that we can still build against older ICU versions; that's safe as far as the specific problem from bug 1315986 is concerned. But we'll shortly be updating other data in Gecko to Unicode 9.0 (in bug 1281448 -- I started on a patch but ran into an unexpected issue that needs diagnosing before it's ready to land). So at that point, Gecko will expect to be supporting Unicode 9, and if we then use an older version of ICU, I'd expect to (at least) see some test failures due to the mismatch in supported versions. Maybe people using --with-system-icu don't care if the resulting product doesn't fully implement the same Unicode version as "stock" Firefox, and as a result doesn't pass all our tests? That seems unfortunate to me, though. I'm more inclined to say we should update the requirement in icu.m4, so that we can count on a known Unicode version for a given Gecko release.

Can you also fix assert to not break with U_LB_COUNT > 43 i.e., if downstream has more up-to-date ICU than Mozilla in future ?

(In reply to Jan Beich from comment #4) > Can you also fix assert to not break with U_LB_COUNT > 43 i.e., if > downstream has more up-to-date ICU than Mozilla in future ? That would be a problem, because the gecko code wouldn't know how to handle new ULineBreak values that might be returned by a newer ICU.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5e948205052d195f08bff9ef5d750f15de4c9d Bug 1316540 - Update version requirement for --with-system-icu to 58.1 for Unicode 9 support. r=waldo

Please request Aurora approval on this when you get a chance.

(In reply to Jonathan Kew (:jfkthame) from comment #5) > (In reply to Jan Beich from comment #4) > > Can you also fix assert to not break with U_LB_COUNT > 43 i.e., if > > downstream has more up-to-date ICU than Mozilla in future ? > > That would be a problem, because the gecko code wouldn't know how to handle > new ULineBreak values that might be returned by a newer ICU. Did Gecko know how to before bug 1299615 landed if built --with-system-icu ? Otherwise, bug 1315986 impacts downstream --with-system-icu where ICU >= 58.

In principle we could add #ifs testing ICU version to handle multiple variants, if it were really important. However, such #ifs are much less maintainable than just bumping ICU version, so I'd rather avoid them. (Also, we periodically patch ICU with patches upstream has accepted, and at that point system-icu becomes temporarily unusable anyway.)

Bumping ICU version fixes mozilla-central. What about release branches? For one, downstream maybe be using ICU 58.1 + --with-system-icu on Firefox 50.

Unless I'm misunderstanding, the backport would only be a single branch, for an uplift that happened a week ago. That seems little enough time that backporting further seems like the right decision. If we'd discovered this several weeks later, it might be a different matter.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13) > Unless I'm misunderstanding, the backport would only be a single branch, for > an uplift that happened a week ago. Correct, bug 1316540 (this one) has to chase bug 1299615 to Aurora. If comment 5 is true for --with-system-icu *before* bug 1299615 then bug 1315986 has to chase at least bug 1265631 to Release as well. Bug 1299615 Part 6 may have similar impact. (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13) > Also, we periodically patch ICU with patches upstream has accepted, > and at that point system-icu becomes temporarily unusable anyway From downstream POV backporting an upstream fix that doesn't break backward compatibility is regular occurence. What would be a problem if Mozilla forks old ICU version (like with Cairo) or exposes newer ICU to a vulnerability because forward compatibility is NPOTB.

(In reply to Jan Beich from comment #14) > If > comment 5 is true for --with-system-icu *before* bug 1299615 then bug > 1315986 has to chase at least bug 1265631 to Release as well. Yes, I believe this is the case. If I'm following correctly, when firefox 49..51 (including bug 1265631) or later is built with ICU 58, you'll run into the crash from bug 1315986. (Note that there might be other similar issues where using a newer ICU version than gecko was aware of could lead to problems, though I am not currently aware of other examples.)

Comment on attachment 8809326 [details] [diff] [review] Update version requirement for --with-system-icu to 58.1 for Unicode 9 support Approval Request Comment [Feature/regressing bug #]: 1299615 [User impact if declined]: build failure using --with-system-icu if the system ICU is too old [Describe test coverage new/current, TreeHerder]: n/a (does not affect mozilla builds) [Risks and why]: minimal, just update --with-system-icu version requirement to match in-tree ICU version [String/UUID change made/needed]: n/a

Comment on attachment 8809326 [details] [diff] [review] Update version requirement for --with-system-icu to 58.1 for Unicode 9 support take this fix for --with-system-icu builds of aurora52