Unify LanguageTag in SpiderMonkey
Categories
(Core :: Internationalization, task, P3)
Tracking
()
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.
Reporter | ||
Updated•3 years ago
|
Reporter | ||
Comment 1•3 years ago
|
||
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.
Reporter | ||
Comment 2•3 years ago
|
||
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.
Reporter | ||
Comment 3•3 years ago
|
||
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 | ||
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 4•3 years ago
|
||
Assignee | ||
Comment 5•3 years ago
|
||
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
Assignee | ||
Comment 6•3 years ago
|
||
Depends on D126982
Assignee | ||
Comment 7•3 years ago
|
||
Depends on D126983
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D126984
Assignee | ||
Comment 9•3 years ago
|
||
Depends on D126985
Assignee | ||
Comment 10•3 years ago
|
||
Depends on D126986
Assignee | ||
Comment 11•3 years ago
|
||
Depends on D126987
Assignee | ||
Comment 12•3 years ago
|
||
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
Assignee | ||
Comment 13•3 years ago
|
||
Depends on D126986
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 14•3 years ago
|
||
Depends on D126989
Comment 15•3 years ago
|
||
Comment 16•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5f20632de2be
https://hg.mozilla.org/mozilla-central/rev/391c305dce1f
https://hg.mozilla.org/mozilla-central/rev/4fa55bded471
https://hg.mozilla.org/mozilla-central/rev/472767f43cad
https://hg.mozilla.org/mozilla-central/rev/dce9c0d6c79c
https://hg.mozilla.org/mozilla-central/rev/b98f10477f44
https://hg.mozilla.org/mozilla-central/rev/05109157a4b5
https://hg.mozilla.org/mozilla-central/rev/e7675e3524da
Comment 17•3 years ago
|
||
I am seeing crashes after this patch when I run mochitests.
i can see two mozilla::intl::Locale
s 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]
Comment 18•3 years ago
|
||
The crash also happen with mere ./mach run
, with or without --enable-debug
.
Comment 19•3 years ago
|
||
Comment 20•3 years ago
|
||
Backed out as requested : https://hg.mozilla.org/integration/autoland/rev/39312476e05fd29b0361d2be4b0bdc907d35c8e2
Comment 21•3 years ago
|
||
Comment 22•3 years ago
|
||
This got backed out for causing crashes in debug builds. :kagami and :eeejay have more info.
Assignee | ||
Comment 23•3 years ago
|
||
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.
Assignee | ||
Comment 24•3 years ago
|
||
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
Assignee | ||
Comment 25•3 years ago
|
||
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?
Comment 29•3 years ago
|
||
Comment 31•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0f3f4c55959f
https://hg.mozilla.org/mozilla-central/rev/d28597ee9b34
https://hg.mozilla.org/mozilla-central/rev/b17cbd1bffd4
https://hg.mozilla.org/mozilla-central/rev/e6cde8c1f04b
https://hg.mozilla.org/mozilla-central/rev/4967544facf7
https://hg.mozilla.org/mozilla-central/rev/b7242c523adb
https://hg.mozilla.org/mozilla-central/rev/3776fb8cf178
https://hg.mozilla.org/mozilla-central/rev/bfa79db3a9ae
https://hg.mozilla.org/mozilla-central/rev/b821888e7c35
Description
•