BalanceTimeDuration: CreateTimeDurationRecord can create too large duration values

[anba]

Mentioned in review for #2727.

The final CreateTimeDurationRecord in BalanceTimeDuration can create duration values which are larger than the maximum allowed duration.

Test case:

let d = Temporal.Duration.from({
  seconds: 2**53 - 1,
  nanoseconds: 999_999_999,
});
d.round({
  largestUnit: "nanoseconds",
});

BalanceTimeDuration is called with norm = {[[TotalNanoseconds]]: 9007199254740991999999999} (the largest possible duration value). The final step in BalanceTimeDuration calls CreateTimeDurationRecord with nanoseconds = 9007199254740991999999999, which performs:

1. Assert: ! IsValidDuration(0, 0, 0, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds) is true.
2. Return the Record { [[Days]]: ℝ(𝔽(days)), [[Hours]]: ℝ(𝔽(hours)), [[Minutes]]: ℝ(𝔽(minutes)), [[Seconds]]: ℝ(𝔽(seconds)), [[Milliseconds]]: ℝ(𝔽(milliseconds)), [[Microseconds]]: ℝ(𝔽(microseconds)), [[Nanoseconds]]: ℝ(𝔽(nanoseconds)) }.

The assertion in step 1 passes, because all computations are performed using mathematical values. But ℝ(𝔽(nanoseconds)) in step 2 performs ℝ(𝔽(9007199254740991999999999)) = 9007199254740992000000000. And 9007199254740992000000000 exceeds the maximum allowed duration value.

[ptomato]

I went through the comment threads on #2727 and I guess you are referring to #2727 (comment)? Unfortunately I must have missed that reply at the time.

I agree, we'll just have to make BalanceTimeDuration fallible again, unfortunately.

[ptomato]

Oh, ugh. It's not just a question of adding "If IsValidDuration(0, 0, 0, days, ..., nanoseconds) is false, throw a RangeError exception" to BalanceTimeDuration, because as you pointed out, the duration in the test case (which is already in test262 in test/built-ins/Temporal/Duration/prototype/round/out-of-range-when-converting-from-normalized-duration.js) actually is a valid duration according to IsValidDuration.

I'm thinking of replacing step 6 in IsValidDuration with this:

1. Let normalizedSeconds be ℝ(𝔽(days)) × 86,400 + ℝ(𝔽(hours)) × 3600 + ℝ(𝔽(minutes)) × 60 + ℝ(𝔽(seconds)) + ℝ(𝔽(milliseconds)) × 10^-3 + ℝ(𝔽(microseconds)) × 10^-6 + ℝ(𝔽(nanoseconds)) × 10^-9.

That seems correct because the ℝ(𝔽(...)) values are what will be retrieved from the Duration slots if the user does any subsequent calculations with that Duration.

But you probably have thought about this a lot more than I have. Do you know of any cases where replacing the above line would actually allow some cases that would currently be disallowed? Do you have any alternative suggestions?

[anba]

I think that approach should work. The ℝ(𝔽(...)) conversions are only needed for milliseconds, microseconds, and nanoseconds, because too large values for all other time units will be handled through the abs(normalizedSeconds) ≥ 2**53 check in IsValidDuration.

[anba]

Commit 1643cc6 only changed IsValidDuration to correctly reject too large duration values, but CreateTimeDurationRecord in BalanceTimeDuration is still incorrectly marked as infallible.

[ptomato]

I'll address that in the next editorial PR, thanks.