[JSRangeErrorException - "Invalid timezone name!"] Fix Intl.DateTimeFormat bug with normalizing timeZone value #571

anton-patrushev · 2021-08-13T09:44:04Z

Info

I have tried to use hermes-engine v0.8.0 with react-native v0.65-rc4. The main reason was to test Intl API implementation since my RN project uses Hermes engine (instead of JSC) for executing JS code on Android. I have also using date-fns & date-fns-tz libraries. The second one relies on Intl API. That's the reason why I've tried to use new hermes-engine with Intl API support.
But when I started to testing it I had realized what something was going wrong. I had noticed what Intl.DateTimeFormat is broken. Below is an example which was broken by Intl.DateTimeFormat implementation.

That is piece of code which is formats date and applies provided timeZone.

import { format } from "date-fns"
import { utcToZonedTime } from "date-fns-tz"

function formatTimezoneDate(date, timezone) {
  return format(utcToZonedTime(date, timezone), 'MMM do yyyy, h:mm:ss a');
}

const date = new Date("2021-08-03T22:04:46Z");
const timeZone = "America/New_York";

const formattedDate = formatTimezoneDate(date, timeZone);

// actualResult
console.log(formattedDate) // "Aug 3rd 2021, 10:04:46 pm"

// expected Result
// "Aug 3rd 2021, 6:04:46 pm"

utcToZonedTime converts provided date into appropriate timeZone and returns newDate.

utcToZonedTime under the hoods uses tzParsetimeZone function which accepts string timeZone value and returns offsetInMilliseconds number value.
This function call isValidTimezoneIANAString. Here isValidTimezoneIANAString function implementation.

function isValidTimezoneIANAString(timeZoneString) {
  try {
    Intl.DateTimeFormat(undefined, {timeZone: timeZoneString});
    return true;
  } catch (error) {
    return false;
  }
}

I have added console.log(error) to catch statement and have seen what Intl.DateTimeFormat throws the next error:

