Bug 840706 - Part 2: Make ToDouble and ToFloat more spec-compliant, r=smaug,xpcom-reviewers,barret

The main use of this method appears to be when parsing attributes with floating
point values in HTML. By changing from PR_strtod, this change does have some
semantic differences around edge-cases, though I've tried to keep it compliant
with the wording in the standard.

Here are some of the edge-cases I investigated:

1. We appear to build nspr with INFNAN_CHECK undefined, meaning that we did not
   parse strings like "inf", "infinity" and "nan" both before and after this
   change.
2. PR_strtod already ignored locale (built with USE_LOCALE undefined), so
   locale behaviour shouldn't have changed.
3. Neither PR_strtod nor the spec support hex floats, so I didn't enable them
   in double-conversion.
4. Leading whitespace is skipped by both PR_strtod and in the spec, so is
   skipped after this change.
5. Numbers too large to fit in a double (e.g. 2E308) were previously returned
   as inf or -inf by PR_strtod. The spec specifies that these values should return
   an error instead, so they have been changed to produce an error with this
   change.
   This is a web-visible change (<progress value=2E308 max=10> is indeterminate
   in Chromium and complete in Firefox before this change, it will be
   indeterminate for both after the change).
6. Parsing for floats & doubles are now handled seperately due to
   differences in the maximum and minimum representable values.

Differential Revision: https://phabricator.services.mozilla.com/D148305

xpcom/string/nsTString.cpp

@@ -26,40 +26,6 @@ bool nsTString<T>::SetCharAt(char16_t aChar, index_type aIndex) {
   return true;
 }
 
