804988 - [Settings] [Clock] [System] System time doesn't take DST into account when changing timezone to DST area (ex, New York)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P2)

Description

[Gene Lian (I already quit Mozilla)]

This is an issue regarding Daylight Saving Time (DST), which might be a critical issue because users would get an incorrect local time when setting system to a DST-included timezone.

STR:
==========
  1) Install 10-24-2012 build for Gaia/Gecko.
  2) Settings > Date & Time > Time Zone
  3) Choose US(Common) > New York (Eastern)

Expected:
==========
  We should see the correct local time which takes DST into account on the following 3 places:
  1) Status bar
  2) Lock screen
  3) Clock app

Actual:
==========
  1) Status bar (wrong! no +1 hour!)
  2) Lock screen (wrong! no +1 hour!)
  3) Clock app (correct!)

Notes:
==========
  1) DST started on 11 March 2012 and ends on 4 November 2012 in U.S., so we can only reproduce (and fix) this issue within this period if we want to take New York as an example.
  2) Current local time in New York: http://www.timeanddate.com/worldclock/city.html?n=179

Comment 1

[Andrew Overholt [:overholt]]

dhylands is hazarding a guess that the timezone is an environment variable which needs to be updated in *all* processes.

Either way, we should be sure we get timezones correct.

Comment 2

[Gene Lian (I already quit Mozilla)]

Hi dhylands,

May I ask where the environment variable you mentioned is? I'm glad to take this if it's due to a Gecko issue.

Comment 3

[Dave Hylands [:dhylands]]

So, on a "normal" linux system, the system clock is in GMT, and the TZ variable can be used to determine the timezone. The default timezone is often determined by the /etc/localtime file.

You can see the effects of the TZ variable by doing:

TZ=PST8PDT date
Wed Oct 31 23:50:51 PDT 2012

TZ=EST5EDT date
Thu Nov  1 02:51:03 EDT 2012

It looks like android uses a property to determine the default:

getprop persist.sys.timezone
PDT+07

The property has the advantage that it's global, whereas the environment variable is per-process.

You can see the code in bionic which uses this by looking at:
bionic/libc/tzcode/localtime.c

Although the property is only read when tzset is called. I took a look at the bionic source code, and tzset gets called whenever any of the following C-runtime functions are called:

localtime
mktime
time2posix
posix2time

So if you call one of those functions or tzset, then the runtime will re-read the persist.sys.timezone and pickup any changes.

It turns out that SetTimezone (from GonkHal.cpp) sets the correct property. So the only thing left is that each process then needs to see it.

dom/system/gonk/TimeZoneSetting.cpp sets up an observer that watches the time.timezone setting, and then calls SetTimezone.

However, it looks like the observer is only called in the main process. This means that processes which are launched after the timezone is changed will see the updated timezone, but processes which are launched before the timezone change won't see the update until they call tzset().

So it looks like you might be able to just put a listener on the time.timezone setting and somehow get GetTimezone to be called.

Comment 4

[Dave Hylands [:dhylands]]

So my guess is that if you boot the phone, change the timezone and the launch the clock app, you should see the correct time in the clock app.

However, if you launch the clock app before changing the setting, then change the setting and then go back to the clock app, then you'll probably see that it has the incorrect time (this assumes the the clock app has the same PID after changing the setting as it did before changing the setting).

I wasn't able to test this since the clock app was crashing on me.

Comment 5

[Gene Lian (I already quit Mozilla)]

Thanks Dave for the detailed information! Very useful! :)

Comment 6

[[e-mail], [e-mail]]

