285615 - Implement a JS_ClearDateCaches method so that embeddings can tell SpiderMonkey to discard cached time zone information

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Matthew Schultz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050217
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050217

I read in bug 173431, comment 3 that Mozilla requires a restart when changing
the timezone in order to get the correct offset to GMT using javascript's
getTimezoneOffset function.  This has the potential to report inaccurate
datetimes to the server assuming the server is converting all local times by
offset to GMT and/or storing them in a database.  I checked the behavior of MSIE
6.0 on Windows XP SP2 and if I were to change the timezone and check the offset
again without reloading the page or restarting the browser, it will reflect the
new timezone offset. 

Reproducible: Always

Steps to Reproduce:
1. Change Timezone of Computer while Mozilla/Firefox is open
2. Output the timezone offset using Javascript

Actual Results:  
The timezone offset remained the same from the point in which the browser started.

Expected Results:  
The timezone offset should reflected the current offset as soon as the javscript
function was called again without restarting the browser and without reloading
the page.

Comment 1

[Matthew Schultz]

Attachment: Test Javasript function getTimezoneOffset after changing timezone

Comment 2

[Bob Clary [:bc] (inactive)]

->core/jseng

Comment 3

[Matthias Versen [:Matti]]

*** Bug 292524 has been marked as a duplicate of this bug. ***

Comment 4

[Dave Townsend [:mossop]]

*** Bug 347196 has been marked as a duplicate of this bug. ***

Comment 6

[Dan Winship]

this is because js_InitDateClass() caches the result of PRMJ_LocalGMTDifference() in a static variable.

also note that if you convert the Date to a string, the string will show the correct current timezone, but still using the offset from the previous timezone So, eg, new Date().toString() would return "Mon Aug 02 2010 10:46:54 GMT-0400 (EST)" before changing timezones, and "Mon Aug 02 2010 10:46:54 GMT-0400 (CET)" after.

Comment 8

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

That might make some date processing a little bit slower but hopefully, we could use something equivalent than the API from bug 714358 to not re-compute the localtza everytime.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 610982 [details] [diff] [review]
Patch v1

I think waldo is probably the right guy to review this... IIRC, the date stuff affects Sunspider pretty heavily.

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to Blake Kaplan (:mrbkap) from comment #9)
> I think waldo is probably the right guy to review this... IIRC, the date
> stuff affects Sunspider pretty heavily.

FWIW, Chrome doesn't have that issue so I believe they do something a bit dynamically. Though, Opera seems to require a re-start to change the timezone. Haven't check other browsers given that they don't run on Linux.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 610982 [details] [diff] [review]
Patch v1

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

Hmm.  I really thought this affected perf, but my recollections of how SunSpider uses date stuff must be subtly wrong, because this is the amount of change I get on the SunSpider date-* tests with this patch (with slight unbitrotting):

TEST                COMPARISON            FROM                 TO             DETAILS
=============================================================================
** TOTAL **:        *1.021x as slow*  88.3ms +/- 0.7%   90.2ms +/- 0.9%     significant
=============================================================================
  date:             *1.023x as slow*  64.5ms +/- 1.0%   66.0ms +/- 0.9%     significant
    format-tofte:   ??                39.9ms +/- 1.1%   40.5ms +/- 1.1%     not conclusive: might be *1.015x as slow*
    format-xparb:   *1.037x as slow*  24.6ms +/- 1.2%   25.5ms +/- 1.1%     significant

  string:           *1.016x as slow*  23.8ms +/- 0.5%   24.2ms +/- 1.0%     significant
    validate-input: *1.016x as slow*  23.8ms +/- 0.5%   24.2ms +/- 1.0%     significant

I suspect that sort of difference could be eaten on SunSpider these days.

But the patch really isn't enough.  It changes things so that queries in SpiderMonkey to get the local time will get an up-to-date value.  But it only does the right thing when the local time is actually queried.  The problem is that all Date objects internally cache the local time.  That cache starts out empty, then gets populated if that date's local time is needed.  See GetAndCacheLocalTime for details.  That cache -- all those caches, in every Date object in existence -- are not cleared when the time zone changes.  So this patch will change attachment 177030 [details]'s behavior, but it wouldn't change it if |var now| in the attachment were computed only once, at page load time (and getTimezoneOffset() were called once at the same time, too, to populate the cache).

You could make an argument that half-correct is better than not-at-all-correct.  However, understanding half-correct, as sites would have to do, means understanding the arcane way we cache local times.  I'm unwilling to require that of web developers who would run into the lingering issues here, were we to take this patch.

I think if we're going to change things here, we need to change them such that a system time zone change affects all Date objects, not just ones whose local times haven't yet been computed.  That seems pretty easy if Date objects cache the TZA used when their local times were computed, then that gets compared against the current TZA when local time info is examined.  But that means you need TZA calculation to be fast, which means you need to avoid the computation when possible, which means you need the system from bug 714358.  So before we fix this bug, I think we need the TZA-change notification system set up first.

