717150 - Support for adjusting the system clock and setting the time zone

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Let's do the work in these steps

Part 1: 
 - Add a hal/Hal.h API for adjusting the system clock.  The API should be
     bool AdjustSystemClock(int64_t aDeltaMilliseconds);
This should return true only if the system clock was successfully adjusted.  See below.
 - Add a hal/fallback/SystemClock.cpp file that implements |AdjustSystemClock()| by just returning false.
 - Add an implementation of |AdjustSystemClock()| to hal/sandbox/SandboxHal.cpp that proxies the call to its parent process.  Copy how the other calls there work.
 - Add a hal/posix/SystemClock.cpp file that implements |AdjustSystemClock()|.  Only build this file when |ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))|; see hal/Makefile.in.  We can use this backend on other OSes, but we'll never have the right permissions except on Gonk.  The implementation of |AdjustSystemClock()| needs to do the following
   * call sched_yield()
   * call clock_gettime(REALTIME) to get the current time, adjust it by aDeltaMilliseconds, then call clock_settime(REALTIME)
The sched_yield() is to try to keep the gettime/settime in the same time slice.  If the process is context-switched out in between, it introduces some clock error.  In the future we should use wake locks here too.

Part 2:
 - Add a Hal API
     bool SetTimezone(const nsCString& aTimezoneSpec);
It should also return true iff the timezone was set successfully.
 - Add FallbackHal and SandboxHal implementations like above.
 - Add an implementation of |SetTimezone()| to hal/gonk/GonkHal.cpp.  This implementation needs to at least
   * set "persist.sys.timezone" to the new value, in the android property DB.
It looks like bionic libc handles time zone changes correctly.

This is probably enough for this bug.  The next part of the work will be notifying listeners of system-clock and timezone changes at the HAL level, propagating those changes across processes, and then hooking up a DOM API.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Hi Chris, 

I am implementing this bug right now. Can I just add the AdjustSystemClock() function to GonkHal.cpp ? If the answer is no, should we add the SetTimezone() function to hal/posix/SysteClock.cpp, too ? 

In order to proxy Call to parent process, it is also necessary to define AdjustSystemClock() in parent part of protocol PHal structure in PHal.ipdl, right ? And add the below function in SandboxHal.cpp to proxy Call to parent process. 
void
AdjustSystemClock()
{
  Hal()->SendAdjustSystemClock();
}

Also implementing below function in Hal.cpp to call AdjustSystemClock() function in SystemClock.cpp. 
bool AdjustSystemClock()
{
  AssertMainThread();
  RETURN_PROXY_IF_SANDBOXED(AdjustSystemClock());
}

Because I am new to gecko, any comments are very welcome. 

BTW, how can I output some debug message to logcat in Hal, I try to call HAL_LOG() but nothing is shown. 

Regards
Vincent

Comment 2

[Mounir Lamouri (:mounir)]

Wouldn't be better to draft an API in dev-webapi before implementing the backend here?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Vincent Chang from comment #1)
>
> I am implementing this bug right now. Can I just add the AdjustSystemClock()
> function to GonkHal.cpp ? If the answer is no, should we add the
> SetTimezone() function to hal/posix/SysteClock.cpp, too ? 
>

Adding AdjustSystemClock() to GonkHal.cpp is fine with me.

SetTimezone() needs to use non-POSIX, Gonk-specific APIs, so it needs to in GonkHal.cpp.
 
> In order to proxy Call to parent process, it is also necessary to define
> AdjustSystemClock() in parent part of protocol PHal structure in PHal.ipdl,
> right ? And add the below function in SandboxHal.cpp to proxy Call to parent
> process. 
> void
> AdjustSystemClock()
> {
>   Hal()->SendAdjustSystemClock();
> }
> 
> Also implementing below function in Hal.cpp to call AdjustSystemClock()
> function in SystemClock.cpp. 
> bool AdjustSystemClock()
> {
>   AssertMainThread();
>   RETURN_PROXY_IF_SANDBOXED(AdjustSystemClock());
> }
> 

Yes, that's right.  Try to follow the example of other calls that are proxied in that file.