(In reply to Dave Hylands [:dhylands] from comment #4)
> So my guess is that if you boot the phone, change the timezone and the
> launch the clock app, you should see the correct time in the clock app.
> 
> However, if you launch the clock app before changing the setting, then
> change the setting and then go back to the clock app, then you'll probably
> see that it has the incorrect time (this assumes the the clock app has the
> same PID after changing the setting as it did before changing the setting).
> 
For these steps, I can not reproduce the situation.
I see the Clock APP display the correct time without crash.

gaia:  2b1ee6f8389212c46634959a3922288c286bbfc4
gecko: 5a551f7362fffdb3e6471ce59874c404c74913a1

> I wasn't able to test this since the clock app was crashing on me.

Thanks Dave for testing experience between Settings and Clock App.

Comment 7

[[e-mail], [e-mail]]

(In reply to Ian Liu [:ianliu] from comment #6)
> (In reply to Dave Hylands [:dhylands] from comment #4)
> > So my guess is that if you boot the phone, change the timezone and the
> > launch the clock app, you should see the correct time in the clock app.
> > 
> > However, if you launch the clock app before changing the setting, then
> > change the setting and then go back to the clock app, then you'll probably
> > see that it has the incorrect time (this assumes the the clock app has the
> > same PID after changing the setting as it did before changing the setting).
> > 
> For these steps, I can not reproduce the situation.
> I see the Clock APP display the correct time without crash.
> 
I have run the step again with Gene.
We can reproduce the issue without crash.

We always new Date() object to get current time when we need to update UI layout of the time display.

For example, the time display of status bar is updated by System APP.
The time display of Clock APP is updated by Clock APP.
And I dump the log of Date() object in System and Clock APP.
I find out the value are different!!
Why are the two instances of Date() object not sync?
Is the issue related by OOP?

> gaia:  2b1ee6f8389212c46634959a3922288c286bbfc4
> gecko: 5a551f7362fffdb3e6471ce59874c404c74913a1
> 
> > I wasn't able to test this since the clock app was crashing on me.
> 
> Thanks Dave for testing experience between Settings and Clock App.

Comment 8

[Benjamin Chen [:bechen]]

Attachment: Clear the DSTOffsetCache when timezone change is detected.

Root cause: DSTOffesetCache in JS_context is not sync with timezone change.
When the timezone had been changed, the observer will update the "static double LocalTZA" in jsdate.cpp. We store the value in DSTOffesetCache class to see if we need to purge the DSTOffesetCache.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 680005 [details] [diff] [review]
Clear the DSTOffsetCache when timezone change is detected.

Forwarding to Waldo.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 680005 [details] [diff] [review]
Clear the DSTOffsetCache when timezone change is detected.

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

Your comment that DSTOffsetCache isn't sync'd with LocalTZA is true -- but the only access to DSTOffsetCache is conditioned on the Date object's cached localTZA being equal to LocalTZA.  So if LocalTZA is up-to-date, there shouldn't be a problem.  And it seems to me LocalTZA can only be out of date if you haven't called js_ClearDateCaches.  So is the real problem here that you aren't calling js_ClearDateCaches in the right place, or aggressively enough?  This patch looks like it's duplicating the clear-date-caches mechanism, the way I read it.

Comment 11

[Benjamin Chen [:bechen]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> Comment on attachment 680005 [details] [diff] [review]
> Clear the DSTOffsetCache when timezone change is detected.
> 
> Review of attachment 680005 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Your comment that DSTOffsetCache isn't sync'd with LocalTZA is true -- but
> the only access to DSTOffsetCache is conditioned on the Date object's cached
> localTZA being equal to LocalTZA.  So if LocalTZA is up-to-date, there
> shouldn't be a problem.  
jsdate.cpp:
static void
FillLocalTimeSlots(DSTOffsetCache *dstOffsetCache, RawObject obj)
{
    if (!obj->getSlot(JSObject::JSSLOT_DATE_LOCAL_TIME).isUndefined() &&
        obj->getSlot(JSObject::JSSLOT_DATE_TZA).toDouble() == LocalTZA)
    {
        return;
    }
    ...
    obj->setSlot(JSObject::JSSLOT_DATE_TZA, DoubleValue(LocalTZA));
    double utcTime = obj->getDateUTCTime().toNumber();
    ...
    double localTime = LocalTime(utcTime, dstOffsetCache);
}

When Gaia creates a Date object then trying to get time through this object, the  FillLocalTimeSlots function will be called.
Then the LocalTZA is always up-to-date (I have checked), but the code flow still uses the dstOffsetCache which is not up-to-date to get time at Line1315 "double localTime = LocalTime(utcTime, dstOffsetCache);" trigger the bug.

In fact we can only call dstOffsetCache->purge() before Line1315. The problem is solved but it is not a good place to call it because every new Date objects in the same jscontext will purge the dstOffsetCache again and again.
So I call the purge function at DSTOffsetCache::getDSTOffsetMilliseconds in attachment 680005 [details] [diff] [review]

By the way, the RANGE_EXPANSION_AMOUNT in DSTOffsetCache now is 30 days. 
So if the DST will change at Nov 1, we will suffer the cache miss during the whole Oct. For example: open clock app and change it to analog clock, cache miss every seconds.
Maybe we can adjust RANGE_EXPANSION_AMOUNT to a smaller value or change it dynamically?

Comment 12

[Benjamin Chen [:bechen]]

Attachment: Clear the DSTOffsetCache when timezone change is detected.

Refine the purge function call, no logic change.

Comment 13

[Benjamin Chen [:bechen]]

Hello Jeff~~
Would you please help to review the patch?

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 681337 [details] [diff] [review]
Clear the DSTOffsetCache when timezone change is detected.

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

I'm too busy these days.  :-(

js_ClearDateCaches doesn't take a JSContext* now, of course.  But in an ideal world, wouldn't that method be the right place to purge the DST offset cache?  The right fix here is to make js_ClearDateCaches take a JSContext*, then have it purge the DST offset after updating LocalTZA, it seems to me.  What flaw do you see in this?  The LocalTZA stuff is purely an implementation detail, so anything which exposes API to access it is definitely the wrong thing to do.

::: js/src/jsdate.h
@@ +67,5 @@
>  
>  extern void
>  js_ClearDateCaches();
>  
> +extern JS_FRIEND_API(double)

friend API is only necessary for stuff called outside the engine.  This isn't called outside the engine (and per my earlier comments shouldn't be), so it would only have to be a regular extern.

Comment 15

[Benjamin Chen [:bechen]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> Comment on attachment 681337 [details] [diff] [review]
> Clear the DSTOffsetCache when timezone change is detected.
> 
> Review of attachment 681337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm too busy these days.  :-(
> 
> js_ClearDateCaches doesn't take a JSContext* now, of course.  But in an
> ideal world, wouldn't that method be the right place to purge the DST offset
> cache?  The right fix here is to make js_ClearDateCaches take a JSContext*,
> then have it purge the DST offset after updating LocalTZA, it seems to me. 
> What flaw do you see in this?  The LocalTZA stuff is purely an
> implementation detail, so anything which exposes API to access it is
> definitely the wrong thing to do.

I had tried to call the purge function in js_ClearDateCaches. That is a perfect time and place to call purge function. But it doesn't work seems there are multiple JSContext in a process. Only purge one JSContext is not enough.

Is there any way that I can get all JSContext in a process when the observer notify(DateCacheCleaner::Notify) is invoked?

Comment 16

[Jeff Walden [:Waldo]]

Sigh, you're right.  LocalTZA as a static global is the primordial problem, then, which implicates bug 815414 I filed yesterday after noticing a flavor of this issue.

Does this need to be fixed in stable-ish branches to target b2g?  We can live with some sort of hackaround maybe like what's here (I haven't looked at it from the suitability-as-a-hackaround point of view yet -- I'll take a second look now, assuming we do need it for branches) for that, but on trunk it sounds like we have to do bug 815414 to fix this properly.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 681337 [details] [diff] [review]
Clear the DSTOffsetCache when timezone change is detected.

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

Okay, I guess as a hackaround for branches this works, broadly, with a few tweaks.  Post a patch making 'em and I'll get to it in short order.

::: js/src/jscntxt.cpp
@@ +45,5 @@
>  #ifdef JS_ION
>  #include "ion/Ion.h"
>  #include "ion/IonFrames.h"
>  #endif
> +#include "jsdate.h"

This should be alphabetized among the other js*.h headers, which means probably it goes about a dozen lines up (going by the context Splinter's showing me now).

@@ +1311,5 @@
> +/*
> + * We must purge the DSTOffsetCache when timezone change is detected.
> + */
> +void
> +DSTOffsetCache::checkLocalTZA()

Let's name this purgeIfTZAIsStale; with that I don't think we need the comment any more.

@@ +1315,5 @@
> +DSTOffsetCache::checkLocalTZA()
> +{
> +    if (mLocalTZA != js_GetLocalTZA()) {
> +        purge();
> +    }

JS doesn't brace single-line ifs.

::: js/src/jsdate.h
@@ +68,5 @@
>  extern void
>  js_ClearDateCaches();
>  
> +extern JS_FRIEND_API(double)
> +js_GetLocalTZA();

Let's get rid of this and just make LocalTZA into an extern -- put it in the js namespace -- then use it across files directly.

::: js/src/prmjtime.h
@@ +63,5 @@
>      inline void purge();
>  
>    private:
> +    void checkLocalTZA();
> +    double mLocalTZA;

SpiderMonkey doesn't use mFoo for member variable names; just name this localTZA.

Comment 18

[Benjamin Chen [:bechen]]

Attachment: Clear the DSTOffsetCache when timezone change is detected.

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 687655 [details] [diff] [review]
Clear the DSTOffsetCache when timezone change is detected.

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

Looks good with the two mini-tweaks noted.  Dunno how this should be handled as far as landings go, since it's only applicable against branches and can't get trunk baking, but I guess that's your problem to figure out, not mine.  :-)

::: js/src/jsdate.cpp
@@ +317,5 @@
>          result += 7;
>      return result;
>  }
>  
>  /* ES5 15.9.1.7. Set by UpdateLocalTZA(). */

Put this comment by the extern declaration in jsdate.h.

@@ +322,5 @@
> +namespace js {
> +
> +double LocalTZA;
> +
> +} /* namespace js */

Let's just have this be |double js::LocalTZA;|.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hey guys, this patch was never uplifted and neither was the alternative fix that landed in bug 815414.

Reopening this bug so that we can figure out what we can get B2G18 branch.

Comment 22

[Andrew Overholt [:overholt]]

Benjamin, do you know how to proceed here?

Comment 23

[Benjamin Chen [:bechen]]

(In reply to Andrew Overholt [:overholt] from comment #22)
> Benjamin, do you know how to proceed here?
Just provide a patch on code base b2g18 and raise the flag approval‑mozilla‑b2g18 ?

Comment 24

[Benjamin Chen [:bechen]]

Attachment: Clear the DSTOffsetCache when timezone change is detected.

The patch is based on b2g18 code base.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DSTOffsetCache is stale if we change timezone.
User impact if declined: Time is incorrect if change timezone to a DST timezone
Testing completed: Manual testing.
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 25

[JP Rosevear [:jpr]]

Any reason not to land this?

Comment 26

[Justin Lebar (not reading bugmail)]

(In reply to JP Rosevear [:jpr] from comment #25)
> Any reason not to land this?

I suspect that RyanVM will come through and land this once he gets back from the holidays.

Comment 27

[Justin Lebar (not reading bugmail)]

Oh, I see; it's not marked as resolved-fixed, so I bet it doesn't show up in his queries unless we also mark it as checkin-needed.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

(In reply to Justin Lebar [:jlebar] from comment #27)
> Oh, I see; it's not marked as resolved-fixed, so I bet it doesn't show up in
> his queries unless we also mark it as checkin-needed.

Your suspicions would be correct ;) I'll be landing it on inbound shortly.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

s/inbound/b2g18

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c3d33b89a602
https://hg.mozilla.org/releases/mozilla-b2g18/rev/83dfc9ad403f