1484829 - times in Slack are an hour off

Status: VERIFIED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

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

[Tracking Requested - why for this release]:

There's a regression in Nightly that's causing times shown in Slack to be an hour off for me.  I've isolated the regression range to
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3ef1c0555a2947c1cbfbd4d253bdb1e68a4a4769&tochange=3d54f2f451ca785ba616d4cba03b9efc004fd385
so I assume it's a regression from bug 1346211.

I see this across multiple Slack instances, including https://mozilla.slack.com/ .

My Slack timezone settting is set to the misnamed (since it's UTC-7 in summer) but clearly correct "(UTC-08:00) Pacific Time (US and Canada)".

All timestamps showing in slack channels and direct messages are showing an hour earlier than they should be, as though daylight saving time weren't currently in effect.

I'm running Nightly on Linux (Ubuntu 18.04), with the system-wide timezone configured as such:
 $ ls -l /etc/localtime 
lrwxrwxrwx 1 root root 27 2018-05-21 21:28:24 /etc/localtime -> /usr/share/zoneinfo/Etc/UTC
but in an X session with the user timezone set to America/Los_Angeles:
$ echo $TZ
:/home/dbaron/.timezone
$ ls -l /home/dbaron/.timezone
lrwxrwxrwx 1 dbaron dbaron 39 2018-07-11 16:40:45 /home/dbaron/.timezone -> /usr/share/zoneinfo/America/Los_Angeles

(I'm not sure if the above makes a difference.)

Comment 1

[André Bargull [:anba]]

ICU doesn't support the "TZ=:/file/path" format <https://unicode-org.atlassian.net/browse/ICU-13694> and ends up with using the raw time zone offset as the current time zone, i.e. "UTC-08:00". 

bug 1478370 will add support for "TZ=:/etc/localtime", so when that bug has landed, we could add better file path support for TZ. In the mean time the workaround is to use "export TZ=America/Los_Angeles".

Comment 2

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

The reason I don't want to use TZ=America/Los_Angeles is because then I can change my timezone (when traveling) without logging out and logging in again.

Comment 3

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 63 since it's a recent regression.

Comment 4

[André Bargull [:anba]]

Attachment: bug1484829.patch

Applies on top of bug 1478370.

Calling |realpath(...)| didn't really work out, because, at least on Ubuntu, some time zone files link to files whose name isn't a valid tzdata time zone id. Therefore I've kept the call to |readlink(...)|, but limited the maximum symlink nesting depth, so we don't need to worry about things like circular symlinks.

Comment 5

[Pascal Chevrel:pascalc]

Jeff, could you review this patch? Thanks!

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 9004984 [details] [diff] [review]
bug1484829.patch

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

::: js/src/vm/DateTime.cpp
@@ +522,5 @@
>  #if defined(XP_WIN)
>          if (IsOlsonCompatibleWindowsTimeZoneId(tz))
>              initialStatus = IcuTimeZoneStatus::NeedsUpdate;
>  #else
> +        if (tz[0] == ':')

Maybe add a comment like

"""
A TZ beginning with a colon is expected to be followed by a path -- typically an absolute path (often /etc/localtime), but alternatively a path relative to /usr/share/zoneinfo/.
"""

That said, reading this code, I am not actually sure why we aren't just setting |initialStatus| to |IcuTimeZoneStatus::NeedsUpdate| in *all* cases.  Why doesn't that just devolve into the exact same behaviors (or at least do so with only minor changes to the current code setup)?

@@ +621,5 @@
>      return false;
>  }
>  #elif ENABLE_INTL_API && defined(ICU_TZ_HAS_RECREATE_DEFAULT)
>  static icu::UnicodeString
> +ReadTimeZoneLink(const char* tz)

"""
Given a presumptive path |tz| to a zoneinfo timezone file (e.g. /etc/localtime), attempt to compute the timezone encoded by that path by repeatedly resolving symlinks until a path containing "/zoneinfo/" followed by timezone-looking components is found.  If a symlink is broken, symlink-following recurs too deeply, non-timezone-looking components are encountered, or some other error is encountered, return the empty string.

If a non-empty string is returned, it's only guaranteed to have certain syntactic validity.  It might not actually *be* a timezone name.
"""

or somesuch.

@@ +627,3 @@
>      // The resolved link name can have different paths depending on the OS.
>      // Follow ICU and only search for "zoneinfo/", see $ICU/common/putil.cpp.
> +    static constexpr char ZoneInfoPath[] = "/zoneinfo/";

Still mostly liking SlashZoneSlash here.

@@ +632,5 @@
> +    // zones are stored in files whose name doesn't match an Olson time zone
> +    // name. For example on Ubuntu, "/usr/share/zoneinfo/America/New_York" is a
> +    // symlink to "/usr/share/zoneinfo/posixrules" and "posixrules" is not an
> +    // Olson time zone name.
> +    // Four hops should be a reasonable limit for most use cases.

Hmm.  Four hops this time?  https://www.youtube.com/watch?v=wZv62ShoStY

@@ +638,5 @@
>  
> +#ifdef PATH_MAX
> +    static constexpr size_t PathMax = PATH_MAX;
> +#else
> +    static constexpr size_t PathMax = 4096;

Reads cleaner if these aren't static.

@@ +643,5 @@
> +#endif
> +    static_assert(PathMax > 0, "PathMax should be larger than zero");
> +
> +    char linkName[PathMax];
> +    const size_t linkNameLen = sizeof(linkName) - 1; // -1 to null-terminate.

May as well make this constexpr.

@@ +654,3 @@
>  
> +    char linkTarget[PathMax];
> +    const size_t linkTargetLen = sizeof(linkTarget) - 1; // -1 to null-terminate.

constexpr

@@ +657,4 @@
>  
> +    uint32_t depth = 0;
> +
> +    // Search until we found "/zoneinfo/" in the link name.

find

@@ +665,5 @@
> +            return icu::UnicodeString();
> +
> +        // Return on error or if the result was truncated.
> +        ssize_t len = readlink(linkName, linkTarget, linkTargetLen);
> +        if (len < 0 || size_t(len) >= linkTargetLen)

Do the same slen/len dance as mentioned in the other bug here.

@@ +671,5 @@
> +
> +        // Ensure linkTarget is null-terminated.
> +        linkTarget[len] = '\0';
> +
> +        // If linkTarget is not an absolute path, resolve it against linkName.

This does not fill me with fear at all.  🙈🙉🙊

@@ +672,5 @@
> +        // Ensure linkTarget is null-terminated.
> +        linkTarget[len] = '\0';
> +
> +        // If linkTarget is not an absolute path, resolve it against linkName.
> +        if (linkTarget[0] != '/') {

Kinda seems like

  // If the target is absolute, continue with that.
  if (linkTarget[0] == '/') {
    std::strcpy(linkName, linkTarget);
    continue;
  }

  // If the target is relative, it must be resolved against either the
  // directory the link was in, or against the current working directory.
  char* separator = std::strrchr(linkName, '/');

  if (!separator) {
    // If the link name is just something like "foo", resolve linkTarget
    // against the current working directory.
    std::strcpy(linkName, linkTarget);
    continue;
  }

  // Remove everything after the final slash in linkName.
  ...

or somesuch is nicer for readability by keeping things less indented.  Also lets the reader roughly forget about the already-considered alternatives along the way better, IMO.

@@ +693,5 @@
> +            std::strcpy(linkName, linkTarget);
> +        }
> +    }
> +
> +    const char* timeZone = timeZoneWithZoneInfo + std::strlen(ZoneInfoPath);

