B2G system time: adjust system clock after receiving NITZ timestamp

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: philikon, Assigned: airpingu)

Tracking

({feature})

unspecified
mozilla18
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [mentor=philikon][LOE:M])

Attachments

(2 attachments, 10 obsolete attachments)

12.61 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
9.62 KB, patch
airpingu
: review+
Details | Diff | Splinter Review
In bug 714352 we added processing NITZ information from the provider, but we don't actually change the clock time anywhere.

https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#423
Seems like a blocker.

philikon, please re-assign if you're not the best person to own this.
Assignee: nobody → philipp
blocking-basecamp: ? → +
Gene, can you take this? Thanks!
Assignee: philipp → clian
(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> Gene, can you take this? Thanks!

Hi :philikon,

Sure :) I'll come back with patches after studying through this issue. Btw, do we need to clean up the whiteboard [good first bug][lang=js][mentor=philikon], which seems copied from other issue I guess? Or it's just what you expected? ;)
Depends on: 714352
Hi :jlebar and Steven,

I'd like to change the TimeManager to be a generic service, because we need to internally use such a service to reset the system time from the platform in addition to the navigator.mozTime.

May I invite you guys to take a glance on this part-1 patch to make sure it's a proper way to solve this issue? Note that this patch is still a WIP. Please let me know if you don't agree with me. :)
Attachment #660362 - Flags: feedback?(slee)
Attachment #660362 - Flags: feedback?(justin.lebar+bug)
Note that this service is going to be used in JS codes (RadioInterfaceLayer.js).
Keywords: feature
Whiteboard: [good first bug][lang=js][mentor=philikon] → [mentor=philikon][LOE:M]
> +  nsCOMPtr<nsIDOMMozTimeManager> pTimeManager =
> +    do_GetService(TIMEMANAGER_CONTRACTID);

We don't use the |p| prefix for pointers.  Just |timeManager| would be fine.

If you're going to use do_GetService, you need to null-check the result, because do_GetService returns null after shutdown.  In particular:

>+  nsCOMPtr<nsIDOMMozTimeManager> pTimeManager =
>+    do_GetService(TIMEMANAGER_CONTRACTID);
>+  NS_ADDREF(*aTime = pTimeManager);

should be NS_IF_ADDREF (which does a null-check before calling addref).

The null-check wasn't necessary before because operator new never returns null in our code.  If we can't fulfill a |new| due to OOM, we simply crash instead.  (This is our "infallible new".)

I might prefer that you used TimeManager::GetInstance() instead of do_GetService() (in which the utility of the null-check is questionable, but I'd leave it in, myself), because I prefer strong-typing to weak, but that doesn't really matter.
Comment on attachment 660362 [details] [diff] [review]
Part 1, Change TimeManager to be a service

Looks good aside from those nits.
Attachment #660362 - Flags: feedback?(justin.lebar+bug) → feedback+
Hm, it does strike me as a little weird to expose a DOM object as a service...  It's not clear to me that's the right thing to do, or even that it's a safe thing to do.  Olli, should we separate out the DOM object part and the service part, or does it not matter?
Attachment #660362 - Flags: feedback?(bugs)
Comment on attachment 660362 [details] [diff] [review]
Part 1, Change TimeManager to be a service

Yeah, not this way. Each web page should get their own instance.
We don't want any special cases which might affect for example to
C++<->JS bindings.
Attachment #660362 - Flags: feedback?(bugs) → feedback-
Hi Justin,

Could you please review the new patch when you have a chance? Please feel free to let me know if you don't have enough bandwith. :) Some notes summarized for your convenience:

1. Based on :smaug and your concerns, I eventually designed a new TimeService, which exposes generic APIs not only to TimeManager but also to internal needs.

2. The remaining parts are code clean-ups, where lots of includes are not needed at all for TimeManager. Also, some minor changes for Makefile.in to follow most of our conventions, like exporting namespace and patch.

Please let me know if I can do anything better. ;)
Attachment #660362 - Attachment is obsolete: true
Attachment #660362 - Flags: feedback?(slee)
Attachment #661712 - Flags: review?(justin.lebar+bug)
Depends on: 714358
Depends on: 783021
> I'd like to change the TimeManager to be a generic service, because we need to internally use such a 
> service to reset the system time from the platform in addition to the navigator.mozTime.