Comment 12

[Jeff Walden [:Waldo]]

That was with 100 runs of tests in before/after instances, to be clear, so the differences aren't just one-off oddities.

Comment 13

[Gene Lian (I already quit Mozilla)]

It seems the Alarm API (bug 749551) depends on this as well, because it needs a way to know the current system timezone to dynamically adjust the alarm setting time whenever the timezone is changed at run-time.

Not sure if this bug could be too complicated to be fixed in a short time (since it was reported at 2005 in the first place...:O)? If this one could take a long time to go, may I ask is there any possible alternative to get the current system timezone offset either in JS or C++?

Any suggestions or comments are highly appreciated :)

Thanks,
Gene

Comment 14

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #13)
> Not sure if this bug could be too complicated to be fixed in a short time
> (since it was reported at 2005 in the first place...:O)? If this one could
> take a long time to go, may I ask is there any possible alternative to get
> the current system timezone offset either in JS or C++?

Reply to myself: if it's an linux-based platform (like B2G), we can calculate the timezone offset as the following C code segment, where the mktime(), gmtime() and localtime() are also capable of dealing with the Daylight Saving Time effect.

#include <time.h>

int
GetTimezoneOffset()
{
  time_t rawTime, gmtTime, localTime;
  rawTime = time(NULL);
  gmtTime = mktime(gmtime(&rawTime));
  localTime = mktime(localtime(&rawTime));
  return static_cast<int>(difftime(gmtTime, localTime)) / 60;
}

Gene

Comment 15

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #14)
> Reply to myself: if it's an linux-based platform (like B2G), we can
> calculate the timezone offset as the following C code segment, where the
> mktime(), gmtime() and localtime() are also capable of dealing with the
> Daylight Saving Time effect.
> 
> #include <time.h>
> 
> int
> GetTimezoneOffset()
> {
>   time_t rawTime, gmtTime, localTime;
>   rawTime = time(NULL);
>   gmtTime = mktime(gmtime(&rawTime));
>   localTime = mktime(localtime(&rawTime));
>   return static_cast<int>(difftime(gmtTime, localTime)) / 60;
> }

Oops... The above logic is wrong. The following should be correct and more flexible.

int
GetTimezoneOffset(bool aIgnoreDST)
{
  struct tm* timeInfo;
  time_t rawTime = time(NULL);

  timeInfo = gmtime(&rawTime);
  int gmtHour = timeInfo->tm_hour;

  timeInfo = localtime(&rawTime);
  int localHour = timeInfo->tm_hour;
  if (aIgnoreDST && timeInfo->tm_isdst > 0) {
    localHour -= 1;
  }

  return (gmtHour - localHour) * 60;
}

Anyway, these codes have nothing to do with this bug. Just trying to design an equivalent C/C++ logic like .getTimezoneOffset().

Thanks,
Gene

Comment 16

[David Mandelin [:dmandelin]]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #11)
> I think if we're going to change things here, we need to change them such
> that a system time zone change affects all Date objects, not just ones whose
> local times haven't yet been computed.  That seems pretty easy if Date
> objects cache the TZA used when their local times were computed, then that
> gets compared against the current TZA when local time info is examined.  But
> that means you need TZA calculation to be fast, which means you need to
> avoid the computation when possible, which means you need the system from
> bug 714358.  So before we fix this bug, I think we need the TZA-change
> notification system set up first.

Apparently B2G wants this. I can see 2 ways to get it done:

1. Get timezone change notifications (bug 714358), then add a clear-date-cache API to SpiderMonkey, and finally add a listener (listeners?) in the DOM (or some B2G thing?) to connect them. The new API could be worked on concurrently with the change notifications.

2. Disable caching of both timezones and local times on B2G. 

The main problem with #1 is that it's gated on bug 713358, and I have no idea how close that one is. Otherwise it seems much preferable to #2: #2 gives a perf hit to B2G and requires special testing.

Comment 17

[David Mandelin [:dmandelin]]

Sean's going to take this on.

Comment 18

[Mounir Lamouri (:mounir)]

This issue is bigger in B2G but still exists in all systems. And also given that we want the Time API for B2G and we could reuse the HAL interface for other platforms, I think we should definitely use the first solution.

With HAL, any C++ code can register to get notifications. We could have a component created at startup just for that (which would be multi-platform). Otherwise, a temporary solution would be to call the JSAPI from the Gonk backend.
I could do the bridge between the JSAPI and the HAL timezone change notifications.

Comment 19

[Sean Stangl [:sstangl]]

We should focus on solving this bug for B2G while not regressing behavior on the desktop: Bug 714358 will yield a notification framework for propagating TZ change events, but event generation ultimately requires per-OS support, which is a larger project. TZ update support for B2G is time-sensitive and strictly easier, so let's handle that first.

I'll have a patch for the JS-side soon, going with the first solution.

Mounir, handling the bridge would be very helpful. Thanks.

