Bug 213324 – [Intl] Enable RelativeTimeFormat and Locale by default

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

[Intl] Enable RelativeTimeFormat and Locale by default

[Ross Kirsling]

Created attachment 402171 [details]
Patch

[Saam Barati]

Comment on attachment 402171 [details]
Patch

r=me
Looks like you have some test failure to fix before landing

[Yusuke Suzuki]

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.

[Ross Kirsling]

Created attachment 402184 [details]
Patch

[Ross Kirsling]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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?

[Ross Kirsling]

(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.

[Ross Kirsling]

Created attachment 402229 [details]
Patch for landing

[EWS]

Committed r263227: <https://trac.webkit.org/changeset/263227>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402229 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64502909>

[Darin Adler]

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?

[Ross Kirsling]

(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.

[Darin Adler]

(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?

[Ross Kirsling]

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>