JSRangeErrorException("Invalid timezone name!")
```js

That is why utcToZonedTime function always converts date into UTC timezone (tzParseTimeZone always returns 0. value)
I have found only one place in Hermes source code where it can throw JSRangeErrorException("Invalid timezone name!").

    Object timeZone = JSObjects.Get(options, "timeZone");
    if (JSObjects.isUndefined(timeZone)) {
      timeZone = DefaultTimeZone();
    } else {
      String normalizedTimeZone = normalizeTimeZoneName(JSObjects.getJavaString(timeZone));
      if (!isValidTimeZoneName(normalizedTimeZone)) {
        throw new JSRangeErrorException("Invalid timezone name!");
      }
    }
    mTimeZone = timeZone;

Here is isValidTimeZoneName method implementation under the hood. It is simple as possible. Everything should be ok.

import java.util.TimeZone;

// ...

  @Override
  public boolean isValidTimeZone(String timeZone) {
    return TimeZone.getTimeZone(timeZone).getID().equals(timeZone);
  }

But isValidTimeZone method receive already messed up timeZone string value. In my case instead of receiving America/New_York value it accepts AMERICA/NEW_YORK. This happens because of normalizeTimeZoneName method. This method transforms timeZone string into uppercase version.
That's why TimeZone.getTimeZone("AMERICA/NEW_YORK").getID().equals("AMERICA/NEW_YORK") will always returns false. (TimeZone.getTimeZone("AMERICA/NEW_YORK").getID() always returns "GMT: string)

Brief Changes

fix normalizeTimeZoneName method to fit into JDK 1.8 java.util.TimeZone specs.
As far as I understand there are some differences between 1.7 & 1.8 java.util.TimeZone implementation. JDK 1.7 supply case insensitive TimeZone module, while JDK 1.8 provides case sensitive module for managing TimeZones. That's why calling the next piece of code will have different behaviors.

// on JDK 1.7 it will be "America/New_York", but 1.8 version makes a fallback to "GMT" id.
TimeZone.getTimeZone("AMERICA/NEW_YORK").getID();

I have fixed normalizeTimeZoneName function to follow ECMAScripts specs and make compatibility with JDK 1.8 and higher versions.

String id1 = normalizeTimeZoneName("America/New_York");
String id2 = normalizeTimeZoneName("AMERICA/NEW_YORK");
String id3 = normalizeTimeZoneName("america/new_york");
String id4 = normalizeTimeZoneName("UtC");
String id5 = normalizeTimeZoneName("gMt+07:00");

System.out.println(id1); // "America/New_York"
System.out.println(id2); // "America/New_York"
System.out.println(id3); // "America/New_York"
System.out.println(id4); // "UTC"
System.out.println(id5); // "GMT+07:00"
        
System.out.println(isValidTimeZoneName(id1)); // true
System.out.println(isValidTimeZoneName(id2)); // true
System.out.println(isValidTimeZoneName(id3)); // true
System.out.println(isValidTimeZoneName(id4)); // true
System.out.println(isValidTimeZoneName(id5)); // true

That's how it behaves now. Tested on my End. Sorry for not providing unit tests for it. I wasn't able to find out how your test system is designed.

facebook-github-bot · 2021-08-13T09:44:08Z

Hi @anton-patrushev!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2021-08-13T10:06:06Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot · 2021-08-13T19:01:38Z

@mhorowitz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mhorowitz · 2021-08-13T19:07:26Z

Thank you for the detailed bug report and the fix!

CircleCI seems to be having issues on Android, but our internal CI system should run tests on this. If you want to run t hem yourself, take a look at https://github.com/facebook/hermes/blob/main/android/intltest/build.gradle which is where the tests are run from. There are instructions at the top. The actual test cases come from https://github.com/tc39/test262

facebook-github-bot · 2021-08-25T16:07:24Z

@anton-patrushev has updated the pull request. You must reimport the pull request before landing.

anton-patrushev · 2021-08-25T16:07:52Z

@mhorowitz So what are next steps you propose?
I have tried to run Intl tests for Android. I think I wasn't successful since almost all of them was failed due to some unknown reason. I have implemented one test, which will ensure what Intl.DateTimeFormat API works correctly, but I wasn't able to check, is this test works correctly or not. I have committed that test. But anyway, can we merge this PR (maybe by applying some requested changes or whatever)? Or maybe you could run this test on your internal CI?
It will be great if someone could run this test using this fixed version of Hermes (my fork) and version of Hermes from the main repo and compare the result. It will indicates that my fix is really working (actually for me it works in the real app).

facebook-github-bot · 2021-08-25T16:08:28Z

@anton-patrushev has updated the pull request. You must reimport the pull request before landing.

anton-patrushev · 2021-09-09T09:30:29Z

@mhorowitz Any updates?

facebook-github-bot · 2021-09-10T20:15:36Z

@mhorowitz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mhorowitz · 2021-09-10T23:37:07Z

Unfortunately, the regression tests didn't behave as I expected internally, either. I got them running, and the regression tests pass. The test you added in 23616f7 did not, though. I had to tweak it a bit, now I have this:

  rt.evaluateJavaScript(
      new StringBuilder()
          .append("var date = new Date('2021-08-13T14:00:00Z');\n")
          .append("var formattedDate = Intl.DateTimeFormat('en-US', {\n")
          .append("timeZone: 'America/New_York',\n")
          .append("month: 'numeric',\n")
          .append("day: 'numeric',\n")
          .append("hour: 'numeric',\n")
          .append("minute: 'numeric'\n")
          .append("}).format(date);\n")
          .toString());

This passes. However, I realized this doesn't (I think) test the underlying fundamental problem of case folding the time zone. If I modify the test to mangle the case:

          .append("timeZone: 'AMERICA/new_YORK',\n")

The output is GMT, not US/Eastern:

com.facebook.hermes.test.HermesInstrumentationTest > testDateTimeFormat[Pixel_3_API_29(AVD) - 10] FAILED
        org.junit.ComparisonFailure: expected:<'8/13, [10:00 A]M'> but was:<'8/13, [2:00 P]M'>

It's possible I'm misunderstanding the change you've made, but I expected the unusual casing would succeed based on the description in the PR. What am I missing?

anton-patrushev · 2021-09-22T16:19:43Z

@mhorowitz Did you run these tests on my branch (with modified Java code) or on other branch, which has the same Java code as the main branch (before my fix)?
This important since I believe AMERICA/new_YORK should work with my fix too. (Because I have implemented function, which is reliable to transform this random case strings into Java TZ compatible format to get the right TimeZone ID).

I will be back with answer (I have an empty project with manually patched hermes-engine executable, which I made locally (thanks to your docs 🤗), to make some kind of manual E2E QA test).
I'm sure it works with America/New_York, but don't remember have I tested it with random case or not 🤔.

mhorowitz · 2021-09-22T18:05:58Z

@anton-patrushev I did my testing on 692ef46 which appears to to be the top revision in your PR. Is there some other revision I should be testing?

anton-patrushev · 2021-09-22T18:38:31Z

@mhorowitz Could I ask you to run this test case (which you mentioned and tweaked) on the main hermes branch to ensure it failed. It should failed since Android Intl API doesn't work for me from the main branch.
In that case we can assume my fix works indeed, otherwise I should think about other issue root-case.
I will be back with my manual tests results.
Thanks in advance!

facebook-github-bot · 2021-09-22T21:02:27Z

@anton-patrushev has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-22T21:03:32Z

@anton-patrushev has updated the pull request. You must reimport the pull request before landing.

anton-patrushev · 2021-09-22T21:52:54Z

@mhorowitz FYI
I have created repo with Intl API usage example.
And I have provided some screenshots.
This one uses the build from current branch with my fix.

So you can compare it with the stable hermes-engine package. It doesn't work correctly for this example.

But my fix still doesn't support case insensitivity :(
I will try to fix it and will be back with updates.

mhorowitz · 2021-09-22T21:56:19Z

@anton-patrushev Thanks for the screen shots! This is consistent with what I see. Your changes prevent Intl from throwing an exception, but do not fix the issues with timezone case sensitivity. I'll hold off merging this for now while you investigate the fix for that.

andreialecu · 2021-09-23T13:44:24Z

We're also running into this problem which unfortunately makes Hermes Intl unusable for us.

@anton-patrushev unless I'm reading it wrong, I believe the remaining problem is here:

https://github.com/anton-patrushev/hermes/blob/c31fe892e1a40e37276ad7c29142eecb4168586f/lib/Platform/Intl/java/com/facebook/hermes/intl/DateTimeFormat.java#L246-L252

mTimeZone should be set to the result of the normalization, otherwise it will remain mangled.

anton-patrushev · 2021-09-23T14:08:15Z

We're also running into this problem which unfortunately makes Hermes Intl unusable for us.

@anton-patrushev unless I'm reading it wrong, I believe the remaining problem is here:

https://github.com/anton-patrushev/hermes/blob/c31fe892e1a40e37276ad7c29142eecb4168586f/lib/Platform/Intl/java/com/facebook/hermes/intl/DateTimeFormat.java#L246-L252

mTimeZone should be set to the result of the normalization, otherwise it will remain mangled.

Thanks for highlighting it @andreialecu!
I will try to investigate it anytime soon. Seems like your assumption is right!
I will be back with answer!

But just to be on the same page.
@mhorowitz My fix not only prevent Intl from crashing with random case timezone name, but it make Intl works correctly with already normalized timezone name (e.g. America/New_York). Since as for me, it throws an error too, even when I use America/New_York in the ideal case 🤷‍♂️.

I will be back with final fix to support case insensitivity too!

andreialecu · 2021-09-23T14:14:03Z

I'd like to point out something regarding the implementation in this PR. There are a bunch of timezones which may not pass through the capitalization function properly. Take a look at: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones

Examples: Etc/GMT+1, US/Alaska or Africa/Dar_es_Salaam

However, I think it's possible to normalize the timezone in a different way. java.util.TimeZone.getAvailableIDs() does return an array with available time zone names. So if the same upper-casing function was applied to the items in that array and then compared to the input, there should be a match.

The original casing of the match would then be the normalized time zone name.

Hopefully this makes sense. :)

facebook-github-bot · 2021-09-24T19:31:47Z

@anton-patrushev has updated the pull request. You must reimport the pull request before landing.

anton-patrushev · 2021-09-24T19:37:08Z

Finally, I'm back :)
So, as @andreialecu proposed, I have reimplemented the whole approach for normalizing and validating timeZone, which comes from the JS side into Android Java internals.

If you would have any questions regarding my approach do not hesitate to ask me.
You can check my repo with the Hermes Intl usage example.
I have updated screenshots. This one with my final fix on Android.

@mhorowitz Please take a look.

facebook-github-bot · 2021-09-24T19:37:16Z

@anton-patrushev has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2021-09-24T19:43:48Z

@anton-patrushev has updated the pull request. You must reimport the pull request before landing.

anton-patrushev · 2021-09-25T15:42:42Z

@mhorowitz
Is there any chance it would be merged?

anton-patrushev · 2021-09-27T08:14:24Z

@mhorowitz any updates?

mhorowitz · 2021-09-27T20:27:18Z

I hope to look at it this week. For the future, I don't tend to check GitHub daily, and almost never on weekends, so pinging me on Saturday morning isn't likely to get a reply until at least Monday.

anton-patrushev · 2021-10-02T17:24:34Z

Can someone provide any info on this?
I found it quite critical for Hermes usage in the Android RN apps.
Sorry for tagging @mhorowitz, but you are the only person who was assigned to this PR (and related issue) from the Facebook team.

Can I do something on my end to make it merged?

andreialecu · 2021-10-06T14:01:55Z

Hey @Huxpro, hope you don't mind the ping. This issue is preventing Intl from working in most (I think) real-life apps on Android.

Mind taking a look at the PR, please?

facebook-github-bot · 2021-10-07T01:16:06Z

@mhorowitz has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mhorowitz

https://github.com/tc39/test262/blob/main/test/intl402/DateTimeFormat/timezone-invalid.js is failing, I suspect due to applying additional locale comparisons as noted in code comments below.

I found this by running the test suite on device. The instructions are a bit scattered in the source tree, so here's what you need.

edit hermes/android/build.gradle to include
"${hermesHostBuild}/build_host_hermesc/ImportHermesc.cmake",
On line 31 or so. it should be pretty clear where it goes.
Run an android emulator, or connect a device
Run these commands

export HERMES_WS_DIR=.../parent-of-hermes-checkout
cd "$HERMES_WS_DIR"
hermes/utils/build/configure.py ./build_host_hermesc
cmake --build ./build_host_hermesc --target hermesc
cd hermes/android
./gradlew intl:prepareTest262
./gradlew intl:connectedAndroidTest

The complete tests take a couple minutes to run on my emulator. On a real device they may take longer. logcat will print output fairly continuously while it runs.

You may trip over a bug which causes a crash result like
Test failed to run to completion. Reason: 'Instrumentation run failed due to 'Process crashed.''. Check device logcat for details
If so, you can apply https://gist.github.com/mhorowitz/f314397b97f0419c53011dded9c89b89 to comment out the problematic tests.

At that point, when you rerun the tests (the final command above) you should see output like
> There were failing tests. See the report at: file:///.../build_android/intltest/reports/androidTests/connected/flavors/debugAndroidTest/index.html
Open that file in your browser, and it will show you failures. Probably, you will see HermesIntlDateFormatTest has failed. Unfortunately, the specific failure does not appear here. You can grep for it in logcat:

$ adb logcat -d | grep 'Failed Tests:'
10-06 18:50:10.244  6896  6917 V HermesIntlNumberFormatTest: Failed Tests: test262/test/intl402/DateTimeFormat/timezone-invalid.js : Invalid time zone name Europe/İstanbul was not rejected. Expected a RangeError to be thrown but no exception was thrown at all

mhorowitz · 2021-10-07T02:21:20Z

lib/Platform/Intl/java/com/facebook/hermes/intl/PlatformDateTimeFormatterAndroid.java

+      .filter(new Predicate<String>() {
+        @Override
+        public boolean test(String tz) {
+          return tz.equalsIgnoreCase(timeZone);


I believe this is incorrect. https://tc39.es/ecma402/#sec-case-sensitivity-and-case-mapping only allows mapping/folding ascii a-z <-> A-Z. equalsIgnoreCase is probably applying additional locale-specific comparisons.

To be honest I don't find it critical. It's strange requirement. But ok, that's how it is.
Any way, I believe my changes will allow map A-Z to a-z and vice versa and it shouldn't be critical issue. I'm finding not working America/New_York (e.g. timezones from IANA TZ db in its original look) more important & crucial.

So I will prefer to leave it as it's right now (since it works, but with such small issues). I guess it will good enough to add test suites which will cover this case sensitivity issue in the future. And fix it.

I will try to do my best and fix it according to ECMA standards and back with result.

You just need to bring this back:

https://github.com/facebook/hermes/pull/571/files#diff-bd6622e75c91b32a4ac2f2a7f9ad2dc1e6c9ffa72c71e78070f3fc57c8d4f4e3L382-L397

Instead of using the built-in case insensitive comparison methods.

mhorowitz · 2021-10-07T02:21:29Z

lib/Platform/Intl/java/com/facebook/hermes/intl/PlatformDateTimeFormatterICU.java

+      .filter(new Predicate<String>() {
+        @Override
+        public boolean test(String tz) {
+          return tz.equalsIgnoreCase(timeZone);


mhorowitz · 2021-10-07T02:42:33Z

lib/Platform/Intl/java/com/facebook/hermes/intl/DateTimeFormat.java

+      } catch (JSRangeErrorException error) {
+        throw error;


you can just remove this try/catch, it doesn't do anything.

anton-patrushev · 2021-10-14T16:46:24Z

@mhorowitz Thanks for your review!
I will dig into this and review all required & proposed changes on this weekend. Will be back with answers.
And if someone have enough time and want to help - I will appreciate any kind of your support!

Summary:  Please feel free to edit anything as necessary. Picking up where #571 left off Error: ![intl_error](https://user-images.githubusercontent.com/30021449/139603461-2ae20e1d-7ead-4fe2-ab9a-da5f647b3d19.png) Cause: #627 (comment) _Coauthored with anton-patrushev_ 🎉 _A special thanks to hcwesson_ 🙏  Pull Request resolved: #627 Test Plan:  Followed testing instructions provided by mhorowitz #571 (review) ![image](https://user-images.githubusercontent.com/30021449/139600222-316bb2e1-f718-4d15-b02e-281374a26eac.png) Reviewed By: kodafb Differential Revision: D32240632 Pulled By: neildhar fbshipit-source-id: d9582d5908b22addb31516834a58649182da5c64

anton-patrushev · 2021-11-12T15:36:26Z

Closed that, since #627 had been merged into main.
Thanks to @calebmackdavenport !!!

anton-patrushev force-pushed the fix-intl-date-time-format branch from 2cb8965 to c42e5b8 Compare August 13, 2021 09:51

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Aug 13, 2021

TheLonelyAstronaut approved these changes Aug 13, 2021

View reviewed changes

anton-patrushev marked this pull request as draft August 13, 2021 13:35

Fix normalizing timeZone function

7991f8c

anton-patrushev force-pushed the fix-intl-date-time-format branch from 0d37e60 to 7991f8c Compare August 13, 2021 17:53

anton-patrushev marked this pull request as ready for review August 13, 2021 18:06

Merge branch 'main' into fix-intl-date-time-format

f5ace46

anton-patrushev mentioned this pull request Aug 13, 2021

Intl.DateTimeFormat throws and RangeError "Invalid timezone name!" when valid IANA timezone is provided #572

Closed

1 task

Add test for Int.DateTimeFormat API

23616f7

Merge branch 'main' into fix-intl-date-time-format

692ef46

Tweak test case

0846d3b

Merge branch 'main' into fix-intl-date-time-format

c31fe89

anton-patrushev added 2 commits September 24, 2021 13:25

re-implement timezone normalizing algorithm

b68946c

fix new algorithm issues

bfb4de8

Merge branch 'main' into fix-intl-date-time-format

b483991

add test case to ensure case insensetivity works correctly

39953b3

mhorowitz requested changes Oct 7, 2021

View reviewed changes

mhorowitz reviewed Oct 7, 2021

View reviewed changes

calebmackdavenport mentioned this pull request Oct 31, 2021

Fix Intl.DateTimeFormat TimeZone #627

Closed

anton-patrushev closed this Nov 12, 2021

[JSRangeErrorException - "Invalid timezone name!"] Fix Intl.DateTimeFormat bug with normalizing timeZone value #571

[JSRangeErrorException - "Invalid timezone name!"] Fix Intl.DateTimeFormat bug with normalizing timeZone value #571

Conversation

anton-patrushev commented Aug 13, 2021 • edited Loading

Info

Brief Changes

facebook-github-bot commented Aug 13, 2021

Action Required

Process

facebook-github-bot commented Aug 13, 2021

facebook-github-bot commented Aug 13, 2021

mhorowitz commented Aug 13, 2021

facebook-github-bot commented Aug 25, 2021

anton-patrushev commented Aug 25, 2021

facebook-github-bot commented Aug 25, 2021

anton-patrushev commented Sep 9, 2021

facebook-github-bot commented Sep 10, 2021

mhorowitz commented Sep 10, 2021 • edited Loading

anton-patrushev commented Sep 22, 2021

mhorowitz commented Sep 22, 2021

anton-patrushev commented Sep 22, 2021

facebook-github-bot commented Sep 22, 2021

facebook-github-bot commented Sep 22, 2021

anton-patrushev commented Sep 22, 2021

mhorowitz commented Sep 22, 2021 • edited Loading

andreialecu commented Sep 23, 2021

anton-patrushev commented Sep 23, 2021

andreialecu commented Sep 23, 2021 • edited Loading

facebook-github-bot commented Sep 24, 2021

anton-patrushev commented Sep 24, 2021

facebook-github-bot commented Sep 24, 2021

facebook-github-bot commented Sep 24, 2021

anton-patrushev commented Sep 25, 2021

anton-patrushev commented Sep 27, 2021

mhorowitz commented Sep 27, 2021

anton-patrushev commented Oct 2, 2021

andreialecu commented Oct 6, 2021

facebook-github-bot commented Oct 7, 2021

mhorowitz left a comment • edited Loading

Choose a reason for hiding this comment

mhorowitz Oct 7, 2021

Choose a reason for hiding this comment

anton-patrushev Oct 14, 2021

Choose a reason for hiding this comment

andreialecu Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

mhorowitz Oct 7, 2021

Choose a reason for hiding this comment

mhorowitz Oct 7, 2021

Choose a reason for hiding this comment

anton-patrushev commented Oct 14, 2021

anton-patrushev commented Nov 12, 2021

anton-patrushev commented Aug 13, 2021 •

edited

Loading

mhorowitz commented Sep 10, 2021 •

edited

Loading

mhorowitz commented Sep 22, 2021 •

edited

Loading

andreialecu commented Sep 23, 2021 •

edited

Loading

mhorowitz left a comment •

edited

Loading

andreialecu Oct 14, 2021 •

edited

Loading