Pick newer equivalent years when the year >= 2038

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

{
    {2034, 2035, 2030, 2031, 2037, 2027, 2033},
    {2012, 2024, 2036, 2020, 2032, 2016, 2028}
};

are probably better choices for equivalent years after 2037 than:

{
    {1978, 1973, 1974, 1975, 1981, 1971, 1977},
    {1984, 1996, 1980, 1992, 1976, 1988, 1972}
};
Posted patch bug1406993.patch (obsolete) — Splinter Review
It seems to more useful to use current resp. near future years for DST computation of years after 2037 than past years. And it also fixes another issue where the time zone offset and time zone ID in Date.prototype.toString() don't match:

Before the patch: |new Date(2038, 11-1, 1).toString()| returns "Mon Nov 01 2038 00:00:00 GMT-0800 (PDT)".

With the patch: |new Date(2038, 11-1, 1).toString()| returns "Mon Nov 01 2038 00:00:00 GMT-0700 (PDT)".
Attachment #8916769 - Flags: review?(jwalden+bmo)
Linux(arm) doesn't get this right [1]. I guess I'll just change ToPRMJTime to call EquivalentYearForDST, too.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc6a24ec10173c5f262c52d57fecaf874013911d
Attachment #8916769 - Flags: review?(jwalden+bmo) → review+
Posted patch bug1406993.patch (obsolete) — Splinter Review
Requesting a new review, because too much changed compared to the first version.

The original patch failed on Linux(arm) because "Mar 31 2040 00:00:00" cannot be represented as a 32-bit time_t value. mktime returned -1 to notify the input can't be processed properly, but since we didn't perform any error handling at all for mktime (and localtime_r) [1], we got bogus results. 

The new patch checks the return values for mktime and localtime_r and tries to workaround 32-bit time_t limitations if possible:
- If the initial mktime failed, we retry it with an equivalent year.
- If it still fails or localtime_r failed, we simply use a fall back value for the time zone offset and set tm_zone to the empty string [2].

I've also changed the start time when we apply the equivalent year workaround to include midnight (start of day) 01/01/2038. I found it confusing not to include the start of the day, well even more confusing is the value of MaxUnixTimeT in [3], which is 12/31/2037 00:00 am, but PST time, but that's another issue. :-)


[1] https://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/js/src/vm/Time.cpp#310-311
[2] tm::tm_zone isn't a "const char*" on all platforms, so we can't directly assign the empty string literal to tm_zone, but instead need a local "char[]": https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed68be17d6d94cc3c1c97a099438741e99ce4d15
[3] https://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/js/src/vm/DateTime.h#176
Attachment #8916769 - Attachment is obsolete: true
Attachment #8917013 - Flags: review?(jwalden+bmo)
Priority: -- → P3
Note to self: Remember testing bug 1029923 after the change has landed.
Comment on attachment 8917013 [details] [diff] [review]
bug1406993.patch

Review of attachment 8917013 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsdate.cpp
@@ +445,5 @@
>      int day = int(DayFromYear(year) + 4) % 7;
>      if (day < 0)
>          day += 7;
>  
> +    auto& yearStartingWith = year < 1970 ? pastYearStartingWith : futureYearStartingWith;

Tack a const on that

@@ +450,5 @@
>      return yearStartingWith[IsLeapYear(year)][day];
>  }
>  
> +// Return true if |t| is representable as a 32-bit time_t variable, that means
> +// the year is in [1970, 2038[.

We tend to use () for exclusive bounds in comments (or at least more often than this), so [1970, 2038).

@@ +454,5 @@
> +// the year is in [1970, 2038[.
> +static bool
> +IsRepresentableAsTime32(double t)
> +{
> +    return t >= 0.0 && t < 2145916800000.0;

Mild preference for |0.0 <= t && t < ...| so it looks more like a three-part inequality.

::: js/src/vm/Time.cpp
@@ +267,5 @@
>      size_t result = 0;
>  #if defined(XP_UNIX) || defined(XP_WIN)
>      struct tm a;
>      int fake_tm_year = 0;
> +    char emptyTimeZoneId[] = "";

Move this next to the one use of it, or at least as close to it as possible for the struct it gets put in?  Enough of this C hangover.  :-)

@@ +308,5 @@
>          td.tm_wday = prtm->tm_wday;
>          td.tm_year = prtm->tm_year - 1900;
>          td.tm_yday = prtm->tm_yday;
>          td.tm_isdst = prtm->tm_isdst;
>          time_t t = mktime(&td);

Add a blank line before this.

@@ +340,5 @@
>       * in that case).
>       * e.g. new Date(1873, 0).toLocaleFormat('%Y %y') => "1873 73"
>       * See bug 327869.
>       */
> +    static const int FAKE_YEAR_BASE = 9900;

Maybe s/static const/constexpr/?
Attachment #8917013 - Flags: review?(jwalden+bmo) → review+
Applied review comments, carrying r+.
Attachment #8917013 - Attachment is obsolete: true
Attachment #8924998 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45a2ee7d468e
Add a new equivalent year mapping table for years after 2037. r=Waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/45a2ee7d468e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Duplicate of this bug: 1029923
You need to log in before you can comment on or make changes to this bug.