Now that I understand this code a bit better, I don't understand this part.

Is the issue that you want to set the system time from a JS component where you don't have access to navigator.mozTime?  Otherwise, I don't understand why you can't either set the time via navigator.mozTime or simply call hal::SetSystemTime.
(In reply to Justin Lebar [:jlebar] from comment #11)
> > I'd like to change the TimeManager to be a generic service, because we need to internally use such a 
> > service to reset the system time from the platform in addition to the navigator.mozTime.
> 
> Now that I understand this code a bit better, I don't understand this part.
> 
> Is the issue that you want to set the system time from a JS component where
> you don't have access to navigator.mozTime?  Otherwise, I don't understand
> why you can't either set the time via navigator.mozTime or simply call
> hal::SetSystemTime.

Hi Justin! Thanks for your replies!

I'm going to set the system time from RadioInterfaceLayer.js (http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#423). I'm wondering can we really call navigator.mozTime.set() there? First of all, I cannot find any existing examples that directly calls navigator.XXX within Gecko. Also, the radio part lives in the chrome process. Does that sound reasonable using APIs registered in navigator? Also, since the radio part is designed by JS codes, we need to construct an interface to call hal:: which is C++. That's why I'm hoping to design a generic service to tackle the above-mentioned issues.

Please let me know if you don't agree with me. :)
Hi :philikon,

I'd like to invite you to review this part. Some notes are summarized for your convenience of review:

1. s/timeInSeconds/timeInMS because I see no reason why we need to strip out the last 3 digits if our time system can accept more precise value.

2. s/localTimeStampInMS/receiveTimeInMS where I think it's more descriptive.

3. -(tz * 15) so that it can be consistent with the value returned by Date().getTimezoneOffset().

4. We have 2 steps for synchronizing the system time with NITZ time:
  a. clock time: using a new service designed in part-1 patch.
  b. timezone: using the settings API with key |time.timezone|.

Please feel free to let me know if anything can be improved. :) Thanks!
Attachment #662069 - Flags: review?(philipp)
Depends on: 791962
No longer depends on: 791962
> I'm going to set the system time from RadioInterfaceLayer.js

I see...okay, thanks!  I think this is the right thing to do.  You don't have a window in that context, so you don't have a navigator.
Comment on attachment 661712 [details] [diff] [review]
Part 1, Provide TimeService for Internal Use

>diff --git a/dom/time/nsITimeService.idl b/dom/time/nsITimeService.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/time/nsITimeService.idl
>@@ -0,0 +1,25 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+
>+%{C++
>+#define NS_TIMESERVICE_CID { 0x80d6f9cc, 0xf16d, 0x40c3, { 0xa5, 0x2e, 0xc4, 0xe6, 0x56, 0xe3, 0x65, 0xb4 } }
>+#define TIMESERVICE_CONTRACTID "@mozilla.org/time/timeservice;1"
>+%}
>+
>+[scriptable, builtinclass, uuid(1fc7fde2-0090-11e2-bdd6-0fea4b9f41f8)]
>+interface nsITimeService : nsISupports
>+{
>+  /* Set the system time.
>+   *
>+   * The |time| argument can be either a Date object or a number.
>+   *
>+   * - If |time| is a number, it's interpreted as seconds since the epoch 
>+   *   (midnight UTC on January 1, 1970)
>+   * - If |time| is a Date object, |set(time)| is equivalent to 
>+   *   |set(time.getTime())|.
>+   */
>+  [implicit_jscontext] void set(in jsval time);
>+};

/Milliseconds/ since the epoch, right?  :)

I don't like this for an internal interface, because it's hard to call from C++
-- I have to create a JSContext, then a JSVal, etc.

We could leave this how it is and add a comment saying something like "If you
want to set the system time from C++, don't bother going through this
interface; just call hal::AdjustSystemClock."

Alternatively, you could make this method take (in uint64_t time), where |time|
is interpreted as milliseconds since the epoch.  The caller in Navigator would
do the conversion just as it does now, and then your caller in the Ril code
would call getTime() on its Date object, if appropriate.