There's a constexpr for that addend.

@@ +694,5 @@
> +        }
> +    }
> +
> +    const char* timeZone = timeZoneWithZoneInfo + std::strlen(ZoneInfoPath);
> +    size_t timeZoneLen = linkName + std::strlen(linkName) - timeZone;

timeZone is a substring of linkName, so why isn't std::strlen(timeZone) clearer *and* faster?

Always nice, IMO, if variables set and then not modified across a big bunch of code can be const.  These two can be const, right?  I think?  Too lazy to double-check.

@@ +744,2 @@
>              // <https://unicode-org.atlassian.net/browse/ICU-13694>
> +            if (tz[0] == ':')

Might be worth a

static inline const char*
TZContainsPath(const char* tzVar)
{
  return tzVar[0] == ':' ? tzVar + 1 : nullptr;
}

to common up this test and |+ 1|s in a couple places.

Comment 7

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] from comment #6)
> ::: js/src/vm/DateTime.cpp
> @@ +522,5 @@
> >  #if defined(XP_WIN)
> >          if (IsOlsonCompatibleWindowsTimeZoneId(tz))
> >              initialStatus = IcuTimeZoneStatus::NeedsUpdate;
> >  #else
> > +        if (tz[0] == ':')
> 
> Maybe add a comment like
> 
> """
> A TZ beginning with a colon is expected to be followed by a path --
> typically an absolute path (often /etc/localtime), but alternatively a path
> relative to /usr/share/zoneinfo/.
> """

Hmm, I didn't implement the second part in this patch (because it seemed unlikely to occur in practice), which also means the patch has a bug because it should check |tz[0] == ':' && tz[1] == '/'| instead of just |tz[0] == ':'|. Well, unless we want to support environment settings like "TZ=:Antarctica/Troll".


> 
> That said, reading this code, I am not actually sure why we aren't just
> setting |initialStatus| to |IcuTimeZoneStatus::NeedsUpdate| in *all* cases. 
> Why doesn't that just devolve into the exact same behaviors (or at least do
> so with only minor changes to the current code setup)?
> 

