Fix: Date.parse() now works the same as the browser by KaustAbhinand · Pull Request #2497 · mozilla/rhino

KaustAbhinand · 2026-09-19T18:05:03Z

Hello @rbri! This is the PR for the issue #2411

What I changed

Nothing much really. Only the file NativeDate.java had to be changed.
The method changed was: date_parseString - I added more if-cases when the month, day and time are out of range and made them return NaN, instead of them rolling over to the next month or day like they used to.

Tests

I have added new methods in NativeDateTest.java. All the test cases pass.

Conclusion

I think its fixed. Please review the changes. Thanks!

KaustAbhinand · 2026-09-19T18:06:20Z

@rbri Please review the changes, I think its fixed. I manually tested it as well. NaN is returned for invalid inputs

KaustAbhinand · 2026-09-19T19:55:16Z

2 CI jobs have failed, but I'm unable to isolate the cause though. Particularly the Java 21 build and test. Can't really tell whats happening. I've tried to re-trigger the CI to see if they fail again. I got these errors:

Java 21 build & test:
FAILURE: Build failed with an exception.

[Incubating] Problems report is available at: file:///home/runner/work/rhino/rhino/build/reports/problems/problems-report.html

What went wrong:
Execution failed for task ':rhino:test'.

There were failing tests. See the report at: file:///home/runner/work/rhino/rhino/rhino/build/reports/tests/test/index.html

Try:
Deprecated Gradle features were used in this build, making it incompatible with Gradle 10.

Run with --scan to get full insights from a Build Scan (powered by Develocity).

BUILD FAILED in 25m 56s
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/9.4.0/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.
124 actionable tasks: 118 executed, 6 from cache
Configuration cache entry stored.
Error: Process completed with exit code 1.

Rhino Test:

FAILURE: Build failed with an exception.

[Incubating] Problems report is available at: file:///home/runner/work/rhino/rhino/build/reports/problems/problems-report.html

What went wrong:

Execution failed for task ':rhino-kotlin:decycleMain'.
Deprecated Gradle features were used in this build, making it incompatible with Gradle 10.

A failure occurred while executing de.obqo.decycle.gradle.DecycleWorker

Stream closed
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

Try:
For more on this, please refer to https://docs.gradle.org/9.4.0/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

Run with --stacktrace option to get the stack trace.
109 actionable tasks: 102 executed, 7 from cache
Run with --info or --debug option to get more log output.
Run with --scan to get full insights from a Build Scan (powered by Develocity).
Get more help at https://help.gradle.org./

BUILD FAILED in 2m 41s
Configuration cache entry stored.
Error: Process completed with exit code 1.

rbri · 2026-09-19T20:03:55Z

Will have a look

KaustAbhinand · 2026-09-19T20:35:21Z

Well the CI is green now, suppose the re-run did it

rbri · 2026-09-20T07:20:34Z

            }
        }
        if (year < 0 || mon < 0 || mday < 0) return ScriptRuntime.NaN;
+        // Reject out of range fields (output NaN instead of rolling over).


maybe we can improve this comment a bit, the first part is only true for the line before
the second part is only of some meaning for people knowing that we did the rollover before, all others are confused

Should I delete the comment entirely?

rbri · 2026-09-20T07:21:50Z

+        if (hour > 24 || min > 59 || sec > 59) return ScriptRuntime.NaN;
+        if (hour == 24 && (min > 0 || sec > 0)) return ScriptRuntime.NaN;
+
        if (sec < 0) sec = 0;


is this still correct? we have the < 0 check for year/mon/mday above, what do the browsers do?

Well Date.parse('20/15/26') did give an NaN, I checked it in the browser as well.

rbri · 2026-09-20T07:22:52Z

        if (year < 0 || mon < 0 || mday < 0) return ScriptRuntime.NaN;
+        // Reject out of range fields (output NaN instead of rolling over).
+        if (mon > 11) return ScriptRuntime.NaN;
+        if (mday < 1 || mday > DaysInMonth(year, mon + 1)) return ScriptRuntime.NaN;


at least mday is now checked twice, maybe we can make the code a bit more logical by checking all conditions field by field

I didn't quite understand the field-by-field part?

Maybe at first one if with all the checks for year, then one with all chechs for month and so on...

KaustAbhinand · 2026-09-23T15:02:09Z

@rbri Hi!

So sorry for the delays

I have changed the if-conditions to make them field-by-field - (month, hour, year, minutes, seconds)

Hopefully its fine now! Please review it 😄

