793558 - Time API: changes does not persist after a restart

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Experimenting with the first run experience and the navigator.mozTime interface I found out that the new date does not persist after a restart of the device.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch, set to alarm rtc

Modified AdjustSystemClock(), set time using alarm driver.

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 663974 [details] [diff] [review]
Patch, set to alarm rtc

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

::: hal/gonk/GonkHal.cpp
@@ +639,5 @@
>      now.tv_nsec += NsecPerSec;
>      now.tv_sec -= 1;
>    }
> +#ifdef SUPPORT_ANDROID_ALARM_RTC
> +  int fd;

Would be best to just use ScopedClose here.

@@ +642,5 @@
> +#ifdef SUPPORT_ANDROID_ALARM_RTC
> +  int fd;
> +  int res;
> +
> +  errno = 0;

We only check errno if there's an error, so I don't think there's a need to clear it..

@@ +644,5 @@
> +  int res;
> +
> +  errno = 0;
> +  fd = open("/dev/alarm", O_RDWR);
> +  if(fd < 0) {

Space after if.

@@ +650,5 @@
> +    return;
> +  }
> +  errno = 0;
> +  res = ioctl(fd, ANDROID_ALARM_SET_RTC, &now);
> +  if(res < 0) {

We can just inline the ioctl call into the if check.

Comment 3

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

Comment on attachment 663974 [details] [diff] [review]
Patch, set to alarm rtc

We're in a gonk-specific file, so no reason to add the ifdef here.  Let's just change the implementation.

I think we also need to handle EINTR from open() and ioctl() here.

Comment 4

[Vincent Chang[:vchang][changyihsin]]

> I think we also need to handle EINTR from open() and ioctl() here.
Hi Chris, 

Thanks for your comments. I have two questions here.  

How many times do we need to retry when we get EINTR calling open() ? Do we need to retry forever ? Or just one or two times is enough ? 

When I checked the ioctl() with "man 2 ioctl", I didn't find EINTR in ERRORS list. I was wondering if we have to handle EINTR calling ioctl() ?

Comment 5

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

(In reply to Vincent Chang from comment #4)
> > I think we also need to handle EINTR from open() and ioctl() here.
> Hi Chris, 
> 
> Thanks for your comments. I have two questions here.  
> 
> How many times do we need to retry when we get EINTR calling open() ? Do we
> need to retry forever ? Or just one or two times is enough ? 
> 

EINTR means a signal interrupted the call, so you can retry indefinitely.  The usual pattern is

  int fd;
  do {
    fd = open(...);
  } while (fd == -1 && errno == EINTR);

> When I checked the ioctl() with "man 2 ioctl", I didn't find EINTR in ERRORS
> list. I was wondering if we have to handle EINTR calling ioctl() ?

Ah, you're correct.  If the man page says no, then no :).

Comment 6

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch, set system time using alarm rtc driver, v1.1

1. Addressed the change that argument type of AdjustSystemClock(aDeltaMilliseconds) is modified from int32 to int64. 
2. Addressed comments 2,3,5.

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 665277 [details] [diff] [review]
Patch, set system time using alarm rtc driver, v1.1

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

r=me with the log messages changed.

::: hal/gonk/GonkHal.cpp
@@ +632,5 @@
> +    fd = open("/dev/alarm", O_RDWR);
> +  } while (fd == -1 && errno == EINTR);
> +  ScopedClose autoClose(fd);
> +  if (fd < 0) {
> +    HAL_LOG(("Unable to open alarm driver: %s\n", strerror(errno)));

Let's not copy the android message if possible.

"Failed to open /dev/alarm: %s"

@@ +637,5 @@
> +    return;
> +  }
> +
> +  if (ioctl(fd, ANDROID_ALARM_SET_RTC, &now) < 0) {
> +    HAL_LOG(("Unable to set rtc to %ld: %s\n", now.tv_sec, strerror(errno)));

"ANDROID_ALARM_SET_RTC failed: %s"

Comment 8

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch, set system time using alarm rtc driver, v1.2

Hi Michael, 

Thanks for reviewing this patch. I updated the patch to address your comments.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ab627c99dd

Should this have a test?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0ab627c99dd

Comment 11

[Justin Lebar (not reading bugmail)]

> Should this have a test?

The bug involves restarting the device?

Also, it's a B2G-specific bug and we currently do not have any unit tests for B2G on Tinderbox.