I thought ICU would perform duplicate work in that case, but it seems like |initDefault()| returns early when the default time zone is already set [1].

[1] https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/intl/icu/source/i18n/timezone.cpp#530,534-538

> @@ +632,5 @@
> > +    // zones are stored in files whose name doesn't match an Olson time zone
> > +    // name. For example on Ubuntu, "/usr/share/zoneinfo/America/New_York" is a
> > +    // symlink to "/usr/share/zoneinfo/posixrules" and "posixrules" is not an
> > +    // Olson time zone name.
> > +    // Four hops should be a reasonable limit for most use cases.
> 
> Hmm.  Four hops this time?  https://www.youtube.com/watch?v=wZv62ShoStY

Should have used seven hops, because the song only goes up to six hops. :-)


> @@ +694,5 @@
> > +        }
> > +    }
> > +
> > +    const char* timeZone = timeZoneWithZoneInfo + std::strlen(ZoneInfoPath);
> > +    size_t timeZoneLen = linkName + std::strlen(linkName) - timeZone;
> 
> Always nice, IMO, if variables set and then not modified across a big bunch
> of code can be const.  These two can be const, right?  I think?  Too lazy to
> double-check.

Err, eight lines of code (when excluding blank lines and comments) seems hardly a "big bunch of code", so I'll leave it as is. :-p

Comment 8

[André Bargull [:anba]]

Attachment: bug1484829.patch

Updated patch per review comments.

Comment 9

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8decefe3ff7e5d42978a107c4559a4de3ec0f741

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/351795558dc5
Handle arbitrary absolute links for the TZ environment variable. r=Waldo

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/351795558dc5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 13

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

The times are still an hour off for me in the 2018-09-12 nightly.

Comment 14

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

So the reason the fix didn't work for me is that I had only partially switched from using TZ=/home/dbaron/.timezone (which works in most places, but apparently didn't work with something else as well) to TZ=:/home/dbaron/.timezone (with the added ":").  (By partially, I was using the colon in my .bashrc (affecting terminals) but not in my .profile (affecting programs started from GNOME, whose startup uses .profile but not .bashrc).  And I'd only made that change on one of the machines where I was doing this; on another I wasn't using the : at all.)

So... while it's not documented at http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html, the filename syntax generally doesn't actually require the initial colon.

In any case, I've changed my scripts to more reliably use the : since that's apparently the POSIX-ly correct thing to do, but it may be worth supporting filenames without the initial colon, i.e., if they start with a /.

However, I'd also note that according to that documentation, TZ=America/Los_Angeles isn't supported either, and TZ=:America/Los_Angeles is required.  Yet I'd think thinks like TZ=America/Los_Angeles might be one of the common forms of using $TZ.

Comment 15

[Pascal Chevrel:pascalc]

Andre, when you request beta uplift, I suppose that bug 1478370 may also need uplifting as it is a dependency. Thanks

Comment 16

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

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #14)
I filed bug 1491159 on comment 13 / comment 14.

Comment 17

[André Bargull [:anba]]

Comment on attachment 9008000 [details] [diff] [review]
bug1484829.patch

Approval Request Comment
[Feature/Bug causing the regression]: Partly bug 1346211 (The issue was pre-existing, but restricted to Intl Date methods, with bug 1346211 it applies to all JavaScript Date methods.)

[User impact if declined]: Local time without daylight savings instead of the requested time zone is used by JavaScript Date methods when the user has set its TZ environment variable to a link to another time zone file.

[Is this code covered by automated tests?]: No.

[Has the fix been verified in Nightly?]: Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
- Note: This feature doesn't apply to Windows systems.
- 1. Start Firefox with TZ set to a link to a different time zone than your system time zone, e.g. from the console with "TZ=:/usr/share/zoneinfo/America/Los_Angeles ./firefox" assuming the system zone is not already America/Los_Angeles.
- 2. Open the developers console (Ctrl+I) and evaluate |new Date().toString()|. The returned string should display "Pacific Daylight Time" or its equivalent value depending on the system locale, for example "Ora de vară în zona Pacific nord-americană" in Romanian. 

[List of other uplifts needed for the feature/fix]: Bug 1478370.

[Is the change risky?]: No.

[Why is the change risky/not risky?]: Similar to bug 1478370, the changes are for the most part already covered by ICU, so it seems unlikely that this additional code will lead to any complications.

[String changes made/needed]: None.

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9008000 [details] [diff] [review]
bug1484829.patch

Regression fix, approved for 63 Beta 8, thanks.

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c83151139ae0

Comment 20

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Managed to reproduce the initial issue on Ubuntu 16.04 x64, using 63.0a1 (2018-08-20) and the steps provided in comment 0 and comment 17. I can confirm the fix is successfully applied on 64.0a1 (2018-09-19) and on the latest beta 63 taskcluster build (20180919010441.firefox.linux64-debug).