I kind of like the second approach better, as it gives us freedom to take
action inside nsITimeService before we actually set the time.  It also would
let us move this "set the timezone" code we've been talking about elsewhere
onto this interface.

If you take the first approach (which is also fine by me), please add a comment
to the set() method indicating that it's exactly like navigator.setTime().

>diff --git a/dom/time/TimeService.h b/dom/time/TimeService.h

>+class TimeService : public nsITimeService

Please add a brief comment above this class declaration, like "This class
implements a service which lets you modify the system time."

r=me because either of the two alternatives above is a trivial change.
Attachment #661712 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 662069 [details] [diff] [review]
Part 2, Hook up NITZ to System Time

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1077,5 @@
> +  handleNitzTime: function handleNitzTime(message) {
> +    debug("NITZ:" +
> +          " networkTime=" + message.networkTimeInMS +
> +          " networkTimeZone=" + message.networkTimeZoneInMinutes +
> +          " dstFlag=" + message.dstFlag +

When DEBUG is enabled, we already log all post messages in JSON format. So I think this debug() call here is superfluous. Please remove.

@@ +1093,5 @@
> +      let absTimeZoneInHours =
> +        Math.floor(Math.abs(message.networkTimeZoneInMinutes) / 60);
> +      let timeZoneStr = "UTC" +
> +        (message.networkTimeZoneInMinutes >= 0 ? "+" : "-") +
> +        (absTimeZoneInHours < 10 ? "0" : "") + absTimeZoneInHours.toString();

A little easier to read:

  let timeZoneStr = "UTC";
  timeZoneStr += message.networkTimeZoneInMinutes >= 0 ? "+" : "-")
  timeZoneStr += ("0" + absTimeZoneInHours).slice(-2);

But what about timezones that don't use full hour offsets? Do we not support these? Tentatively r+, but let's answer this question before landing.
Attachment #662069 - Flags: review?(philipp) → review+
(In reply to Justin Lebar [:jlebar] from comment #15)
> /Milliseconds/ since the epoch, right?  :)

Ha yes! Actually, I've already had that done in my local patch. ;)

> Alternatively, you could make this method take (in uint64_t time), where
> |time|
> is interpreted as milliseconds since the epoch.  The caller in Navigator
> would
> do the conversion just as it does now, and then your caller in the Ril code
> would call getTime() on its Date object, if appropriate.

Thanks Justin for the reviews and the good points! :) I'll take approach #2 which also sounds comfortable to me. Please feel free to let me know if anything come to your mind before landing.
(In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Comment on attachment 662069 [details] [diff] [review]
> Part 2, Hook up NITZ to System Time
> 
> Review of attachment 662069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> But what about timezones that don't use full hour offsets? Do we not support
> these? Tentatively r+, but let's answer this question before landing.

Hi :philikon! Thanks for the reviews! So far, I assume the timezone is always interpreted by hour offsets because:

1. All the timezone representations around the world are all hour offsets: http://en.wikipedia.org/wiki/List_of_tz_database_time_zones. I cannot find any existing country is using non-hour offsets.

2. The system can only accept the setting values like "America/Chicago" or "UTC-05". So even if the NITZ timezone can be precise to minute (is that really useful?), I still need to convert it to an hour-offset value anyway. I used Math.floor() to ensure we won't get unexpected timezone values.

Btw, I just had some concerns, hoping to discuss with you. :)

1. Do we need to declare a setting value like |time.automatic-update|? The system only needs to sync with NITZ when this value is true. I'm asking why is supposing users would like to use the timezone manually set by themselves instead of NITZ.

2. I still kind of worry about the DST flags. Has DST really been included in the |message.networkTimeZoneInMinutes|? I had difficulties checking that because Asians don't use DST. Is that possible for you to test that if you're in a DST area now (before 9/30)? Or any useful spec document can explicitly tell us about that?

I won't summit anything before everything is confirmed to be safe. :)
> I cannot find any existing country is using non-hour offsets.

I opened that page, did ctrl+f, and typed ":3".