-/**
- * nsTString::ToDouble
- */
-template <>
-double nsTString<char>::ToDouble(bool aAllowTrailingChars,
-                                 nsresult* aErrorCode) const {
-  double res = 0.0;
-  if (this->Length() > 0) {
-    char* conv_stopped;
-    const char* str = this->get();
-    // Use PR_strtod, not strtod, since we don't want locale involved.
-    res = PR_strtod(str, &conv_stopped);
-    if (aAllowTrailingChars && conv_stopped != str) {
-      *aErrorCode = NS_OK;
-    } else if (!aAllowTrailingChars && conv_stopped == str + this->Length()) {
-      *aErrorCode = NS_OK;
-    } else {
-      *aErrorCode = NS_ERROR_ILLEGAL_VALUE;
-    }
-  } else {
-    // The string was too short (0 characters)
-    *aErrorCode = NS_ERROR_ILLEGAL_VALUE;
-  }
-  return res;
-}
-
-template <>
-double nsTString<char16_t>::ToDouble(bool aAllowTrailingChars,
-                                     nsresult* aErrorCode) const {
-  NS_LossyConvertUTF16toASCII cString(BeginReading(), Length());
-  return aAllowTrailingChars ? cString.ToDoubleAllowTrailingChars(aErrorCode)
-                             : cString.ToDouble(aErrorCode);
-}
-
 template <typename T>
 void nsTString<T>::Rebind(const char_type* data, size_type length) {
   // If we currently own a buffer, release it.

xpcom/string/nsTString.h

@@ -180,37 +180,6 @@ class nsTString : public nsTSubstring<T> {
 
   char_type operator[](index_type aIndex) const { return CharAt(aIndex); }
 
-  /**
-   * Perform string to double-precision float conversion.
-   *
-   * @param   aErrorCode will contain error if one occurs
-   * @return  double-precision float rep of string value
-   */
-  double ToDouble(nsresult* aErrorCode) const {
-    return ToDouble(/* aAllowTrailingChars = */ false, aErrorCode);
-  }
-
-  /**
-   * Perform string to single-precision float conversion.
-   *
-   * @param   aErrorCode will contain error if one occurs
-   * @return  single-precision float rep of string value
-   */
-  float ToFloat(nsresult* aErrorCode) const {
-    return float(ToDouble(aErrorCode));
-  }
-
-  /**
-   * Similar to above ToDouble and ToFloat but allows trailing characters that
-   * are not converted.
-   */
-  double ToDoubleAllowTrailingChars(nsresult* aErrorCode) const {
-    return ToDouble(/* aAllowTrailingChars = */ true, aErrorCode);
-  }
-  float ToFloatAllowTrailingChars(nsresult* aErrorCode) const {
-    return float(ToDoubleAllowTrailingChars(aErrorCode));
-  }
-
   /**
    * Set a char inside this string at given index
    *
@@ -248,8 +217,6 @@ class nsTString : public nsTSubstring<T> {
   friend const nsTString<char>& VoidCString();
   friend const nsTString<char16_t>& VoidString();
 
-  double ToDouble(bool aAllowTrailingChars, nsresult* aErrorCode) const;
-
   // Used by Null[C]String.
   explicit nsTString(DataFlags aDataFlags)
       : substring_type(char_traits::sEmptyBuffer, 0,
@@ -257,13 +224,6 @@ class nsTString : public nsTSubstring<T> {
                        ClassFlags::NULL_TERMINATED) {}
 };
 
-template <>
-double nsTString<char>::ToDouble(bool aAllowTrailingChars,
-                                 nsresult* aErrorCode) const;
-template <>
-double nsTString<char16_t>::ToDouble(bool aAllowTrailingChars,
-                                     nsresult* aErrorCode) const;
-
 extern template class nsTString<char>;
 extern template class nsTString<char16_t>;
 

xpcom/string/nsTStringRepr.cpp

@@ -6,6 +6,9 @@
 
 #include "nsTStringRepr.h"
 
+#include "double-conversion/string-to-double.h"
+#include "mozilla/FloatingPoint.h"
+#include "nsError.h"
 #include "nsString.h"
 
 namespace mozilla::detail {
@@ -181,6 +184,89 @@ bool nsTStringRepr<T>::EqualsIgnoreCase(const std::string_view& aString) const {
                     });
 }
 
+// We can't use the method `StringToDoubleConverter::ToDouble` due to linking
+// issues on Windows as it's in mozglue. Instead, implement the selection logic
+// using an overload set.
+//
+// StringToFloat is used instead of StringToDouble for floats due to differences
+// in rounding behaviour.
+static void StringToFP(
+    const double_conversion::StringToDoubleConverter& aConverter,
+    const char* aData, size_t aLength, int* aProcessed, float* aResult) {
+  *aResult = aConverter.StringToFloat(aData, aLength, aProcessed);
+}
+
+static void StringToFP(
+    const double_conversion::StringToDoubleConverter& aConverter,
+    const char* aData, size_t aLength, int* aProcessed, double* aResult) {
+  *aResult = aConverter.StringToDouble(aData, aLength, aProcessed);
+}
+
+static void StringToFP(
+    const double_conversion::StringToDoubleConverter& aConverter,
+    const char16_t* aData, size_t aLength, int* aProcessed, float* aResult) {
+  *aResult = aConverter.StringToFloat(reinterpret_cast<const uc16*>(aData),
+                                      aLength, aProcessed);
+}
+
+static void StringToFP(
+    const double_conversion::StringToDoubleConverter& aConverter,
+    const char16_t* aData, size_t aLength, int* aProcessed, double* aResult) {
+  *aResult = aConverter.StringToDouble(reinterpret_cast<const uc16*>(aData),
+                                       aLength, aProcessed);
+}
+
+template <typename FloatT, typename CharT>
+static FloatT ParseFloatingPoint(const nsTStringRepr<CharT>& aString,
+                                 bool aAllowTrailingChars,
+                                 nsresult* aErrorCode) {
+  // Performs conversion to double following the "rules for parsing
+  // floating-point number values" from the HTML standard.
+  // https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values
+  //
+  // This behaviour allows for leading spaces, and will not generate infinity or
+  // NaN values except in error conditions.
+  int flags = double_conversion::StringToDoubleConverter::ALLOW_LEADING_SPACES;
+  if (aAllowTrailingChars) {
+    flags |= double_conversion::StringToDoubleConverter::ALLOW_TRAILING_JUNK;
+  }
+  double_conversion::StringToDoubleConverter converter(
+      flags, mozilla::UnspecifiedNaN<double>(),
+      mozilla::UnspecifiedNaN<double>(), nullptr, nullptr);
+
+  FloatT result;
+  int processed;
+  StringToFP(converter, aString.Data(), aString.Length(), &processed, &result);
+
+  *aErrorCode = mozilla::IsFinite(result) ? NS_OK : NS_ERROR_ILLEGAL_VALUE;
+  return result;
+}
+
+template <typename T>
+double nsTStringRepr<T>::ToDouble(nsresult* aErrorCode) const {
+  return ParseFloatingPoint<double, T>(*this, /* aAllowTrailingChars */ false,
+                                       aErrorCode);
+}
+
+template <typename T>
+double nsTStringRepr<T>::ToDoubleAllowTrailingChars(
+    nsresult* aErrorCode) const {
+  return ParseFloatingPoint<double, T>(*this, /* aAllowTrailingChars */ true,
+                                       aErrorCode);
+}
+
+template <typename T>
+float nsTStringRepr<T>::ToFloat(nsresult* aErrorCode) const {
+  return ParseFloatingPoint<float, T>(*this, /* aAllowTrailingChars */ false,
+                                      aErrorCode);
+}
+
+template <typename T>
+float nsTStringRepr<T>::ToFloatAllowTrailingChars(nsresult* aErrorCode) const {
+  return ParseFloatingPoint<float, T>(*this, /* aAllowTrailingChars */ true,
+                                      aErrorCode);
+}
+
 }  // namespace mozilla::detail
 
 template class mozilla::detail::nsTStringRepr<char>;

