Closed Bug 1719746 Opened 3 years ago Closed 3 years ago

Unify LanguageTag in SpiderMonkey

Categories

(Core :: Internationalization, task, P3)

task

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox95 --- fixed

People

(Reporter: gregtatum, Assigned: dminor)

References

Details

(Whiteboard: [i18n-unification])

Attachments

(9 files, 3 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
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
Status: RESOLVED → REOPENED
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

Created:
Updated:
Size: