Closed Bug 1719746 Opened 3 years ago Closed 3 years ago

Unify LanguageTag in SpiderMonkey

Categories

(Core :: Internationalization, task, P3)

Product:
Core
Component:
Internationalization
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
95 Branch
Tracking Flags:
Tracking Status
firefox95 --- fixed

People

(Reporter: gregtatum, Assigned: dminor)

References

Details

(Whiteboard: [i18n-unification])

Attachments

(9 files, 3 obsolete files)

Bug 1719746 - Copy LanguageTag to intl/components as Locale; r=#platform-i18n-reviewers
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719746 - Add Locale to moz.build; r=#platform-i18n-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719746 - Rework SpiderMonkey specific code in unified Locale; r=#platform-i18n-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719746 - Add gtest for Locale; r=#platform-i18n-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719746 - Use intl::Locale in SpiderMonkey; r=anba!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719746 - Update make_intl_data.py to generate LocaleGenerated.cpp; r=anba!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719746 - Add StringAsciiChars; r=#platform-i18n-reviewers!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719746 - Fix hazard analysis errors;r=anba!
48 bytes, text/x-phabricator-request
Details | Review
Bug 1719746 - Rename Locale to MozLocale; r=#platform-i18n-reviewers!
48 bytes, text/x-phabricator-request
Details | Review

https://searchfox.org/mozilla-central/source/js/src/builtin/intl/LanguageTag.cpp

This one might be a quick one to unify. It's calling out to likely subtags.

Summary: Unify IntlObject.cpp in SpiderMonkey → Unify LanguageTag in SpiderMonkey

From dminor:

The LanguageTag implementation is partly code generated from CLDR data. I had a look while doing the Intl.Locale experiments and it seems like it might be tricky to disentangle, as we'll need to make sure we don't break the ICU update process if we move it. It also doesn't map completely cleanly to ICU4X, where we're exposing Locale, and LanguageTag is closer to LanguageIdentifier in ICU4X.

Looking into this further, this one probably will be tricky, as Dan suggests. I hadn't looked into all of the code generation code. We can do the naive thing of just removing the headers by unifying the likely subtags, but I don't think this is a really good solution. I'm wondering if we could migrate LanguageTag to be backed by ICU4X only. This would probably good to brainstorm on solutions here.

LanguageTag is making it tricky to migrate DisplayNames in Bug 1719735, as the fallbacking behavior requires manipulating the language tags. The display names is tightly coupled to this API. If we can pull LanguageTag to be completely in the unified API, then it will be feasible to migrate the fallbacking behavior as well.

Blocks: 1719735

Here is a patch of the unified class, which I'm not happy with the design yet, but the tests pass. https://gist.github.com/gregtatum/d21c427268f10619b60d71c282b4fb9b

Assignee: nobody → dminor
Blocks: 1719748
Whiteboard: [i18n-unification]
Status: NEW → ASSIGNED

This moves the LanguageTag implementation from js/src/builtin/intl to
intl/components and renames them to Locale. The next commit will add
them to the build system.

Depends on D126981

Depends on D126986

Depends on D126987

This updates the script to generate intl/components/LocaleGenerated.cpp. I've
also tried to make it better match clang-format's style in places where that
wasn't too challenging.

Depends on D126988

Attachment #9243542 - Attachment is obsolete: true
Attachment #9243536 - Attachment is obsolete: true
Attachment #9243541 - Attachment is obsolete: true

Depends on D126989

Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f20632de2be
Copy LanguageTag to intl/components as Locale; r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/391c305dce1f
Add Locale to moz.build; r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/4fa55bded471
Rework SpiderMonkey specific code in unified Locale; r=platform-i18n-reviewers,gregtatum,anba
https://hg.mozilla.org/integration/autoland/rev/472767f43cad
Add gtest for Locale; r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/dce9c0d6c79c
Add StringAsciiChars; r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/b98f10477f44
Use intl::Locale in SpiderMonkey; r=anba
https://hg.mozilla.org/integration/autoland/rev/05109157a4b5
Update make_intl_data.py to generate LocaleGenerated.cpp; r=anba,gregtatum
https://hg.mozilla.org/integration/autoland/rev/e7675e3524da
Fix hazard analysis errors;r=anba

I am seeing crashes after this patch when I run mochitests.

i can see two mozilla::intl::Locales defined. rr catches one in construction and the other in destruction
the constructor used is https://searchfox.org/mozilla-central/source/intl/components/src/Locale.h#181
the destructor is from a different class and it throws a fit because mRaw is garbage: https://searchfox.org/mozilla-central/source/intl/locale/MozLocale.h#54

Native stack:

#0  0x00007f63eabf6f67 in core::ptr::drop_in_place<core::option::Option<alloc::boxed::Box<[unic_langid_impl::subtags::variant::Variant], alloc::alloc::Global>>> ()
    at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/ptr/mod.rs:192
