Time API: changes does not persist after a restart

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: vingtetun, Assigned: vchang)

Tracking

Trunk
mozilla18
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.

Updated

6 years ago
Assignee: nobody → vchang
(Assignee)

Comment 1

6 years ago
Created attachment 663974 [details] [diff] [review]
Patch, set to alarm rtc

Modified AdjustSystemClock(), set time using alarm driver.
Attachment #663974 - Flags: review?(jones.chris.g)

Comment 2

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

Comment 4

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

Comment 6

6 years ago
Created attachment 665277 [details] [diff] [review]
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.
Attachment #663974 - Attachment is obsolete: true
Attachment #665277 - Flags: review?(mwu)

Comment 7

6 years ago
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"
Attachment #665277 - Flags: review?(mwu) → review+
(Assignee)

Comment 8

6 years ago
Created attachment 665802 [details] [diff] [review]
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.
Attachment #665277 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ab627c99dd

Should this have a test?
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c0ab627c99dd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
> 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.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.