Closed Bug 1484829 Opened Last year Closed Last year

times in Slack are an hour off

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 + verified
firefox64 --- verified

People

(Reporter: dbaron, Assigned: anba)

References

()

Details

(Keywords: regression, site-compat)

Attachments

(1 file, 1 obsolete file)

[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.)
Summary: times in Slack ar an hour off → times in Slack are an hour off
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".
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.
Tracked for 63 since it's a recent regression.
Depends on: 1478370
Attached patch bug1484829.patch (obsolete) — Splinter Review
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.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #9004984 - Flags: review?(jwalden+bmo)
Jeff, could you review this patch? Thanks!
Flags: needinfo?(jwalden+bmo)
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.
Attachment #9004984 - Flags: review?(jwalden+bmo) → review+
Flags: needinfo?(jwalden+bmo)
(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
Attached patch bug1484829.patchSplinter Review
Updated patch per review comments.
Attachment #9004984 - Attachment is obsolete: true
Attachment #9008000 - Flags: review+
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/351795558dc5
Handle arbitrary absolute links for the TZ environment variable. r=Waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/351795558dc5
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Please request Beta approval on this when you get a chance.
Flags: needinfo?(andrebargull)
The times are still an hour off for me in the 2018-09-12 nightly.
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.
Andre, when you request beta uplift, I suppose that bug 1478370 may also need uplifting as it is a dependency. Thanks
(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #14)
I filed bug 1491159 on comment 13 / comment 14.
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.
Flags: needinfo?(andrebargull)
Attachment #9008000 - Flags: approval-mozilla-beta?
Comment on attachment 9008000 [details] [diff] [review]
bug1484829.patch

Regression fix, approved for 63 Beta 8, thanks.
Attachment #9008000 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.