xpcom/string/nsTStringRepr.h

@@ -420,6 +420,35 @@ class nsTStringRepr {
 
   int32_t RFindCharInSet(const string_view& aSet, int32_t aOffset = -1) const;
 
+  /**
+   * Perform locale-independent string to double-precision float conversion.
+   *
+   * Leading spaces in the string will be ignored. The returned value will be
+   * finite unless aErrorCode is set to a failed status.
+   *
+   * @param   aErrorCode will contain error if one occurs
+   * @return  double-precision float rep of string value
+   */
+  double ToDouble(nsresult* aErrorCode) const;
+
+  /**
+   * Perform locale-independent string to single-precision float conversion.
+   *
+   * Leading spaces in the string will be ignored. The returned value will be
+   * finite unless aErrorCode is set to a failed status.
+   *
+   * @param   aErrorCode will contain error if one occurs
+   * @return  single-precision float rep of string value
+   */
+  float ToFloat(nsresult* aErrorCode) const;
+
+  /**
+   * Similar to above ToDouble and ToFloat but allows trailing characters that
+   * are not converted.
+   */
+  double ToDoubleAllowTrailingChars(nsresult* aErrorCode) const;
+  float ToFloatAllowTrailingChars(nsresult* aErrorCode) const;
+
  protected:
   nsTStringRepr() = delete;  // Never instantiate directly
 

xpcom/tests/gtest/TestStrings.cpp

@@ -46,6 +46,9 @@ using mozilla::BlackBox;
 using mozilla::fallible;
 using mozilla::IsAscii;
 using mozilla::IsUtf8;
+using mozilla::Maybe;
+using mozilla::Nothing;
+using mozilla::Some;
 using mozilla::Span;
 
 #define TestExample1                                                           \
@@ -1539,102 +1542,134 @@ TEST_F(Strings, huge_capacity) {
   }
 }
 
