Closed Bug 1146900 Opened 5 years ago Closed 4 years ago

[Shinano][Aries] Use timekeep instead of time_daemon

Categories

(Firefox OS Graveyard :: GonkIntegration, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

(Depends on 1 open bug)

Details

(Keywords: foxfood, Whiteboard: [systemsfe][spark])

Attachments

(3 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1110010 +++

As documented in bug 1110010 comment 33, let's drop this blob.
Hm, QC's RIL code depends on libtime_genoff.so:
> 03-24 15:26:14.410  1397  1397 E RILD    : dlopen failed: dlopen failed: could not load library "libtime_genoff.so" needed by "libril-qc-qmi-1.so"; caused by library "libtime_genoff.so" not found
Depends on: 1135716
Attached patch Support timekeep daemon r=... (obsolete) — Splinter Review
> 04-26 00:04:43.470   262   262 E TimeKeep: Will execute: restore
> 03-24 17:36:37.000   262   262 E TimeKeep: Time restored!
> 03-24 17:36:37.990   295   295 I         : Starting bootanimation with (1) format framebuffer
> 03-24 17:36:39.010   295   295 I Gecko   : -*- TimeKeepService: B2G time service enabled
Attached file Shinano common PR
Attachment #8582638 - Flags: review?(mwu)
Attached file Manifest PR
Should we fork this repo to our mozilla-b2g account, also, or is it fine like this?
Attachment #8582639 - Flags: review?(mwu)
Comment on attachment 8582484 [details] [diff] [review]
Support timekeep daemon r=...

So this is implementing something similar to what was done in bug 1110010. I stole the time observer fix from there too.

I'm just wondering whether we should:
 - try to detect the case where there is also qc-time-service present, and let it handle. We cannot rely on libtime_genoff being there since it's needed anyway by libril-qc stuff
 - maybe hide this service behing a build flag that would be set for the platforms that will support it

Also, please note this should work on Flame too, but I have not yet tested it.
Attachment #8582484 - Flags: feedback?(fabrice)
Attachment #8582484 - Flags: feedback?(aosmond)
And FYI the original Java logic provided in the repository is visible at: https://github.com/sonyxperiadev/timekeep/blob/master/src/com/sony/timekeep/TimeKeep.java#L44

We are just doing the same but in Gecko :)
The service needs to be stored outside of the gecko repo since it's a device specific feature/hack. Only the devices that need it should have it installed, so there should be no need to detect whether it needs to be run.
Attachment #8582484 - Attachment is obsolete: true
Attachment #8582484 - Flags: feedback?(fabrice)
Attachment #8582484 - Flags: feedback?(aosmond)
(In reply to Alexandre LISSY :gerard-majax from comment #4)
> Created attachment 8582638 [details] [review]
> Shinano common PR

So this one now includes the component, as suggested in comment 8.
Attached patch Fix time change observers r=... (obsolete) — Splinter Review
Comment on attachment 8582996 [details] [diff] [review]
Fix time change observers r=...

So that's the same as in bug 1110010 for just the time observer changes, stole from Andrew.
Flags: needinfo?(aosmond)
Attachment #8582996 - Flags: review?(fabrice)
Bevis, we noticed that libgenoff_time used to somehow notify the modem. Do you know how we could check that we broke nothing on this side?
Flags: needinfo?(btseng)
(In reply to Alexandre LISSY :gerard-majax from comment #12)
> Bevis, we noticed that libgenoff_time used to somehow notify the modem. Do
> you know how we could check that we broke nothing on this side?

Sorry, I have no idea about this. :(
Flags: needinfo?(btseng)
Comment on attachment 8582638 [details] [review]
Shinano common PR

The manifest is under one license and the xpcom component is under another license. r=me with everything under apache 2.
Attachment #8582638 - Flags: review?(mwu) → review+
Comment on attachment 8582639 [details] [review]
Manifest PR

Going to be a little more work to get a new remote in, but I'm ok with it. Also r=me if you decide to fork this to mozilla-b2g. Up to you.
Attachment #8582639 - Flags: review?(mwu) → review+
(In reply to Michael Wu [:mwu] from comment #15)
> Comment on attachment 8582639 [details] [review]
> Manifest PR
> 
> Going to be a little more work to get a new remote in, but I'm ok with it.
> Also r=me if you decide to fork this to mozilla-b2g. Up to you.

I've forked it so that we do not get breakages because of further changes that could happen in the future.
(In reply to Michael Wu [:mwu] from comment #14)
> Comment on attachment 8582638 [details] [review]
> Shinano common PR
> 
> The manifest is under one license and the xpcom component is under another
> license. r=me with everything under apache 2.

Set the proper license in the XPCOM, thanks :)
Blocks: 1147815
Blocks: spark
Comment on attachment 8582996 [details] [diff] [review]
Fix time change observers r=...

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

redirecting to bent since he was looking at the c++ changes in bug 1110010
Attachment #8582996 - Flags: review?(fabrice) → review?(bent.mozilla)
Comment on attachment 8582996 [details] [diff] [review]
Fix time change observers r=...

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

My only reservation with the patch (being the author :)) was perhaps making this generic would be better; for example, one could register for notifications when observers for a specific topic transition between empty and non-empty, and use the same mechanism for EventListenerManager::AddEventListenerInternal. It was very frustrating to realize observers could miss events due to there being no window event listeners, despite them being separate notification mechanisms.
Attachment #8582996 - Flags: review+
Flags: needinfo?(aosmond)
Whiteboard: [systemsfe]
Blocks: 1110010
Comment on attachment 8582996 [details] [diff] [review]
Fix time change observers r=...

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

I'd love to get this fixed ASAP but this patch is kind of backwards...

We don't want to add any special code to nsObserverService, it's just a generic notification system.

Instead we should just register for time notifications at startup, and whenever we get a notification we should call nsObserverService::NotifyObservers with the "system-clock-change" topic.
Attachment #8582996 - Flags: review?(bent.mozilla) → review-
Thanks Ben, that indeed mostly matches Andrew's comment 20. I'll dig into this and try to fix. Andrew, in the meantime, if you have brain availability to fix it, feel free :)
Flags: needinfo?(aosmond)
The window dependency comes from bug 795328
(In reply to Alexandre LISSY :gerard-majax from comment #23)
> The window dependency comes from bug 795328

Never mind, it was there before.
So, just a couple of stupid questions to make sure I understand correctly. 

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #21)
> Comment on attachment 8582996 [details] [diff] [review]
> Fix time change observers r=...
> 
> Review of attachment 8582996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd love to get this fixed ASAP but this patch is kind of backwards...
> 
> We don't want to add any special code to nsObserverService, it's just a
> generic notification system.

What you dislike is:
 - in nsObserverService we had hard-coded specific handling for "system-clock-change"
 - the matching changes in the TimeChangeObserver that does |return nsSystemTimeChangeObserver::GetInstance()->EnableObserverServiceNotificationsImpl();|

> 
> Instead we should just register for time notifications at startup, and
> whenever we get a notification we should call
> nsObserverService::NotifyObservers with the "system-clock-change" topic.

I'm not sure to get exactly what you mean by "register for time notifications at startup". Are you referring to |RegisterSystemClockChangeObserver(sObserver);| ?

Which startup are we referring about ? TimeChangeObserver, or device ?
Flags: needinfo?(bent.mozilla)
(In reply to Alexandre LISSY :gerard-majax from comment #25)
> What you dislike is:
>  - in nsObserverService we had hard-coded specific handling for
> "system-clock-change"

Yep!

>  - the matching changes in the TimeChangeObserver that does |return
> nsSystemTimeChangeObserver::GetInstance()-
> >EnableObserverServiceNotificationsImpl();|

I don't care about this really, whatever works!

> I'm not sure to get exactly what you mean by "register for time
> notifications at startup".

I'm saying that we need a component that gets instantiated at xpcom-startup that calls registers an always-on listener. That listener then forwards to nsObserverList whenever it needs to. I guess we only need this with MOZ_WIDGET_GONK?
Flags: needinfo?(bent.mozilla)
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #26)
> nsObserverList

Oops, meant nsObserverService.
To ease further understanding, one symptom of the issue is:
> $ adb logcat -v threadtime| grep -i 'hal::'
> 04-09 13:10:22.000   328   328 I Gecko   : hal::NotifySystemClockChange(1416949594700)
> 04-09 13:10:25.160   328   328 I Gecko   : hal::RegisterSystemClockChangeObserver()

This is what is explained in bug 1110010 comment 31: the |RegisterSystemClockChangeObserver| call is only done once there is some window.
Attached patch Fix time change observers, v2 (obsolete) — Splinter Review
Updated as per bent's feedback. I think this is what you meant in combination with: https://bugzilla.mozilla.org/attachment.cgi?id=8590343&action=diff

The QTimeService XPCOM component calls the register/deregister functions of nsITimeService to enable the notifications.
Attachment #8582996 - Attachment is obsolete: true
Flags: needinfo?(aosmond)
Attachment #8590348 - Flags: review?(bent.mozilla)
Minor fix, I was a little too fast on the trigger and left in some code that was used but was replaced.
Attachment #8590348 - Attachment is obsolete: true
Attachment #8590348 - Flags: review?(bent.mozilla)
Attachment #8590351 - Flags: review?(bent.mozilla)
Comment on attachment 8590351 [details] [diff] [review]
Fix time change observers, v2.1

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

This looks mostly fine, but I think we can simplify this a bit:

::: dom/time/TimeService.cpp
@@ +30,5 @@
>  
>  NS_IMETHODIMP
> +TimeService::Register()
> +{
> +  return mozilla::time::AddObserver();

Just call this in the constructor?

@@ +36,5 @@
> +
> +NS_IMETHODIMP
> +TimeService::Deregister()
> +{
> +  return mozilla::time::RemoveObserver();

And call this in the destructor?

::: dom/time/nsITimeService.idl
@@ +15,5 @@
> +  /* Enable system clock change notifications. */
> +  void register();
> +
> +  /* Disable system clock change notifications. */
> +  void deregister();

I don't think these should be needed.

Instead, just add an 'app-startup' entry to the category list here for TimeService: http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#1303

That way it will always be available, no need to manually register/deregister.
Attachment #8590351 - Flags: review?(bent.mozilla) → review-
Priority: -- → P1
Alex, do we want to keep working on this?
Blocks: spark-device
No longer blocks: spark
Flags: needinfo?(lissyx+mozillians)
Whiteboard: [systemsfe] → [systemsfe][spark]
Well according to Andrew we would still need it, but I have not been able to prove any cases where not having it was blocking ; not even as in bug 1110010.

Andrew, do you have some time?
Flags: needinfo?(lissyx+mozillians) → needinfo?(aosmond)
Duplicate of this bug: 1167090
[Extending summary to mention the main symptom here, to increase discoverability for people who are about to file dupes of this]
Summary: [Shinano][Aries] Use timekeep instead of time_daemon → [Shinano][Aries] Use timekeep instead of time_daemon (aka timestamps shown for files over MTP are incorrect)
Duplicate of this bug: 1177181
(In reply to Daniel Holbert [:dholbert] from comment #35)
> [Extending summary to mention the main symptom here, to increase
> discoverability for people who are about to file dupes of this]

I disagree: filesystem dates are good, EXIF infos dates are good. I think it's a MTP-specific issue.
Summary: [Shinano][Aries] Use timekeep instead of time_daemon (aka timestamps shown for files over MTP are incorrect) → [Shinano][Aries] Use timekeep instead of time_daemon
(In reply to Alexandre LISSY :gerard-majax from comment #39)
> I disagree: filesystem dates are good, EXIF infos dates are good. I think
> it's a MTP-specific issue.

(in other words: recent dupes of this bug were actually about a different issue, & were incorrectly duped. That issue is now covered by bug 1167090.)
Flags: needinfo?(aosmond)
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.