1686831 - nsPrintfCString uses locale-adapted decimal separators when stringifying floats

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[:Gijs (he/him)]

See bug 1686816 for some context of why this causes bugs.

Apparently using rust would help with fixing this, but it's not entirely straightforward because nsPrintfCString delegates into mozglue right now and we don't already have a rust-based printf formatter.

Comment 1

[Mike Hommey [:glandium]]

per discussion on Matrix:

so in fact, we'd "just" need to make mozilla::PrintfTarget::cvt_f use DoubleToStringConverter instead of snprintf

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Add support for %F and %G, and properly support %n$E.

%F and %G are the same as %f and %g, but using caps for the exponent
indicator, and for "inf"/"nan" for infinity and NaN.

%n$E is the same as %E, but taking the nth argument.

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Still apply fillers when no numbers are printed.

The standard for printf says that for integers, the result of converting
zero with an explicit precision of zero shall be no characters. But
flags and width still need to apply.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Respect min_exponent_width when exponent is 0.

When using the DoubleToStringConverter with a min_exponent_width of,
say, 2, values are converted as e.g. 1.23e05 rather than 1.23e5.

In cases where an exponent of 0 is printed, though, min_exponent_width
is currently not respected, yielding e.g. 1.23e0 instead of 1.23e00.

Sent upstream to https://github.com/google/double-conversion/pull/148

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Handle trailing zero removal in double-conversion.

Bug 608915 switched nsString::AppendFloat to double-conversion, while
handling trailing zero removal on its own. This can be made more
effectively when doing so in double-conversion itself.

This replaces the need for the used_exponential_notation outparam.

This will also be useful to implement printf's %g, which has the same
behavior.

New patch sent upstream to https://github.com/google/double-conversion/pull/149

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Use double-conversion for mozilla::PrintfTarget::cvt_f.

This makes mozilla::PrintfTarget consistent across all locales (not
printing e.g. "," instead of "." for the decimal point in floats in some
locales)

This implementation passes all the glibc tests in stdio-common/tfformat.c
except two tests because of the difference in how values like e.g 0.25 are
rounded. Printf in glibc and on MacOS, as well as Rust std::fmt, round to
nearest, ties to even. Double-conversion, as well as printf on Windows
and conversion functions in ECMAScript round to nearest, ties away from
zero.

The standard for printf says rounding is implementation-defined so
either way is technically correct.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Import glibc printf test cases verbatim.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Print out the expected and actual results when failing in TestPrintf.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Hook glibc printf tests in TestPrintf.cpp.