(By mistake I put the PR number as the issue number in the commit message)

KaustAbhinand · 2026-09-23T15:08:30Z

Another note though, I discovered another issue (was not sure if it was in the scope for this PR, so I didn't implement it).

For inputs like Date.parse('02/12/26') - Rhino gives -1384925400000 and the browser gives 1770834600000
Similarly for Date.parse('01/01/49') - Rhino gives -662707800000 and the browser gives 2493052200000

For 01/01/50 and 12/31/99 though, the browser and Rhino give the same output.

But here, the NaN checks work fine though, so I committed it. Thought I would report this behaviour before I do anything.

Should I create a new issue for this?

rbri · 2026-09-23T15:34:12Z

@KaustAbhinand please make a new issue

will review this on sunday, sorry

rbri · 2026-09-23T15:37:37Z

+        // Parsing a time that is out of range - Needs to give an NaN.
+        ctorDateTimeString("NaN", "String(Date.parse('01/01/2026 25:00'))");
+        ctorDateTimeString("NaN", "String(Date.parse('01/01/2026 10:75'))");
+        ctorDateTimeString("NaN", "String(Date.parse('01/01/2026 10:30:75'))");


mybe you can add tests for the border cases here

min 59/60

sec 59/60 and

also for the several hour checks ((hour > 24 || (hour == 24 && (min > 0 || sec > 0))

gbrail · 2026-09-23T15:48:31Z

And I hate to ask but can you please do research on whether we need to handle leap seconds. How does Java handle it?

KaustAbhinand · 2026-09-24T13:58:56Z

@rbri i have updated the test file as well.
Please review

KaustAbhinand · 2026-09-24T13:59:32Z

And I hate to ask but can you please do research on whether we need to handle leap seconds. How does Java handle it?

Sure @gbrail, I'll do a quick read up on it.

KaustAbhinand · 2026-09-24T14:25:23Z

Well I've done some done some poking around with Claude, and read Oracles's docs on how a leap second is handled. Here is what I've gathered.

Java's date class only reflects UTC, it is intended to reflect the coordinated universal times, but it depends on the host environment of the JVM. But the OS is the real limitation - nearly all of the modern OS' assume a day as 86,400 seconds, whereas the UTC inserts a second as the last second of Dec 31 or June 30. But the doc also says that the computer clocks aren't accurate enough to reflect this distinction.

I guess it ultimately means that Java ignores this altogether.

Whether Rhino needs to implement it?

Well ECMAScript's Date object is designed to ignore leap seconds. Time is measured in milliseconds since the epoch using a simplified model where every day has exactly 86,400,000 ms — leap seconds don't exist in this model at all.

(This I have quoted from Claude)

And also, the parseISOString method already has a check - if sec > 59 - defined as NaN. So anyway, the default handling is to ignore it.

https://docs.oracle.com/javase/8/docs/api/java/util/Date.html - this was the Docs I had looked up.

@gbrail, I hope it answers your question! :)

Fix: Date.parse() now works the same as the browser

3f5c206

Retrigger CI

4f48ed9

rbri reviewed Sep 20, 2026

View reviewed changes

Fix: Added checks for each field (issue mozilla#2497)

ad28fba

rbri reviewed Sep 23, 2026

View reviewed changes

Fix: Updated the test file, covering field-by-fied tests

3412d80

Conversation

KaustAbhinand commented Sep 19, 2026

What I changed

Tests

Conclusion

Uh oh!

KaustAbhinand commented Sep 19, 2026

Uh oh!

KaustAbhinand commented Sep 19, 2026

Uh oh!

rbri commented Sep 19, 2026

Uh oh!

KaustAbhinand commented Sep 19, 2026

Uh oh!

rbri Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

KaustAbhinand Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

rbri Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KaustAbhinand Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

rbri Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

KaustAbhinand Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

rbri Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

KaustAbhinand commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KaustAbhinand commented Sep 23, 2026

Uh oh!

rbri commented Sep 23, 2026

Uh oh!

rbri Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

gbrail commented Sep 23, 2026 via email • edited by rbri Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KaustAbhinand commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KaustAbhinand commented Sep 24, 2026

Uh oh!

KaustAbhinand commented Sep 24, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rbri Sep 20, 2026 •

edited

Loading

KaustAbhinand commented Sep 23, 2026 •

edited

Loading

gbrail commented Sep 23, 2026 via email •

edited by rbri

Loading

KaustAbhinand commented Sep 24, 2026 •

edited

Loading