#1  0x00007f63eabf1996 in core::ptr::drop_in_place<unic_langid_impl::LanguageIdentifier> ()
    at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/ptr/mod.rs:192
#2  0x00007f63eabf19b9 in core::ptr::drop_in_place<alloc::boxed::Box<unic_langid_impl::LanguageIdentifier, alloc::alloc::Global>> ()
    at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/ptr/mod.rs:192
#3  0x00007f63eabee0f0 in unic_langid_ffi::unic_langid_destroy (langid=0x5502000000000000)
    at intl/locale/rust/unic-langid-ffi/src/lib.rs:48
#4  0x00007f63e096c729 in mozilla::DefaultDelete<mozilla::intl::ffi::LanguageIdentifier>::operator()(mozilla::intl::ffi::LanguageIdentifier*) const (this=0x7ffde6804ec0, aPtr=0x5502000000000000)
    at /home/eitan/Mozilla/gecko/objdir-linux-tmp/dist/include/mozilla/intl/MozLocaleBindings.h:19
#5  0x00007f63e096c6bc in mozilla::UniquePtr<mozilla::intl::ffi::LanguageIdentifier, mozilla::DefaultDelete<mozilla::intl::ffi::LanguageIdentifier> >::reset(mozilla::intl::ffi::LanguageIdentifier*)
    (this=0x7ffde6804ec0, aPtr=0x0)
    at /home/eitan/Mozilla/gecko/objdir-linux-tmp/dist/include/mozilla/UniquePtr.h:305
#6  0x00007f63e096c659 in mozilla::UniquePtr<mozilla::intl::ffi::LanguageIdentifier, mozilla::DefaultDelete<mozilla::intl::ffi::LanguageIdentifier> >::~UniquePtr() (this=0x7ffde6804ec0)
    at /home/eitan/Mozilla/gecko/objdir-linux-tmp/dist/include/mozilla/UniquePtr.h:253
#7  0x00007f63e096b759 in mozilla::intl::Locale::~Locale() (this=0x7ffde6804eb8)
    at /home/eitan/Mozilla/gecko/objdir-linux-tmp/dist/include/mozilla/intl/MozLocale.h:54
#8  0x00007f63e9e4fe9d in js::intl_supportedLocaleOrFallback(JSContext*, unsigned int, JS::Value*)
    (cx=0x7f63c9d19e00, argc=1, vp=0x7f63ab5719f0)
    at /home/eitan/Mozilla/gecko/js/src/builtin/intl/IntlObject.cpp:463