It is worth noting that some of these tests fail on Windows for rounding
difference reasons (see later commit from this bug for more details),
and on both Windows and mac for differences in formatting for INFINITY
and NAN. All the tests pass on Linux (since the underlying printf is
currently glibc's).

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Update double-conversion to upstream revision bf46072.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Add some more double formatting tests.

Patch sent upstream. https://sourceware.org/bugzilla/show_bug.cgi?id=27245

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dcc6396e455a
Import glibc printf test cases verbatim. r=mhoye
https://hg.mozilla.org/integration/autoland/rev/b64d3e89bf68
Print out the expected and actual results when failing in TestPrintf. r=nika
https://hg.mozilla.org/integration/autoland/rev/486a184530a7
Add support for %F and %G, and properly support %n$E. r=nika,Gankra
https://hg.mozilla.org/integration/autoland/rev/dd9b7e71dcfb
Still apply fillers when no numbers are printed. r=Gankra
https://hg.mozilla.org/integration/autoland/rev/9751918b1ccb
Hook glibc printf tests in TestPrintf.cpp. r=nika,mhoye
https://hg.mozilla.org/integration/autoland/rev/c4d470be0184
Update double-conversion to upstream revision bf46072. r=nika
https://hg.mozilla.org/integration/autoland/rev/308000f1e14b
Add some more double formatting tests. r=nika
https://hg.mozilla.org/integration/autoland/rev/cf6dd6eab427
Handle trailing zero removal in double-conversion. r=nika,jwalden,Gankra
https://hg.mozilla.org/integration/autoland/rev/0e03d508c8d4
Use double-conversion for mozilla::PrintfTarget::cvt_f. r=nika

Comment 13

[Natalia Csoregi [:nataliaCs]]

Backed out for sanitizer failures on nsTSubstring.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/fd8ee841d27f5a4512cd74f9d5adf6693d0afbb4

push: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=GjoyKqavTx257ruIVQsNgA.0&revision=0e03d508c8d4be298d0423ea2589dabed4b1136c

failure log: https://treeherder.mozilla.org/logviewer?job_id=328058202&repo=autoland&lineNumber=4681

[task 2021-01-28T07:41:36.473Z] 07:41:36 INFO - TEST-START | /encoding/legacy-mb-japanese/euc-jp/eucjp-encode-href-errors-han.html?19001-20000
[task 2021-01-28T07:41:36.474Z] 07:41:36 INFO - Closing window 266
[task 2021-01-28T07:41:36.870Z] 07:41:36 INFO - PID 9050 | #0 0x7f46812f8506 in nsTSubstring<char>::AppendVprintf(char const*, __va_list_tag*) /builds/worker/checkouts/gecko/xpcom/string/nsTSubstring.cpp:1219:5
[task 2021-01-28T07:41:36.873Z] 07:41:36 INFO - PID 9050 | #1 0x7f46812b983f in nsPrintfCString::nsPrintfCString(char const*, ...) /builds/worker/workspace/obj-build/dist/include/nsPrintfCString.h:31:5
[task 2021-01-28T07:41:36.899Z] 07:41:36 INFO - PID 9050 | #2 0x7f46874e0ee3 in mozilla::dom::MediaSource::SetDuration(double, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/media/mediasource/MediaSource.cpp:218:21
[task 2021-01-28T07:41:36.938Z] 07:41:36 INFO - PID 9050 | #3 0x7f4684db96ee in mozilla::dom::MediaSource_Binding::set_duration(JSContext*, JS::Handle<JSObject*>, void*, JSJitSetterCallArgs) /builds/worker/workspace/obj-build/dom/bindings/MediaSourceBinding.cpp:227:24
[task 2021-01-28T07:41:36.944Z] 07:41:36 INFO - PID 9050 | #4 0x7f4686294e81 in bool mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/dom/bindings/BindingUtils.cpp:3181:8
[task 2021-01-28T07:41:36.959Z] 07:41:36 INFO - PID 9050 | #5 0x7f468c9c9796 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:503:13
[task 2021-01-28T07:41:36.961Z] 07:41:36 INFO - PID 9050 | #6 0x7f468c9c9796 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:594:12
[task 2021-01-28T07:41:36.962Z] 07:41:36 INFO - PID 9050 | #7 0x7f468c9cb63e in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:647:10
[task 2021-01-28T07:41:36.963Z] 07:41:36 INFO - PID 9050 | #8 0x7f468c9cb9c0 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:664:8
[task 2021-01-28T07:41:36.963Z] 07:41:36 INFO - PID 9050 | #9 0x7f468c9cd71b in js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:802:10
[task 2021-01-28T07:41:36.981Z] 07:41:36 INFO - PID 9050 | #10 0x7f468cf09abb in SetExistingProperty(JSContext*, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyResult>, JS::ObjectOpResult&) /builds/worker/checkouts/gecko/js/src/vm/NativeObject.cpp:2644:8
[task 2021-01-28T07:41:36.982Z] 07:41:36 INFO - PID 9050 | #11 0x7f468cf08f09 in bool js::NativeSetProperty<(js::QualifiedBool)1>(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) /builds/worker/checkouts/gecko/js/src/vm/NativeObject.cpp:2679:14
[task 2021-01-28T07:41:36.983Z] 07:41:36 INFO - PID 9050 | #12 0x7f468c9e00d0 in js::SetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::ObjectOpResult&) /builds/worker/checkouts/gecko/js/src/vm/ObjectOperations-inl.h:282:10
[task 2021-01-28T07:41:36.984Z] 07:41:36 INFO - PID 9050 | #13 0x7f468c9ab823 in SetPropertyOperation /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:272:10
[task 2021-01-28T07:41:36.985Z] 07:41:36 INFO - PID 9050 | #14 0x7f468c9ab823 in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3074:12
[task 2021-01-28T07:41:36.986Z] 07:41:36 INFO - PID 9050 | #15 0x7f468c994abb in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:473:13
[task 2021-01-28T07:41:36.986Z] 07:41:36 INFO - PID 9050 | #16 0x7f468c9c9599 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:619:13
[task 2021-01-28T07:41:36.987Z] 07:41:36 INFO - PID 9050 | #17 0x7f468c9cb63e in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:647:10
[task 2021-01-28T07:41:36.988Z] 07:41:36 INFO - PID 9050 | #18 0x7f468c9cb9c0 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:664:8
[task 2021-01-28T07:41:37.023Z] 07:41:37 INFO - PID 9050 | #19 0x7f468ce29dd2 in js::fun_call(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/js/src/vm/JSFunction.cpp:1100:10
[task 2021-01-28T07:41:37.023Z] 07:41:37 INFO - PID 9050 | #20 0x7f468c9c9796 in CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:503:13
[task 2021-01-28T07:41:37.023Z] 07:41:37 INFO - PID 9050 | #21 0x7f468c9c9796 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:594:12
[task 2021-01-28T07:41:37.023Z] 07:41:37 INFO - PID 9050 | #22 0x7f468c9cb63e in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:647:10
[task 2021-01-28T07:41:37.024Z] 07:41:37 INFO - PID 9050 | #23 0x7f468c9b2edd in CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:651:10
[task 2021-01-28T07:41:37.024Z] 07:41:37 INFO - PID 9050 | #24 0x7f468c9b2edd in Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3309:16
[task 2021-01-28T07:41:37.024Z] 07:41:37 INFO - PID 9050 | #25 0x7f468c994abb in js::RunScript(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:473:13
[task 2021-01-28T07:41:37.025Z] 07:41:37 INFO - PID 9050 | #26 0x7f468c9c9599 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:619:13
[task 2021-01-28T07:41:37.025Z] 07:41:37 INFO - PID 9050 | #27 0x7f468c9cb63e in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:647:10
[task 2021-01-28T07:41:37.026Z] 07:41:37 INFO - PID 9050 | #28 0x7f468c9cb9c0 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:664:8
[task 2021-01-28T07:41:37.033Z] 07:41:37 INFO - PID 9050 | #29 0x7f468d89cee2 in js::jit::InvokeFunction(JSContext*, JS::Handle<JSObject*>, bool, bool, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/jit/VMFunctions.cpp:753:10
[task 2021-01-28T07:41:37.033Z] 07:41:37 INFO - PID 9050 | #30 0x7f468d89d9b2 in js::jit::InvokeFromInterpreterStub(JSContext*, js::jit::InterpreterStubExitFrameLayout*) /builds/worker/checkouts/gecko/js/src/jit/VMFunctions.cpp:773:8
[task 2021-01-28T07:41:37.040Z] 07:41:37 INFO - PID 9050 | #31 0x7f45f67c1f93 (<unknown module>)
[task 2021-01-28T07:41:37.041Z] 07:41:37 INFO - PID 9050 | AddressSanitizer can not provide additional info.
[task 2021-01-28T07:41:37.041Z] 07:41:37 INFO - PID 9050 | SUMMARY: AddressSanitizer: SEGV /builds/worker/checkouts/gecko/xpcom/string/nsTSubstring.cpp:1219:5 in nsTSubstring<char>::AppendVprintf(char const*, __va_list_tag*)
[task 2021-01-28T07:41:37.042Z] 07:41:37 INFO - PID 9050 | ==12713==ABORTING

Another failure: https://treeherder.mozilla.org/logviewer?job_id=328058352&repo=autoland&lineNumber=3923

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/b8600d49d664
Backed out 9 changesets for sanitizer failures on nsTSubstring.cpp. CLOSED TREE

Comment 15

[Mike Hommey [:glandium]]

Attachment: Bug 1686831 - Allow double-conversion's ToFixed to handle ±DBL_MAX.

While at this point PrintfTarget doesn't use double-conversion, add a
test that it can (and thus will) handle the largest double output possible
with the default %f precision.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd4bcc71abfe
Import glibc printf test cases verbatim. r=mhoye
https://hg.mozilla.org/integration/autoland/rev/6d5eda5013da
Print out the expected and actual results when failing in TestPrintf. r=nika
https://hg.mozilla.org/integration/autoland/rev/ff182e909baf
Add support for %F and %G, and properly support %n$E. r=nika,Gankra
https://hg.mozilla.org/integration/autoland/rev/c538af90c0b0
Still apply fillers when no numbers are printed. r=Gankra
https://hg.mozilla.org/integration/autoland/rev/9e8f4956b566
Hook glibc printf tests in TestPrintf.cpp. r=nika,mhoye
https://hg.mozilla.org/integration/autoland/rev/94b425982fcd
Update double-conversion to upstream revision bf46072. r=nika
https://hg.mozilla.org/integration/autoland/rev/e31454300bd7
Add some more double formatting tests. r=nika
https://hg.mozilla.org/integration/autoland/rev/4074040827fe
Handle trailing zero removal in double-conversion. r=nika,jwalden,Gankra
https://hg.mozilla.org/integration/autoland/rev/d855f6e2e8c4
Allow double-conversion's ToFixed to handle ±DBL_MAX. r=nika
https://hg.mozilla.org/integration/autoland/rev/7e0ed9bf3fb9
Use double-conversion for mozilla::PrintfTarget::cvt_f. r=nika

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/dd4bcc71abfe
https://hg.mozilla.org/mozilla-central/rev/6d5eda5013da
https://hg.mozilla.org/mozilla-central/rev/ff182e909baf
https://hg.mozilla.org/mozilla-central/rev/c538af90c0b0
https://hg.mozilla.org/mozilla-central/rev/9e8f4956b566
https://hg.mozilla.org/mozilla-central/rev/94b425982fcd
https://hg.mozilla.org/mozilla-central/rev/e31454300bd7
https://hg.mozilla.org/mozilla-central/rev/4074040827fe
https://hg.mozilla.org/mozilla-central/rev/d855f6e2e8c4
https://hg.mozilla.org/mozilla-central/rev/7e0ed9bf3fb9

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:glandium, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.