Support for adjusting the system clock and setting the time zone

RESOLVED FIXED in mozilla13

Status

()

Core
Hardware Abstraction Layer (HAL)
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cjones, Assigned: vchang)

Tracking

(Blocks: 1 bug)

Trunk
mozilla13
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Assignee)

Updated

6 years ago
Assignee: nobody → vchang
(Assignee)

Comment 1

6 years ago
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
Wouldn't be better to draft an API in dev-webapi before implementing the backend here?
(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.
(Assignee)

Comment 4

6 years ago
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
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.
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().
(Assignee)

Comment 7

6 years ago
Created attachment 596630 [details] [diff] [review]
add hal api to adjust system time and timezone
Attachment #596630 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #596630 - Flags: review? → review?(jones.chris.g)
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

6 years ago
(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.
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 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."
Attachment #596630 - Flags: review?(jones.chris.g)
(Assignee)

Comment 12

6 years ago
(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.
(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 ...
(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.
(Assignee)

Comment 15

6 years ago
Created attachment 598185 [details] [diff] [review]
add hal api to adjust system time and timezone version 2

revise the implementation based on Chris's comment
Attachment #596630 - Attachment is obsolete: true

Comment 16

6 years ago
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/vchang@mozilla.com-8e94f65e9461
(Assignee)

Updated

6 years ago
Attachment #598185 - Flags: review?(jones.chris.g)
Comment on attachment 598185 [details] [diff] [review]
add hal api to adjust system time and timezone version 2

Looks good, thanks!
Attachment #598185 - Flags: review?(jones.chris.g) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c24d8549c37
Keywords: checkin-needed
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/5c24d8549c37
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.