> VE 	+1030-06656 	America/Caracas 		−04:30 	−04:30
> CA 	+4734-05243 	America/St Johns 	Newfoundland Time, including SE Labrador 	−03:30 	−02:30 	
>  	Asia/Calcutta 		+05:30 	+05:30 	Link to Asia/Kolkata

and a bunch more.
Note there are even some :45's and more bizarre stuff.

http://en.wikipedia.org/wiki/UTC%2B13:45

See the list of time zones at the bottom of that page.
Thanks Justin a lot for pointing this out! Ah... I'm over-confident on my experience.

Sorry, :philikon! It's my bad to conclude on that. I'll try to work out how to deal with those bizarre timezone. Still need to confirm with you about that 2 questions at comment #18.
(In reply to Gene Lian [:gene] from comment #18)
> (In reply to Philipp von Weitershausen [:philikon] from comment #16)
> > Comment on attachment 662069 [details] [diff] [review]
> > Part 2, Hook up NITZ to System Time
> > 
> > Review of attachment 662069 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > But what about timezones that don't use full hour offsets? Do we not support
> > these? Tentatively r+, but let's answer this question before landing.
> 
> Hi :philikon! Thanks for the reviews! So far, I assume the timezone is
> always interpreted by hour offsets because:
> 
> 1. All the timezone representations around the world are all hour offsets:
> http://en.wikipedia.org/wiki/List_of_tz_database_time_zones. I cannot find
> any existing country is using non-hour offsets.

Response to myself: thanks to Justin's discovers, we have areas using timezones with non-hour-offset.

> 
> 2. The system can only accept the setting values like "America/Chicago" or
> "UTC-05". So even if the NITZ timezone can be precise to minute (is that
> really useful?), I still need to convert it to an hour-offset value anyway.
> I used Math.floor() to ensure we won't get unexpected timezone values.

This is also wrong. After more detailed experiments, the linux kernel can even accept something like "UTC-05:30".
(In reply to Gene Lian [:gene] from comment #18)
> (In reply to Philipp von Weitershausen [:philikon] from comment #16)
> Btw, I just had some concerns, hoping to discuss with you. :)
> 
> 1. Do we need to declare a setting value like |time.automatic-update|? The
> system only needs to sync with NITZ when this value is true. I'm asking why
> is supposing users would like to use the timezone manually set by themselves
> instead of NITZ.
> 
> 2. I still kind of worry about the DST flags. Has DST really been included
> in the |message.networkTimeZoneInMinutes|? I had difficulties checking that
> because Asians don't use DST. Is that possible for you to test that if
> you're in a DST area now (before 9/30)? Or any useful spec document can
> explicitly tell us about that?

Respond to this concern #2 myself: as described at http://androidxref.com/4.0.4/xref/frameworks/base/telephony/java/com/android/internal/telephony/cdma/CdmaServiceStateTracker.java#1276, the DST correction has been applied in the time zone obviously.
Hi Justin,

Although you've already had r+ on this, I'd like to invite you to take a look on the new patch because I use |int64_t| as our input type instead of |uint64_t| you suggested. I feel more comfortable doing that because JS_Now() also returns the same type. Other similar JS functions like js_DateGetMsecSinceEpoch() or Date.toNumber() also return signed double.

Also, since hal::AdjustSystemClock() can only accept diff time to set the system clock time, I move the diff calculating to the new time service, which should be as closer as we can to the hal::AdjustSystemClock().

Please let me know if it doesn't sound reasonable to you. :)
Attachment #661712 - Attachment is obsolete: true
Attachment #663951 - Flags: review?(justin.lebar+bug)
Hi :philikon,

The new patch handle the following issues:

1. Deal with the non-hour time zone offset. For example, time zone 630 will be "UTC+10:30".

2. Clarify the DST correction, which has already been applied in the time zone, so don't worry about the DST effect.

Still need your decision to see if we need to add a setting like |time.auto-update|? The system only needs to sync with NITZ when this value is true. I'm asking why is supposing users would prefer manually setting the timezone by themselves instead of NITZ. Or just firing another follow-up to address that and checking in the current part?

Thanks very much for your review and sorry again for my previous wrong conclusion about the hour offsets! :)
Attachment #662069 - Attachment is obsolete: true
Attachment #663956 - Flags: review?(philipp)
>+[scriptable, builtinclass, uuid(1fc7fde2-0090-11e2-bdd6-0fea4b9f41f8)]
>+interface nsITimeService : nsISupports
>+{
>+  /* Set the system time.
>+   *
>+   * The |aTimeInMS| argument is the time in milliseconds we want to set.

s/milliseconds/milliseconds since the epoch/.

>+  void set(in int64_t aTimeInMS);

int64_t is fine here, thanks for pointing it out.

>+};

>diff --git a/dom/time/TimeService.h b/dom/time/TimeService.h
>+class TimeService : public nsITimeService

Per the final part of comment 15:

> Please add a brief comment above this class declaration, like "This class
> implements a service which lets you modify the system time."

>diff --git a/dom/time/TimeService.cpp b/dom/time/TimeService.cpp
>+/* static */ already_AddRefed<nsITimeService>
>+TimeService::GetInstance()

It's a nit, but I'd prefer if this returned already_AddRefed<nsTimeService>.
In general, we should prefer concrete classes where possible.

>+nsresult
>+TimeService::Set(int64_t aTimeInMS) {
>+  hal::AdjustSystemClock(
>+    static_cast<int32_t>(aTimeInMS - (JS_Now() / PR_USEC_PER_MSEC)));
>+  return NS_OK;
>+}

This is correct for now, but see bug 794127; this really should be an int64_t!
Do you want to fix that bug first?
Attachment #663951 - Flags: review?(justin.lebar+bug) → review+
(In reply to Gene Lian [:gene] from comment #18)
> (In reply to Philipp von Weitershausen [:philikon] from comment #16)
> > Comment on attachment 662069 [details] [diff] [review]
> > Part 2, Hook up NITZ to System Time
> > 
> > Review of attachment 662069 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > But what about timezones that don't use full hour offsets? Do we not support
> > these? Tentatively r+, but let's answer this question before landing.
> 
> Hi :philikon! Thanks for the reviews! So far, I assume the timezone is
> always interpreted by hour offsets because:
> 
> 1. All the timezone representations around the world are all hour offsets:
> http://en.wikipedia.org/wiki/List_of_tz_database_time_zones. I cannot find
> any existing country is using non-hour offsets.

I can see enough offsets ending in :30, :15, :45 in that list.

> 2. The system can only accept the setting values like "America/Chicago" or
> "UTC-05". So even if the NITZ timezone can be precise to minute (is that
> really useful?), I still need to convert it to an hour-offset value anyway.
> I used Math.floor() to ensure we won't get unexpected timezone values.

You (or better Gaia) need to convert it to the value that the name stands for, yes. But as I pointed out above, that value doesn't have to be a full hour.

> 1. Do we need to declare a setting value like |time.automatic-update|? The
> system only needs to sync with NITZ when this value is true. I'm asking why
> is supposing users would like to use the timezone manually set by themselves
> instead of NITZ.

That sounds good to me.

> 2. I still kind of worry about the DST flags. Has DST really been included
> in the |message.networkTimeZoneInMinutes|? I had difficulties checking that
> because Asians don't use DST. Is that possible for you to test that if
> you're in a DST area now (before 9/30)? Or any useful spec document can
> explicitly tell us about that?

I will try to confirm this.
Attachment #663956 - Flags: review?(philipp) → review+
Depends on: 794127
(In reply to Justin Lebar [:jlebar] from comment #26)
> >+[scriptable, builtinclass, uuid(1fc7fde2-0090-11e2-bdd6-0fea4b9f41f8)]
> >+interface nsITimeService : nsISupports
> >+{
> >+  /* Set the system time.
> >+   *
> >+   * The |aTimeInMS| argument is the time in milliseconds we want to set.
> 
> s/milliseconds/milliseconds since the epoch/.
> 
> >+  void set(in int64_t aTimeInMS);
> 
> int64_t is fine here, thanks for pointing it out.
> 
> >+};
> 
> >diff --git a/dom/time/TimeService.h b/dom/time/TimeService.h
> >+class TimeService : public nsITimeService
> 
> Per the final part of comment 15:
> 
> > Please add a brief comment above this class declaration, like "This class
> > implements a service which lets you modify the system time."

Sorry! My bad!

> 
> >diff --git a/dom/time/TimeService.cpp b/dom/time/TimeService.cpp
> >+/* static */ already_AddRefed<nsITimeService>
> >+TimeService::GetInstance()
> 
> It's a nit, but I'd prefer if this returned already_AddRefed<nsTimeService>.
> In general, we should prefer concrete classes where possible.

Confused a bit here. Do you mean removing the |static| for this function-return or just the comment here?

> 
> >+nsresult
> >+TimeService::Set(int64_t aTimeInMS) {
> >+  hal::AdjustSystemClock(
> >+    static_cast<int32_t>(aTimeInMS - (JS_Now() / PR_USEC_PER_MSEC)));
> >+  return NS_OK;
> >+}
> 
> This is correct for now, but see bug 794127; this really should be an
> int64_t!
> Do you want to fix that bug first?

Thanks for addressing this issue. Sounds better to fix that first. :)
> Confused a bit here.

Sorry, I just mean returning the concrete class nsTimeService instead of the interface nsITimeService.  It's not a big deal either way, though.
(In reply to Justin Lebar [:jlebar] from comment #29)
> > Confused a bit here.
> 
> Sorry, I just mean returning the concrete class nsTimeService instead of the
> interface nsITimeService.  It's not a big deal either way, though.

Ha! I missed the "I"! Thanks Justin!

I think whether to use nsTimeService or nsITimeService depends on how we return the GetInstance(). Supposing I use TimeService (actually, not nsTimeService) both in the .h and .cpp, the compiler would complain:

../../dist/include/nsCOMPtr.h: In member function 'already_AddRefed<T>::operator already_AddRefed<U>() [with U = mozilla::dom::time::TimeService, T = nsITimeService]':
/home/gene/mozilla/dom/time/TimeService.cpp:27:   instantiated from here
../../dist/include/nsCOMPtr.h:170: error: invalid conversion from 'nsITimeService*' to 'mozilla::dom::time::TimeService*'

because I use a nsCOMPtr to return that:

  nsCOMPtr<nsITimeService> service(do_QueryInterface(sSingleton));
  return service.forget();

Do you mind keeping my original design if it's not a big deal? Actually, this also follows the already_AddRefed example at https://developer.mozilla.org/en-US/docs/already_AddRefed, though. Please feel free to let me know if you don't like this. :)
r=jlebar

Justin, please let me know if comment #30 doesn't make sense to you. :)
Attachment #663951 - Attachment is obsolete: true
Attachment #664825 - Flags: review+
Hi :philikon again :)

The new patch takes "time.nitz.automatic-update.enabled" setting into consideration. We use NITZ to adjust system time only when that setting is true.

This is a feature bug and needs to be checked in before 9/28 if possible. Thanks again for your review. :)
Attachment #663956 - Attachment is obsolete: true
Attachment #664840 - Flags: review?(philipp)
r=jlebar and provide a virtual destructor to let try server pass.

Justin, please let me know if comment #30 doesn't make sense to you. :)
Attachment #664825 - Attachment is obsolete: true
Attachment #664925 - Flags: review+
r=jlebar and provide a virtual destructor to let try server pass.

Justin, please let me know if comment #30 doesn't make sense to you. :)
Attachment #664925 - Attachment is obsolete: true
Attachment #664929 - Flags: review+
(In reply to Gene Lian [:gene] from comment #32)
> Created attachment 664840 [details] [diff] [review]
> Part 2, Hook up NITZ to System Time, V3
> 
> Hi :philikon again :)
> 
> The new patch takes "time.nitz.automatic-update.enabled" setting into
> consideration. We use NITZ to adjust system time only when that setting is
> true.

Hi :philikon,

Btw, you might be wondering why not just using the existing handle()/handleError() for nsISettingsServiceCallback. That's because the NITZ message cannot be passed into them in the current .get() mechanism. Therefore, I feel more comfortable handling them within handleNitzTime(), which is inspired by http://mxr.mozilla.org/mozilla-central/source/dom/wifi/WifiWorker.js#1738.
> because I use a nsCOMPtr to return that:
>
>  nsCOMPtr<nsITimeService> service(do_QueryInterface(sSingleton));
>  return service.forget();

Right, that won't work because service.forget() is an already_AddRefed<nsITimeService>, but you're trying to return an already_AddRefed<TimeService>, and there's no implicit conversion from the first to the second.

sSingleton is StaticRefPtr<TimeService>, so to make this work, you'd do

  nsRefPtr<TimeService> service = sSingleton;
  return service.forget();
Comment on attachment 664840 [details] [diff] [review]
Part 2, Hook up NITZ to System Time, V3

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1141,5 @@
> +   * Handle the NITZ message.
> +   */
> +  handleNitzTime: function handleNitzTime(message) {
> +    // Read the 'time.nitz.automatic-update.enabled' setting to see if
> +    // we need to adjust the system clock time and time zone by NITZ.

This is unnecessary. RadioInterfaceLayer already observes settings changes, so it can just update a boolean attribute whenever the  'time.nitz.automatic-update.enabled' changes. Just add another `case` to RadioInterfaceLayer::handle().

Then this whole method can collapse to

  if (!this.nitzAutomaticUpdateEnabled) {
    return;
  }
  this.setTimeByNitz(message);

at which point you can even inline `setTimeByNitz()`.

@@ +1151,5 @@
> +          return;
> +        // This could happen during the very first boot-up where the
> +        // 'time.nitz.automatic-update.enabled' is not yet decided.
> +        if (aResult === null) {
> +          aResult = true;

This is not necessary. As long as we handle a missing setting gracefully, there's no need to do this. Let's add a Gaia default setting instead.
Attachment #664840 - Flags: review?(philipp)
Thanks Philikon! Sounds good to me :) May I ask when will you go off-line? I should be able to come back with the new patch you suggested in 20 minutes.
Hi Philikon! Thanks for the review again :) Hoping I can get your review+ today if possible.
Attachment #664840 - Attachment is obsolete: true
Attachment #665736 - Flags: review?(philipp)
Comment on attachment 665736 [details] [diff] [review]
Part 2, Hook up NITZ to System Time, V4

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

Nice!
Attachment #665736 - Flags: review?(philipp) → review+
Address comment 36 and rebase the patch. Thanks Justing for the suggestion! Good to learn that. :)

r=jlebar.
Attachment #664929 - Attachment is obsolete: true
Attachment #665793 - Flags: review+
Rebase the patch and r=philikon.

Thanks Philikon for the review! :)
Attachment #665736 - Attachment is obsolete: true
Attachment #665794 - Flags: review+
Sorry, https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=52be204da1cf had to be backed out and this push conflicted with it, so had to come out too.

Happy for this to reland once you rebase :-)

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9d251fdb2e
https://hg.mozilla.org/mozilla-central/rev/99bf6b597e44
https://hg.mozilla.org/mozilla-central/rev/dacfa6d8c92e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
I have to ask.  Did these patches really need to be run (multiple times!) on try with "-p all -t all"?  They look pretty b2g-specific, and it's not clear they need Talos runs...
Hi Boris! Someone used to tell me I need to run -all in anyway no matter if we're really sure it's related a specific test or not. I think I can understand your concern and I'll try to narrow down the scope to avoid occupying too many try-server resources. Sorry for any inconvenience!
> Someone used to tell me I need to run -all in anyway no matter if we're really sure it's related a 
> specific test or not.

Unfortunately we need -p all to get b2g builds.  But there are no tests for those builds.

So in general, two try pushes, with "-b do -p all -u none" and "-b d -p linux64 -u all" would probably be sufficient for most b2g changes.  Maybe we could have gotten away without the unit tests in this bug, since this code doesn't run on non-b2g platforms (although the C++ /is/ compiled).

Gene, the advice you received may have been based on consensus when it was given, but within the past month or two we've hit a hard wall in terms of available resources for testing, so we're trying to conserve where we can.
Thanks Justin for the suggestions. Really useful! I'll try to pass the info to let other B2G engineers know in our office.
Depends on: 801096

Comment 52

6 years ago
Dear Gene,
I have two questions about the timezone from NITZ:
1. We get the a string like "utc-08:00" for time.timezone in RadioInterfaceLayer.js, but gecko/dom/system/gonk/TimeZoneSettingObserver.cpp didn't recognize the "utc-08:00" format. You can check the cpp file.
2. Why change networkTimeZoneInMinutes to -(tz * 15)?? I found that I get utc-08:00 from china mobile's nitz message which should be utc+08:00

Thanks very much.
Flags: needinfo?(gene.lian)
Sorry for the delayed response. Just came from a week-long vacation.

(In reply to xiaokang.chen from comment #52)
> Dear Gene,
> I have two questions about the timezone from NITZ:
> 1. We get the a string like "utc-08:00" for time.timezone in
> RadioInterfaceLayer.js, but
> gecko/dom/system/gonk/TimeZoneSettingObserver.cpp didn't recognize the
> "utc-08:00" format. You can check the cpp file.

Actually, it can. The Android Kernel (please see the implementation of hal::SetTimezone(...)) can accept either the following 2 formats:

  - "region/city" (set by Gaia)
  - "UTC{+,-}hh:mm" (set by Gecko's NITZ)

> 2. Why change networkTimeZoneInMinutes to -(tz * 15)?? I found that I get
> utc-08:00 from china mobile's nitz message which should be utc+08:00

Nice catch. In general, we use "UTC+hh:mm" for the time zone on the eastern side (like China) of International Time Line instead of "UTC-hh:mm" [1]. However, the Android Kernel has to set the time zone with an opposite sign (I don't know why).

I think we should make the |time.timezone| setting save the correct sign and flip the sign in TimeZoneSettingObserver. Would you like to take this bug for that?

[1] http://en.wikipedia.org/wiki/Time_zone#Offsets_from_UTC
Flags: needinfo?(gene.lian)
(In reply to Gene Lian [:gene] (Summit + PTOs 10/3 ~ 10/14) from comment #53)
> I think we should make the |time.timezone| setting save the correct sign and
> flip the sign in TimeZoneSettingObserver. Would you like to take this bug
> for that?

Fire Bug 926777.

Comment 55

6 years ago
Dear Gene, 
   we find that when we set time.timezone in UTC format, the tzset can't find the time offset;
   all UTC  return -28800 which is for America/Los_Angeles;
   I want to know why all utc return -28800;
   Is there any web to introduce the details for transform the two format;
What the UTC format do you set for |time.timezone|? The correct value you should input is supposed to be "UTC-08:00" for now. After bug 926777 lands, it's supposed to be "UTC+08:00".

Comment 57

6 years ago
the UTC  "UTC+08:00" is set for time.timezone, but the tzset can't find the UTC+08:00, return the -28800 offset;

Comment 58

6 years ago
we find the persist.sys.timezone replaced the TZ, our UTC+08:00 is set to persist.sys.timzone,  the tzset will get the offset accordind to the timezone name, but it can't find the "UTC+08:00" and all UTC;
Now, it can't find the offset in the timezoneinfo.dat, timezoneinfo.idx;
It alse can't find the offset in posix

Comment 59

6 years ago
the log
tzload: path : UTC-08:00
tzload: full path : /system/usr/share/zoneinfo/UTC-08:00
tzload: could not open '/system/usr/share/zoneinfo/UTC-08:00', trying '/system/usr/share/zoneinfo/zoneinfo.idx'
tzload: invalid offset (-1)
tzload: path : posixrules
tzload: full path : /system/usr/share/zoneinfo/posixrules
tzload: could not open '/system/usr/share/zoneinfo/posixrules', trying '/system/usr/share/zoneinfo/zoneinfo.idx'
tzload: invalid offset (-1)
tzload: stdoffset (-28800)

Comment 60

6 years ago
i don't know if the UTC format can't be recognized will affect the systemtime;
Skao is helping look into this but cannot reproduce this by master (the system time on the device can be correctly updated by |time.timezone|). Will test it on V1.1 later.

Is Bug 926806 addressing exactly the same issue? We had better move the discussions and tests to there. This one has been closed.
You need to log in before you can comment on or make changes to this bug.