> Because I am new to gecko, any comments are very welcome. 
> 

No problem!  Feel free to ask questions :).

> BTW, how can I output some debug message to logcat in Hal, I try to call
> HAL_LOG() but nothing is shown. 
> 

#include "android/log.h"
#define LOG(args...)  __android_log_print(ANDROID_LOG_INFO, "GonkHal" , ## args)

will send LOG("foo") messages to logcat.  I'm not sure why HAL_LOG() isn't going to logcat, but I wouldn't worry about that right now.

Comment 4

[Vincent Chang[:vchang][changyihsin]]

Hi Chris, 

Thanks for your comments. 
I have one more question related to AdjustSystemClock(int64_t aDeltaMilliseconds) function. 
The parameter aDeltaMilliseconds is related to current system time in milliseconds right ? 
It maybe positive or negative number ?
Should we provide others API which set system time directly at this moment ? I found that Android platform provides lots of time related APIs. 

Best Regard
Vincent

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hey Vincent, sorry I missed your comment here.

 - yes, aDeltaMilliseconds is the delta from the current system clock.  It can be positive or negative.
 - I don't understand your second question.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For the clock_settime() issue, use this function

static int
sys_clock_settime(clockid_t clk_id, const struct timespec *tp)
{
  return syscall(__NR_clock_settime, clk_id, tp);
}

just like you would use clock_settime().

Comment 7

[Vincent Chang[:vchang][changyihsin]]

Attachment: add hal api to adjust system time and timezone

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 596630 [details] [diff] [review]
add hal api to adjust system time and timezone

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

Why SetTimeZone returns a boolean if this boolean is always true?

Also, the coding style says function definition should be done like this
retval
Class::Function()
{
}

You don't follow that sometimes.

Comment 9