#9  0x00007f63e986cf0f in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&)
    (cx=0x7f63c9d19e00, native=0x7f63e9e4f540 <js::intl_supportedLocaleOrFallback(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...)
    at /home/eitan/Mozilla/gecko/js/src/vm/Interpreter.cpp:385
#10 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) (cx=0x7f63c9d19e00, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call)
    at /home/eitan/Mozilla/gecko/js/src/vm/Interpreter.cpp:472
#11 0x00007f63e986d626 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason)
    (cx=0x7f63c9d19e00, args=..., reason=js::CallReason::Call)
    at /home/eitan/Mozilla/gecko/js/src/vm/Interpreter.cpp:532
#12 0x00007f63e986d4bf in js::CallFromStack(JSContext*, JS::CallArgs const&)
    (cx=0x7f63c9d19e00, args=...) at /home/eitan/Mozilla/gecko/js/src/vm/Interpreter.cpp:536
#13 0x00007f63e9862583 in Interpret(JSContext*, js::RunState&) (cx=0x7f63c9d19e00, state=...)
    at /home/eitan/Mozilla/gecko/js/src/vm/Interpreter.cpp:3240
#14 0x00007f63e9850e6b in js::RunScript(JSContext*, js::RunState&) (cx=0x7f63c9d19e00, state=...)
    at /home/eitan/Mozilla/gecko/js/src/vm/Interpreter.cpp:354
#15 0x00007f63e986d181 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)
    (cx=0x7f63c9d19e00, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call)
    at /home/eitan/Mozilla/gecko/js/src/vm/Interpreter.cpp:504

JS Stack:

0 DefaultLocale() ["self-hosted":5795:22]
1 LookupMatcher(availableLocales = ""Collator"", requestedLocales = "") ["self-hosted":5838:22]
2 BestFitMatcher(availableLocales = ""Collator"", requestedLocales = "") ["self-hosted":5854:23]
3 ResolveLocale(availableLocales = ""Collator"", requestedLocales = ""    <Failed to get argument while inspecting stack frame>
, relevantExtensionKeys = "co,kf,kn", localeData = [function]) ["self-hosted":5887:22]
4 resolveCollatorInternals(    <Failed to get argument while inspecting stack frame>
) ["self-hosted":5558:33]
5 getInternals(obj = "[object Intl.Collator]") ["self-hosted":6080:21]
    this = null
6 localeCompare(that = ""http://mochi.test:8888/redirect.html"") ["self-hosted":4163:29]
    this = http://mochi.test:8888/redirect.html
7 Links_compareLinks(aLink1 = "[object Object]", aLink2 = "[object Object]") ["resource://gre/modules/NewTabUtils.jsm":1816:17]
    this = [object Object]
8 Links_compareLinks("[object Object]", "[object Object]") ["self-hosted":1175:26]
9 search(comparator = [function], array = "[object Object]", target = "[object Object]") ["resource://gre/modules/BinarySearch.jsm":62:26]
    this = [object Object]
10 indexOf(comparator = [function], array = "[object Object]", target = "[object Object]") ["resource://gre/modules/BinarySearch.jsm":19:28]
    this = [object Object]
11 Links__binsearch(aArray = "[object Object]", aLink = "[object Object]", aMethod = ""indexOf"") ["resource://gre/modules/NewTabUtils.jsm":2099:32]
    this = [object Object]
12 Links__indexOf(aArray = "[object Object]", aLink = "[object Object]") ["resource://gre/modules/NewTabUtils.jsm":2091:16]
    this = [object Object]
13 Links_onLinkChanged(aProvider = "[object Object]", aLink = "[object Object]") ["resource://gre/modules/NewTabUtils.jsm":2015:21]
    this = [object Object]
14 PlacesProvider__callObservers(aMethodName = ""onLinkChanged"", aArg = "[object Object]") ["resource://gre/modules/NewTabUtils.jsm":730:26]
    this = [object Object]
15 handlePlacesEvents(aEvents = "[object PlacesVisitTitle]") ["resource://gre/modules/NewTabUtils.jsm":712:15]
    this = [object Object]
16 handlePlacesEvents("[object PlacesVisitTitle]") ["self-hosted":1175:26]

The crash also happen with mere ./mach run, with or without --enable-debug.

Backout by abutkovits@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39312476e05f
Backed out 9 changesets (bug 1719746, bug 1735341) as requested by dev. CLOSED TREE

Backed out as requested : https://hg.mozilla.org/integration/autoland/rev/39312476e05fd29b0361d2be4b0bdc907d35c8e2

Status: RESOLVED → REOPENED
status-firefox95: fixed → ---
Resolution: FIXED → ---
Target Milestone: 95 Branch → ---
Backout by abutkovits@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/a7b96cf65cfc
Backed out 9 changesets (bug 1719746, bug 1735341) as requested by dev. CLOSED TREE

This got backed out for causing crashes in debug builds. :kagami and :eeejay have more info.

Flags: needinfo?(dminor)

It looks like the problems are the result of having two identically named mozilla::intl::Locale classes, one in intl/locale/MozLocale and the other in intl/components/Locale :(. I'm going to rename the intl/locale version to MozLocale to get this landed. I've filed Bug 1736017 to clean this up properly, we should move callers of the MozLocale version to the intl/components version.

Flags: needinfo?(dminor)

There is also a mozilla::intl::Locale in intl/locale/MozLocale.h. This naming
collision was causing crashes in debug builds where the wrong destructor ended
up being called. This patch renames the intl/locale/MozLocale.h class to
MozLocale.

I've filed Bug 1736017 to move callers of the MozLocale version to the
unified intl/components/Locale version.

Depends on D128317

Unfortunately, I'm not able to reproduce the crash on my system. I have a try run here: https://treeherder.mozilla.org/jobs?repo=try&revision=73cc56111c5013478b80bb58c08fac29458f13c2, :eeejay or :saschanaz, would it be possible for one of you to check that the rename does in fact fix the crash?

Flags: needinfo?(krosylight)
Flags: needinfo?(eitan)
Depends on: 1736081

Thanks, that works 👍

Flags: needinfo?(krosylight)

Thanks for checking!

Flags: needinfo?(eitan)
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0f3f4c55959f
Copy LanguageTag to intl/components as Locale; r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/d28597ee9b34
Add Locale to moz.build; r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/b17cbd1bffd4
Rework SpiderMonkey specific code in unified Locale; r=platform-i18n-reviewers,gregtatum,anba
https://hg.mozilla.org/integration/autoland/rev/e6cde8c1f04b
Add gtest for Locale; r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/4967544facf7
Add StringAsciiChars; r=platform-i18n-reviewers,gregtatum
https://hg.mozilla.org/integration/autoland/rev/b7242c523adb
Use intl::Locale in SpiderMonkey; r=anba
https://hg.mozilla.org/integration/autoland/rev/3776fb8cf178
Update make_intl_data.py to generate LocaleGenerated.cpp; r=anba,gregtatum
https://hg.mozilla.org/integration/autoland/rev/bfa79db3a9ae
Fix hazard analysis errors;r=anba
https://hg.mozilla.org/integration/autoland/rev/b821888e7c35
Rename Locale to MozLocale; r=platform-i18n-reviewers,zbraniecki
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: