[Intl] Enable RelativeTimeFormat and Locale by default
Created attachment 402171 [details] Patch
Comment on attachment 402171 [details] Patch r=me Looks like you have some test failure to fix before landing
Oh! Interesting, this looks like a real bug. The bug is that we are deleting UNumberFormat twice. 151 UErrorCode status = U_ZERO_ERROR; 152 m_numberFormat = std::unique_ptr<UNumberFormat, UNumberFormatDeleter>(unum_open(UNUM_DECIMAL, nullptr, 0, dataLocaleWithExtensions.data(), nullptr, &status)); 153 if (UNLIKELY(U_FAILURE(status))) { 154 throwTypeError(globalObject, scope, "failed to initialize RelativeTimeFormat"_s); 155 return; 156 } 157 158 m_relativeDateTimeFormatter = std::unique_ptr<URelativeDateTimeFormatter, URelativeDateTimeFormatterDeleter>(ureldatefmt_open(dataLocaleWithExtensions.data(), m_numberFormat.get(), m_style, UDISPCTX_CAPITALIZATION_FOR_STANDALONE, &status)); 159 if (UNLIKELY(U_FAILURE(status))) { 160 throwTypeError(globalObject, scope, "failed to initialize RelativeTimeFormat"_s); 161 return; 162 } 163 } In L158, we pass m_numberFormat.get() while it is owned by m_numberFormat's unique_ptr. But according to the document, https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/ureldatefmt_8h.html#ad321349fc3e3f6da7c0b1a542dc49ffb > nfToAdopt A number formatter to set for this URelativeDateTimeFormatter object (instead of the default decimal formatter). Ownership of this UNumberFormat object will pass to the URelativeDateTimeFormatter object (the URelativeDateTimeFormatter adopts the UNumberFormat), which becomes responsible for closing it. If the caller wishes to retain ownership of the UNumberFormat object, the caller must clone it (with unum_clone) and pass the clone to ureldatefmt_open. May be NULL to use the default decimal formatter. the ownership will be adopted by URelativeDateTimeFormatter.
Created attachment 402184 [details] Patch
Thanks for helping me identify the issue! Asking for review again just to make sure I'm not overlooking some established pattern for this sort of thing.
Comment on attachment 402184 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402184&action=review r=me with one comment. > Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:155 > + m_relativeDateTimeFormatter = std::unique_ptr<URelativeDateTimeFormatter, URelativeDateTimeFormatterDeleter>(ureldatefmt_open(dataLocaleWithExtensions.data(), m_rawNumberFormat, m_style, UDISPCTX_CAPITALIZATION_FOR_STANDALONE, &status)); > if (UNLIKELY(U_FAILURE(status))) { > throwTypeError(globalObject, scope, "failed to initialize RelativeTimeFormat"_s); > return; I think we could leak m_rawNumberFormat if opening m_relativeDateTimeFormatter failed here.
Comment on attachment 402184 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402184&action=review >> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:155 >> return; > > I think we could leak m_rawNumberFormat if opening m_relativeDateTimeFormatter failed here. Can you check the semantics of ownership of numberFormat when the opening fails in ICU?
(In reply to Yusuke Suzuki from comment #7) > Comment on attachment 402184 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402184&action=review > > >> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:155 > >> return; > > > > I think we could leak m_rawNumberFormat if opening m_relativeDateTimeFormatter failed here. > > Can you check the semantics of ownership of numberFormat when the opening > fails in ICU? Thanks for catching that! Looks like the only way this should be able to fail for us is if ICU fails to allocate a SharedNumberFormat for the UNumberFormat we pass in: https://github.com/unicode-org/icu/blob/master/icu4c/source/i18n/reldatefmt.cpp#L1240 So it should be safe for us to unum_close here.
Created attachment 402229 [details] Patch for landing
Committed r263227: <https://trac.webkit.org/changeset/263227> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402229 [details].
<rdar://problem/64502909>
Comment on attachment 402229 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=402229&action=review > Source/JavaScriptCore/runtime/IntlObject.cpp:155 > -void IntlObject::finishCreation(VM& vm, JSGlobalObject* globalObject) > +void IntlObject::finishCreation(VM& vm, JSGlobalObject*) > { > Base::finishCreation(vm); > - if (Options::useIntlLocale()) { > - auto* localeConstructor = IntlLocaleConstructor::create(vm, IntlLocaleConstructor::createStructure(vm, globalObject, globalObject->functionPrototype()), jsCast<IntlLocalePrototype*>(globalObject->localeStructure()->storedPrototypeObject())); > - putDirectWithoutTransition(vm, vm.propertyNames->Locale, localeConstructor, static_cast<unsigned>(PropertyAttribute::DontEnum)); > - } > - if (Options::useIntlRelativeTimeFormat()) { > - auto* relativeTimeFormatConstructor = IntlRelativeTimeFormatConstructor::create(vm, IntlRelativeTimeFormatConstructor::createStructure(vm, globalObject, globalObject->functionPrototype()), jsCast<IntlRelativeTimeFormatPrototype*>(globalObject->relativeTimeFormatStructure()->storedPrototypeObject())); > - putDirectWithoutTransition(vm, vm.propertyNames->RelativeTimeFormat, relativeTimeFormatConstructor, static_cast<unsigned>(PropertyAttribute::DontEnum)); > - } > } Can we remove this now-empty function somehow? > Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:280 > + status = callBufferProducingFunction(unum_formatDoubleForFields, m_rawNumberFormat, absValue, buffer, iterator.get()); Seems fragile to just use this. Not sure we’re guaranteed that the adopted formatter can continue to be used. Could we clone instead? Or find a way to do this formatting with ureldatefmt calls?
(In reply to Darin Adler from comment #12) > Comment on attachment 402229 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402229&action=review > > > Source/JavaScriptCore/runtime/IntlObject.cpp:155 > > -void IntlObject::finishCreation(VM& vm, JSGlobalObject* globalObject) > > +void IntlObject::finishCreation(VM& vm, JSGlobalObject*) > > { > > Base::finishCreation(vm); > > - if (Options::useIntlLocale()) { > > - auto* localeConstructor = IntlLocaleConstructor::create(vm, IntlLocaleConstructor::createStructure(vm, globalObject, globalObject->functionPrototype()), jsCast<IntlLocalePrototype*>(globalObject->localeStructure()->storedPrototypeObject())); > > - putDirectWithoutTransition(vm, vm.propertyNames->Locale, localeConstructor, static_cast<unsigned>(PropertyAttribute::DontEnum)); > > - } > > - if (Options::useIntlRelativeTimeFormat()) { > > - auto* relativeTimeFormatConstructor = IntlRelativeTimeFormatConstructor::create(vm, IntlRelativeTimeFormatConstructor::createStructure(vm, globalObject, globalObject->functionPrototype()), jsCast<IntlRelativeTimeFormatPrototype*>(globalObject->relativeTimeFormatStructure()->storedPrototypeObject())); > > - putDirectWithoutTransition(vm, vm.propertyNames->RelativeTimeFormat, relativeTimeFormatConstructor, static_cast<unsigned>(PropertyAttribute::DontEnum)); > > - } > > } > > Can we remove this now-empty function somehow? I was initially going to, but we're going to need to do the same add-and-remove dance for every new Intl feature. > > Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.cpp:280 > > + status = callBufferProducingFunction(unum_formatDoubleForFields, m_rawNumberFormat, absValue, buffer, iterator.get()); > > Seems fragile to just use this. Not sure we’re guaranteed that the adopted > formatter can continue to be used. Could we clone instead? Or find a way to > do this formatting with ureldatefmt calls? The UNumberFormat should be guaranteed to live as long as the URelativeDateTimeFormatter does. We control the lifetime of the latter and assert that it's been instantiated correctly inside the formatInternal call a few lines prior (though we could assert here too if we'd like). We certainly can clone, but it seems very silly -- the only reason we're passing in a UNumberFormat in the first place (instead of letting the URelativeDateTimeFormatter create a default one internally) is because we need direct access to it here.
(In reply to Ross Kirsling from comment #13) > (In reply to Darin Adler from comment #12) > > Seems fragile to just use this. Not sure we’re guaranteed that the adopted > > formatter can continue to be used. Could we clone instead? Or find a way to > > do this formatting with ureldatefmt calls? > > The UNumberFormat should be guaranteed to live as long as the > URelativeDateTimeFormatter does. Why? Couldn’t a future version consume the UNumberFormat instead of keeping a pointer to it? I’m not sure I understand the API contract fully; if you’re sure, I will back off. > the only reason we're passing in a UNumberFormat in the first place (instead of letting the URelativeDateTimeFormatter create a default one internally) is because we need direct access to it here Can we just create our own and let URelativeDateTimeFormatter create its own? Is it important that they are the same object? A performance optimization? Affects behavior and correctness?
You're right that the contract I'm assuming isn't explicit, so yeah, better safe than sorry. Committed r263243: <https://trac.webkit.org/changeset/263243>