[Thinker Li [:sinker]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #8)
> Comment on attachment 596630 [details] [diff] [review]
> add hal api to adjust system time and timezone
> 
> Review of attachment 596630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why SetTimeZone returns a boolean if this boolean is always true?
Gonk is one of implementations.  Not every platform always does it successful.

By the way.  I suggest to print warning/error messages for empty functions and return false.  Do not fail silently.

Comment 10

[Mounir Lamouri (:mounir)]

Actually, you should have a hal/fallback/FallbackTiming.cpp (or whatever name you prefer) that all backends except gonk should use. I think you might find some other fallback files like that. Look at hal/Makefile.in to see how it works.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 596630 [details] [diff] [review]
add hal api to adjust system time and timezone

I don't understand why SetTimezone() returns a bool but
AdjustSystemClock() doesn't.  They can both fail.  I'm perfectly fine
with leaving both of them with |void| return values and adding in
error notifications in a followup patch, which will probably need to
be asynchronous.  Either way, the return values of the two functions
should be consistent.

>diff --git a/hal/android/AndroidHal.cpp b/hal/android/AndroidHal.cpp

>+void
>+AdjustSystemClock(int64_t aDeltaMilliseconds)
>+{}
>+
>+bool 
>+SetTimezone(const nsCString& aTimezoneSpec)
>+{}
>+

I agree with Mounir in comment 10.  Let's factor this out into a file
in hal/fallback.  Follow the example of the other files in there.

>diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp

>+#if defined(MOZ_WIDGET_GONK)

This is always true in GonkHal, no need to guard on it.

>+static int
>+sys_clock_settime(clockid_t clk_id, const struct timespec *tp)

Please document that this is the POSIX clock_settime() function, but
it's just not exposed through bionic.

>+int64_t
>+timespec_milliseconds(struct timespec *a) 

I don't think this helper is necessary.  See below.

>+void 
>+AdjustSystemClock(int64_t aDeltaMilliseconds)
>+{
>+  millisecond = timespec_milliseconds(&now);
>+  millisecond += aDeltaMilliseconds;
>+  now.tv_sec = millisecond/1000; 
>+  now.tv_nsec = (millisecond%1000)*1000000;

This would be a lot simpler as

  now.tv_sec += aDeltaMilliseconds / 1000;
  now.tv_nsec += kNsecPerMsec * (aDeltaMilliseconds % 1000);

where kNsecPerMsec is a constant defined elsewhere.  With more than
three 0's it gets very hard to scan the integer literal when it's used
in arithmetic expressions.

>+bool 
>+SetTimezone(const nsCString& aTimezoneSpec)
>+{ 
>+  property_set("persist.sys.timezone", aTimezoneSpec.get());

According to spec, it's supposed to be called automatically when
needed, but I think we should call tzset() here to be safe.  bionic
does not have a good record of following POSIX specifications.

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl

>+    AdjustSystemClock(int64 aDeltaMilliseconds);
>+    SetTimezone(nsCString aTimezoneSpec);
> 

The return value from SetTimezone() isn't propagated here so let's
just drop the return value.

>diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp

>+void
>+SetTimezone(const nsCString& aTimezoneSpec)
>+{	
>+  Hal()->SendSetTimezone(nsCString(aTimezoneSpec));
>+} 
>+

This won't link, because the definition of this function doesn't match
the declaration.  Did you try compiling this patch?

This patch looks very good.  I'd like to see a second version with the
comments above fixed.  I'm clearing the review request here to mean,
"the approach of the patch looks good, but I'd like to see a second
version."

Comment 12

[Vincent Chang[:vchang][changyihsin]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> Comment on attachment 596630 [details] [diff] [review]
> add hal api to adjust system time and timezone
> 
> I don't understand why SetTimezone() returns a bool but
> AdjustSystemClock() doesn't.  They can both fail.  I'm perfectly fine
> with leaving both of them with |void| return values and adding in
> error notifications in a followup patch, which will probably need to
> be asynchronous.  Either way, the return values of the two functions
> should be consistent.
> 
> >diff --git a/hal/android/AndroidHal.cpp b/hal/android/AndroidHal.cpp
> 
> >+void
> >+AdjustSystemClock(int64_t aDeltaMilliseconds)
> >+{}
> >+
> >+bool 
> >+SetTimezone(const nsCString& aTimezoneSpec)
> >+{}
> >+
> 
> I agree with Mounir in comment 10.  Let's factor this out into a file
> in hal/fallback.  Follow the example of the other files in there.
 
> >diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp
> 
> >+#if defined(MOZ_WIDGET_GONK)
> 
> This is always true in GonkHal, no need to guard on it.
> 
> >+static int
> >+sys_clock_settime(clockid_t clk_id, const struct timespec *tp)
> 
> Please document that this is the POSIX clock_settime() function, but
> it's just not exposed through bionic.

I will add comments to this function. 
 
> >+int64_t
> >+timespec_milliseconds(struct timespec *a) 
> 
> I don't think this helper is necessary.  See below.
> 
> >+void 
> >+AdjustSystemClock(int64_t aDeltaMilliseconds)
> >+{
> >+  millisecond = timespec_milliseconds(&now);
> >+  millisecond += aDeltaMilliseconds;
> >+  now.tv_sec = millisecond/1000; 
> >+  now.tv_nsec = (millisecond%1000)*1000000;
> 
> This would be a lot simpler as
> 
>   now.tv_sec += aDeltaMilliseconds / 1000;
>   now.tv_nsec += kNsecPerMsec * (aDeltaMilliseconds % 1000);

Is it possible that now.tv_nsec will greater than 1 second after plus aDeltaMilliseconds ?  

> where kNsecPerMsec is a constant defined elsewhere.  With more than
> three 0's it gets very hard to scan the integer literal when it's used
> in arithmetic expressions.
> 
> >+bool 
> >+SetTimezone(const nsCString& aTimezoneSpec)
> >+{ 
> >+  property_set("persist.sys.timezone", aTimezoneSpec.get());
> 
> According to spec, it's supposed to be called automatically when
> needed, but I think we should call tzset() here to be safe.  bionic
> does not have a good record of following POSIX specifications.

So I should call tzset() after property_set() to make sure timezone will be set
after we modify it ? 

> >diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
> 
> >+    AdjustSystemClock(int64 aDeltaMilliseconds);
> >+    SetTimezone(nsCString aTimezoneSpec);
> > 
> 
> The return value from SetTimezone() isn't propagated here so let's
> just drop the return value.
> 
> >diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp
> 
> >+void
> >+SetTimezone(const nsCString& aTimezoneSpec)
> >+{	
> >+  Hal()->SendSetTimezone(nsCString(aTimezoneSpec));
> >+} 
> >+
> 
> This won't link, because the definition of this function doesn't match
> the declaration.  Did you try compiling this patch?

Sorry, it's my typo. 
I had compiled this code, but compiler didn't generate any error. Function declaration in Hal.h and function definition in SandboxHal.cpp have different namespaces. It maybe the reason.   


> This patch looks very good.  I'd like to see a second version with the
> comments above fixed.  I'm clearing the review request here to mean,
> "the approach of the patch looks good, but I'd like to see a second
> version."

I will have a second version with the comments above fixed soon.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Vincent Chang from comment #12)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> > >+void 
> > >+AdjustSystemClock(int64_t aDeltaMilliseconds)
> > >+{
> > >+  millisecond = timespec_milliseconds(&now);
> > >+  millisecond += aDeltaMilliseconds;
> > >+  now.tv_sec = millisecond/1000; 
> > >+  now.tv_nsec = (millisecond%1000)*1000000;
> > 
> > This would be a lot simpler as
> > 
> >   now.tv_sec += aDeltaMilliseconds / 1000;
> >   now.tv_nsec += kNsecPerMsec * (aDeltaMilliseconds % 1000);
> 
> Is it possible that now.tv_nsec will greater than 1 second after plus
> aDeltaMilliseconds ?  
> 

You're right!  Thanks.  Your original implementation is ok, but please name the helper function like this

int64_t
TimespecToMilliseconds(struct timespec *aTs)
{

to follow coding style.

> > where kNsecPerMsec is a constant defined elsewhere.  With more than
> > three 0's it gets very hard to scan the integer literal when it's used
> > in arithmetic expressions.
> > 
> > >+bool 
> > >+SetTimezone(const nsCString& aTimezoneSpec)
> > >+{ 
> > >+  property_set("persist.sys.timezone", aTimezoneSpec.get());
> > 
> > According to spec, it's supposed to be called automatically when
> > needed, but I think we should call tzset() here to be safe.  bionic
> > does not have a good record of following POSIX specifications.
> 
> So I should call tzset() after property_set() to make sure timezone will be
> set
> after we modify it ? 
> 

That's my recommendation.  It shouldn't be necessary, but let's be safe.

> > >diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp
> > 
> > >+void
> > >+SetTimezone(const nsCString& aTimezoneSpec)
> > >+{	
> > >+  Hal()->SendSetTimezone(nsCString(aTimezoneSpec));
> > >+} 
> > >+
> > 
> > This won't link, because the definition of this function doesn't match
> > the declaration.  Did you try compiling this patch?
> 
> Sorry, it's my typo. 
> I had compiled this code, but compiler didn't generate any error. Function
> declaration in Hal.h and function definition in SandboxHal.cpp have
> different namespaces. It maybe the reason.   
> 

Hal.cpp should be see a declaration of |bool hal_sandbox::SetTimezone()| and try to call that.  It's odd that the code still links ...

Comment 14

[Thinker Li [:sinker]]

(In reply to Chris Jones [:cjones][:warhammer] from comment #13)
> Hal.cpp should be see a declaration of |bool hal_sandbox::SetTimezone()| and
> try to call that.  It's odd that the code still links ...
Name mangling does not encode return type of a function in its symbol in objects.
I think it is the reason why it works.

Comment 15

[Vincent Chang[:vchang][changyihsin]]

Attachment: add hal api to adjust system time and timezone version 2

revise the implementation based on Chris's comment

Comment 16

[Mozilla RelEng Bot]

Try run for 8e94f65e9461 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8e94f65e9461
Results (out of 214 total builds):
    success: 181
    warnings: 19
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8e94f65e9461

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 598185 [details] [diff] [review]
add hal api to adjust system time and timezone version 2

Looks good, thanks!

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c24d8549c37

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5c24d8549c37