Comment 20

[Sean Stangl [:sstangl]]

Attachment: Define JS_ClearDateCaches().

Implements a new public API JS_ClearDateCaches(), which updates the LocalTZA.

Date objects cache their TZAs, and check for cache invalidation by comparing against the (static) global TZA.

This avoids querying the system timezone frequently, preserving existing performance. However, it means that updating the TZA becomes the responsibility of the host environment through the notification system in Bug 714358. Currently support is only being planned for B2G, so the bug will remain unfixed on each OS until someone hooks up some flavor of event handler.

Added in a bunch of style fixes to coast along with this patch. They can be removed if you would prefer minimalism.

Comment 21

[Sean Stangl [:sstangl]]

Attachment: Define JS_ClearDateCaches(). [v2]

Some old cruft was accidentally left in the previous patch.

Comment 22

[Mounir Lamouri (:mounir)]

Can we have a C++ unit test for that? It could change the timezone, call the JSAPI method and checks that the Date object TZ has actually changed. I'm not sure if we have access to the JSAPI from those tests though.

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 638554 [details] [diff] [review]
Define JS_ClearDateCaches(). [v2]

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

Why all the extraneous style fixes in jsdate.cpp?  Not that I'm opposed to any of them, but it would have been nice to keep them to a separate patch, and keep this limited to just the TZA-caching changes.

::: js/src/jsapi.h
@@ +5471,5 @@
>  extern JS_PUBLIC_API(JSBool)
>  JS_ObjectIsDate(JSContext *cx, JSObject *obj);
>  
> +extern JS_PUBLIC_API(void)
> +JS_ClearDateCaches(JSContext *cx);

Give me an API comment by this, please, explaining what it does and when to call it.

::: js/src/jsdate.cpp
@@ +384,1 @@
>  static double LocalTZA;

I'd have this refer only to UpdateLocalTZA, myself, and have people search for callers of that to learn more.  Better still would be a class hiding this value completely (so you'd get it and set it using methods on the class), but that may be more trouble than it's worth.

@@ +1351,5 @@
>  
>      return true;
>  }
>  
> +static inline bool

In C++, for standalone methods, there's no reason to use |static| -- just |inline bool| will do the trick.

@@ +1352,5 @@
>      return true;
>  }
>  
> +static inline bool
> +NeedCacheLocalTime(JSObject *obj)

This name doesn't work for me.  How about ShouldUpdateCachedLocalTime?

@@ +1361,5 @@
> +    if (obj->getSlot(JSObject::JSSLOT_DATE_LOCAL_TIME).isUndefined())
> +        return true;
> +
> +    /* System timezone updated since cache was populated. */
> +    double CachedTZA = obj->getSlot(JSObject::JSSLOT_DATE_TZA).toDouble();

cachedTZA

@@ +1365,5 @@
> +    double CachedTZA = obj->getSlot(JSObject::JSSLOT_DATE_TZA).toDouble();
> +    if (CachedTZA != LocalTZA)
> +        return true;
> +
> +    return false;

return cachedTZA != LocalTZA;

Or you could just do return obj->getSlot(JSObject::JSSLOT_DATE_TZA).toDouble() != LocalTZA, up to you.  I don't think the local adds a whole lot, myself.

@@ +1378,1 @@
>      return true;

I'd invert these and do if (!ShouldUpdateCachedLocalTime(obj)) return true; return Fill.... to get the common case out of the way first.  Heck, I'd just add if (!...) at the top of FillLocalTimes, rename it to GetCachedLocalTime, and kill this function entirely.  The extra level of abstraction doesn't help, in my opinion.

::: js/src/jsdate.h
@@ +66,5 @@
>                   int hour, int min, int sec);
>  
> +extern JS_FRIEND_API(void)
> +js_ClearDateCaches();
> +

This doesn't need to be friend API if there's also JS_ClearDateCaches, right?

::: js/src/jsobj.h
@@ +623,5 @@
> +    static const uint32_t JSSLOT_DATE_LOCAL_DATE = 5;
> +    static const uint32_t JSSLOT_DATE_LOCAL_DAY = 6;
> +    static const uint32_t JSSLOT_DATE_LOCAL_HOURS = 7;
> +    static const uint32_t JSSLOT_DATE_LOCAL_MINUTES = 8;
> +    static const uint32_t JSSLOT_DATE_LOCAL_SECONDS = 9;

I'd make these all JSSLOT_DATE_COMPONENTS_START + {0,1,2,3,4,5,6,7}, myself, but this works.

@@ +628,2 @@
>  
> +    static const uint32_t DATE_CLASS_RESERVED_SLOTS = 10;

And I'd make this JSSLOT_DATE_LOCAL_SECONDS + 1.

Comment 24

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8590078b5508

This implements the JS-side, but the bug remains unresolves. To resolve, the timezone change notification system from Bug 714358 must land (initially just for B2G), and then the event must be propagated through to JS_ClearDateCaches().

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8590078b5508