840706 - Make nsTString_CharT::ToDouble/ToFloat output an error code if the string doesn't parse to a finite number

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Jonathan Watt [:jwatt]]

It seems that nsTString_CharT::ToDouble can return an infinity value while returning NS_OK via the out-param. (See bug 836314 comment 15.) It seems worthwhile to add a note about that to this method's documenting comment.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 713104 [details] [diff] [review]
patch

2 drive-by nits:

>+         * @return  double-precision float rep of string value. If the number
>+         *            is too large to be stored in a double the value returned

add comma after "double"

>+         *            will be +/-Infinity. Note that in this case aErrorCode
>+         *            will NOT be set to a failure code.

I think the last sentence would be clearer as:
 "Note that this will NOT cause aErrorCode to be set to a failure code"

(The patch's current language implies (to me at least) that float-overflow *suppresses* errors, whereas I think we should be saying overflow *doesn't cause* errors.)

Comment 3

[Daniel Holbert [:dholbert]]

Also: whatever changes we make here to the ToDouble documentation, we should presumably make the same changes in the ToFloat documentation.

Comment 4

[Benjamin Smedberg]

Is this infinity behavior intentional? I see the comment which says that it is currently possible, but it's not clear that is the actual correct or desired behavior. Should overflow really parse to infinity?

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I have no idea whether it was intentional.

Comment 6

[Benjamin Smedberg]

Comment on attachment 713104 [details] [diff] [review]
patch

From IRC discussion, I much prefer fixing the bug so that large numbers do not parse as infinity. I don't really know what they *should* do (largest float possible? throw an exception?).

Comment 7

[Jonathan Watt [:jwatt]]

How about we have it set the out-param to NS_ERROR_FAILURE if there is a parse error, else set it to NS_ERROR_ILLEGAL_VALUE (or something) if the resulting value is not finite, else set it to NS_OK. That way, by default, non-finite values should take callers' error code paths, but if a caller want to opt in to getting infinity values to handle that specially then it it can distinguish between parse and overflow errors.

Comment 8

[Benjamin Smedberg]

Yeah, I guess that's ok. I wonder how many clients actually check the outparam anyway...

Comment 9

[Daniel Holbert [:dholbert]]

I think comment 7 sounds reasonable, but with one caveat -- it does hide some information from callers, because they (presumably) wouldn't be able to tell from the error code whether the value is positive vs. negative infinity, and that might matter to them if they do want special behavior for infinity.

(e.g. suppose we're parsing a minimum-allowed-value.  Then, negative infinity means "all values are valid", and positive infinity means "no values are valid".  That's an important distinction)

So in practice, I'm not sure NS_ERROR_ILLEGAL_VALUE (or whatever) would carry enough information to allow clients to usefully handle it separate from NS_ERROR_FAILURE.  Not a huge deal, though.

Comment 10

[Jonathan Watt [:jwatt]]

Actually I wasn't proposing changing the behavior of returning the +/-Infinity value. Just making sure that we set the out-param to a special error code in that case so that callers that have an NS_FAILED error handling code path (based on that out-param) will take that error code path (although, as bsmedberg points out, perhaps not all callers have such a path).

Comment 11

[Daniel Holbert [:dholbert]]

(Ah, that makes sense -- thanks for the clarification.)

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch

The tests aren't actually run, but I thought I might as well add the changes to the test file anyway so that they will run if someone fixes them to work with libxul builds.

Comment 13

[Benjamin Smedberg]

Comment on attachment 741008 [details] [diff] [review]
patch

Is the return value of ToDouble defined when an error condition occurs? It looks like you'll get infinity for overflow, but 0.0 for an empty string. And what will you get for an unparseable string like "0.33foobar"? I think we should probably define the API to return NaN in those cases, and do that.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

I ran into this again while working on bug 1772006. The HTML standard for parsing floating point numbers specifies that values rounding to infinity should produce an error rather than +/-inf (https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-floating-point-number-values), which actually has an impact on our behaviour (e.g. <progress value=1e309 max=10> is complete in Firefox as value parses to +inf, and indeterminate in Chromium). I have some local patches which change our parsing over to use double-conversion::StringToDoubleConverter to be more in-line with the standard, change the error handling to return NaN in most cases, and which make +-inf an error.

While looking into this, I discovered one location which depends on the existing edge-case behaviour right now: preference handling. ToFloat is called on a string and the result is unconditionally assigned into aResult: 1, 2. Many callers of preference APIs, including preferences themselves, assume that the outparameter won't be assigned to in error cases, meaning that for invalid floating point values, whatever value was returned is observed.

Unfortunately just changing us to not clobber the outparam on error also doesn't work perfectly, as preferences also appears to depend on infinity being an OK result. Tests like this set a preference to Number.MAX_VALUE (DBL_MAX) which is outside the range of valid float values, and thus becomes positive infinity when read.

Given that we don't really control prefs and users could theoretically depend on the parsing behaviour for invalid float preferences in some way or another, I'm somewhat reluctant to change how we parse these values. Preference float parsing should likely continue to be built on PR_strtod for a bit longer.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 840706 - Part 1: Add MFBT_API annotations to other methods in StringToDoubleConverter, r=glandium

The annotations added in bug 1770158 only covered StringToDouble,
however as noted in the comments in that file, that isn't sufficient for
accurately parsing floating point numbers. This patch extends the
annotations to the single-precision floating point numbers as well and
will be used in the next part to clean up the implementation of
nsTString::ToDouble.

Depends on D148303

Comment 16

[Nika Layzell [:nika] (ni? for response)]

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

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.

Depends on D148304

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 840706 - Part 3: Restore old float-parsing behaviour for preferences, r=#preferences-reviewers

These can be set by users, and callers often don't do a great job of
handling errors, so maintaining the existing error behaviour is our
safest option here. In addition, there are some tests which set prefs to
very large values, resulting in inf being used as the returned
preference value.

In the future it may be nice to make the error handling behaviour act
more consistently with other branches (not clobbering the outparam on
error), though that was left out from this change.

Depends on D148305

Comment 18

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

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:jwatt, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 19

[Jens Stutte [:jstutte]]

:nika, IIUC the patches are yours? Do you still plan to land them?

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba5d28166805
Part 1: Add MFBT_API annotations to other methods in StringToDoubleConverter, r=glandium
https://hg.mozilla.org/integration/autoland/rev/d32ab1fc40ed
Part 2: Make ToDouble and ToFloat more spec-compliant, r=smaug,xpcom-reviewers,barret
https://hg.mozilla.org/integration/autoland/rev/00327070cdc6
Part 3: Restore old float-parsing behaviour for preferences, r=KrisWright

Comment 21

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/ba5d28166805
https://hg.mozilla.org/mozilla-central/rev/d32ab1fc40ed
https://hg.mozilla.org/mozilla-central/rev/00327070cdc6

Comment 22

[Nika Layzell [:nika] (ni? for response)]

(In reply to Jens Stutte [:jstutte] from comment #19)

:nika, IIUC the patches are yours? Do you still plan to land them?

slipped through the cracks and landed now.