-static void test_tofloat_helper(const nsString& aStr, float aExpected,
+static void test_tofloat_helper(const nsString& aStr,
-                                bool aSuccess) {
+                                mozilla::Maybe<float> aExpected) {
   nsresult result;
-  EXPECT_EQ(aStr.ToFloat(&result), aExpected);
+  float value = aStr.ToFloat(&result);
-  if (aSuccess) {
+  if (aExpected) {
-    EXPECT_EQ(result, NS_OK);
+    EXPECT_TRUE(NS_SUCCEEDED(result));
+    EXPECT_EQ(value, *aExpected);
   } else {
-    EXPECT_NE(result, NS_OK);
+    EXPECT_TRUE(NS_FAILED(result));
   }
 }
 
 TEST_F(Strings, tofloat) {
-  test_tofloat_helper(u"42"_ns, 42.f, true);
+  test_tofloat_helper(u"42"_ns, Some(42.f));
-  test_tofloat_helper(u"42.0"_ns, 42.f, true);
+  test_tofloat_helper(u"42.0"_ns, Some(42.f));
-  test_tofloat_helper(u"-42"_ns, -42.f, true);
+  test_tofloat_helper(u"-42"_ns, Some(-42.f));
-  test_tofloat_helper(u"+42"_ns, 42, true);
+  test_tofloat_helper(u"+42"_ns, Some(42));
-  test_tofloat_helper(u"13.37"_ns, 13.37f, true);
+  test_tofloat_helper(u"13.37"_ns, Some(13.37f));
-  test_tofloat_helper(u"1.23456789"_ns, 1.23456789f, true);
+  test_tofloat_helper(u"1.23456789"_ns, Some(1.23456789f));
-  test_tofloat_helper(u"1.98765432123456"_ns, 1.98765432123456f, true);
+  test_tofloat_helper(u"1.98765432123456"_ns, Some(1.98765432123456f));
-  test_tofloat_helper(u"0"_ns, 0.f, true);
+  test_tofloat_helper(u"0"_ns, Some(0.f));
-  test_tofloat_helper(u"1.e5"_ns, 100000, true);
+  test_tofloat_helper(u"1.e5"_ns, Some(100000));
-  test_tofloat_helper(u""_ns, 0.f, false);
+  test_tofloat_helper(u""_ns, Nothing());
-  test_tofloat_helper(u"42foo"_ns, 42.f, false);
+  test_tofloat_helper(u"42foo"_ns, Nothing());
-  test_tofloat_helper(u"foo"_ns, 0.f, false);
+  test_tofloat_helper(u"foo"_ns, Nothing());
-  test_tofloat_helper(u"1.5e-"_ns, 1.5f, false);
+  test_tofloat_helper(u"1.5e-"_ns, Nothing());
+
+  // Leading spaces are ignored
+  test_tofloat_helper(u"  \t5"_ns, Some(5.f));
+
+  // Values which are too large generate an error
+  test_tofloat_helper(u"3.402823e38"_ns, Some(3.402823e+38));
+  test_tofloat_helper(u"1e39"_ns, Nothing());
+  test_tofloat_helper(u"-3.402823e38"_ns, Some(-3.402823e+38));
+  test_tofloat_helper(u"-1e39"_ns, Nothing());
+
+  // Values which are too small round to zero
+  test_tofloat_helper(u"1.4013e-45"_ns, Some(1.4013e-45f));
+  test_tofloat_helper(u"1e-46"_ns, Some(0.f));
+  test_tofloat_helper(u"-1.4013e-45"_ns, Some(-1.4013e-45f));
+  test_tofloat_helper(u"-1e-46"_ns, Some(-0.f));
 }
 
 static void test_tofloat_allow_trailing_chars_helper(const nsString& aStr,
-                                                     float aExpected,
+                                                     Maybe<float> aExpected) {
-                                                     bool aSuccess) {
   nsresult result;
-  EXPECT_EQ(aStr.ToFloatAllowTrailingChars(&result), aExpected);
+  float value = aStr.ToFloatAllowTrailingChars(&result);
-  if (aSuccess) {
+  if (aExpected) {
-    EXPECT_EQ(result, NS_OK);
+    EXPECT_TRUE(NS_SUCCEEDED(result));
+    EXPECT_EQ(value, *aExpected);
   } else {
-    EXPECT_NE(result, NS_OK);
+    EXPECT_TRUE(NS_FAILED(result));
   }
 }
 
 TEST_F(Strings, ToFloatAllowTrailingChars) {
-  test_tofloat_allow_trailing_chars_helper(u""_ns, 0.f, false);
+  test_tofloat_allow_trailing_chars_helper(u""_ns, Nothing());
-  test_tofloat_allow_trailing_chars_helper(u"foo"_ns, 0.f, false);
+  test_tofloat_allow_trailing_chars_helper(u"foo"_ns, Nothing());
-  test_tofloat_allow_trailing_chars_helper(u"42foo"_ns, 42.f, true);
+  test_tofloat_allow_trailing_chars_helper(u"42foo"_ns, Some(42.f));
-  test_tofloat_allow_trailing_chars_helper(u"42-5"_ns, 42.f, true);
+  test_tofloat_allow_trailing_chars_helper(u"42-5"_ns, Some(42.f));
-  test_tofloat_allow_trailing_chars_helper(u"13.37.8"_ns, 13.37f, true);
+  test_tofloat_allow_trailing_chars_helper(u"13.37.8"_ns, Some(13.37f));
-  test_tofloat_allow_trailing_chars_helper(u"1.5e-"_ns, 1.5f, true);
+  test_tofloat_allow_trailing_chars_helper(u"1.5e-"_ns, Some(1.5f));
 }
 
-static void test_todouble_helper(const nsString& aStr, double aExpected,
+static void test_todouble_helper(const nsString& aStr,
-                                 bool aSuccess) {
+                                 Maybe<double> aExpected) {
   nsresult result;
-  EXPECT_EQ(aStr.ToDouble(&result), aExpected);
+  double value = aStr.ToDouble(&result);
-  if (aSuccess) {
+  if (aExpected) {
-    EXPECT_EQ(result, NS_OK);
+    EXPECT_TRUE(NS_SUCCEEDED(result));
+    EXPECT_EQ(value, *aExpected);
   } else {
-    EXPECT_NE(result, NS_OK);
+    EXPECT_TRUE(NS_FAILED(result));
   }
 }
 
 TEST_F(Strings, todouble) {
-  test_todouble_helper(u"42"_ns, 42, true);
+  test_todouble_helper(u"42"_ns, Some(42));
-  test_todouble_helper(u"42.0"_ns, 42, true);
+  test_todouble_helper(u"42.0"_ns, Some(42));
-  test_todouble_helper(u"-42"_ns, -42, true);
+  test_todouble_helper(u"-42"_ns, Some(-42));
-  test_todouble_helper(u"+42"_ns, 42, true);
+  test_todouble_helper(u"+42"_ns, Some(42));
-  test_todouble_helper(u"13.37"_ns, 13.37, true);
+  test_todouble_helper(u"13.37"_ns, Some(13.37));
-  test_todouble_helper(u"1.23456789"_ns, 1.23456789, true);
+  test_todouble_helper(u"1.23456789"_ns, Some(1.23456789));
-  test_todouble_helper(u"1.98765432123456"_ns, 1.98765432123456, true);
+  test_todouble_helper(u"1.98765432123456"_ns, Some(1.98765432123456));
-  test_todouble_helper(u"123456789.98765432123456"_ns, 123456789.98765432123456,
+  test_todouble_helper(u"123456789.98765432123456"_ns,
-                       true);
+                       Some(123456789.98765432123456));
-  test_todouble_helper(u"0"_ns, 0, true);
+  test_todouble_helper(u"0"_ns, Some(0));
-  test_todouble_helper(u"1.e5"_ns, 100000, true);
+  test_todouble_helper(u"1.e5"_ns, Some(100000));
-  test_todouble_helper(u""_ns, 0, false);
+  test_todouble_helper(u""_ns, Nothing());
-  test_todouble_helper(u"42foo"_ns, 42, false);
+  test_todouble_helper(u"42foo"_ns, Nothing());
-  test_todouble_helper(u"foo"_ns, 0, false);
+  test_todouble_helper(u"foo"_ns, Nothing());
-  test_todouble_helper(u"1.5e-"_ns, 1.5, false);
+  test_todouble_helper(u"1.5e-"_ns, Nothing());
+
+  // Leading spaces are ignored
+  test_todouble_helper(u"  \t5"_ns, Some(5.));
+
+  // Values which are too large generate an error
+  test_todouble_helper(u"1.797693e+308"_ns, Some(1.797693e+308));
+  test_todouble_helper(u"1e309"_ns, Nothing());
+  test_todouble_helper(u"-1.797693e+308"_ns, Some(-1.797693e+308));
+  test_todouble_helper(u"-1e309"_ns, Nothing());
+
+  // Values which are too small round to zero
+  test_todouble_helper(u"4.940656e-324"_ns, Some(4.940656e-324));
+  test_todouble_helper(u"1e-325"_ns, Some(0.));
+  test_todouble_helper(u"-4.940656e-324"_ns, Some(-4.940656e-324));
+  test_todouble_helper(u"-1e-325"_ns, Some(-0.));
 }
 
 static void test_todouble_allow_trailing_chars_helper(const nsString& aStr,
-                                                      double aExpected,
+                                                      Maybe<double> aExpected) {
-                                                      bool aSuccess) {
   nsresult result;
-  EXPECT_EQ(aStr.ToDoubleAllowTrailingChars(&result), aExpected);
+  double value = aStr.ToDoubleAllowTrailingChars(&result);
-  if (aSuccess) {
+  if (aExpected) {
-    EXPECT_EQ(result, NS_OK);
+    EXPECT_TRUE(NS_SUCCEEDED(result));
+    EXPECT_EQ(value, *aExpected);
   } else {
-    EXPECT_NE(result, NS_OK);
+    EXPECT_TRUE(NS_FAILED(result));
   }
 }
 
 TEST_F(Strings, ToDoubleAllowTrailingChars) {
-  test_todouble_allow_trailing_chars_helper(u""_ns, 0, false);
+  test_todouble_allow_trailing_chars_helper(u""_ns, Nothing());
-  test_todouble_allow_trailing_chars_helper(u"foo"_ns, 0, false);
+  test_todouble_allow_trailing_chars_helper(u"foo"_ns, Nothing());
-  test_todouble_allow_trailing_chars_helper(u"42foo"_ns, 42, true);
+  test_todouble_allow_trailing_chars_helper(u"42foo"_ns, Some(42));
-  test_todouble_allow_trailing_chars_helper(u"42-5"_ns, 42, true);
+  test_todouble_allow_trailing_chars_helper(u"42-5"_ns, Some(42));
-  test_todouble_allow_trailing_chars_helper(u"13.37.8"_ns, 13.37, true);
+  test_todouble_allow_trailing_chars_helper(u"13.37.8"_ns, Some(13.37));
-  test_todouble_allow_trailing_chars_helper(u"1.5e-"_ns, 1.5, true);
+  test_todouble_allow_trailing_chars_helper(u"1.5e-"_ns, Some(1.5));
 }
 
 TEST_F(Strings, Split) {