hal::RegisterSystemTimeChangeObserver does not register the observer through sandbox.

Hi Mounir, Would you help me review this patch? Or suggest someone review it? Thanks.

delete unused log

Steven, could you point me to the original bug? (I guess this is some kind of follow-up)

Hi Mounir, Sorry for not giving the reference bug. Here it is, Bug 714358, the observer implementation in hal.

I don't really know the context of that patch so it's hard to do a quick review. The code looks good but I will have to understand what was wrong and how that codes fixes it. Given that Justin reviewed the original code, I'm CCing him in case of he wants to still that review from me. In the mean time, Steven, feel free to give me a bit of context, it will make the review simpler ;)

(In reply to Mounir Lamouri (:mounir) from comment #5) > I don't really know the context of that patch so it's hard to do a quick > review. The code looks good but I will have to understand what was wrong and > how that codes fixes it. Given that Justin reviewed the original code, I'm > CCing him in case of he wants to still that review from me. In the mean > time, Steven, feel free to give me a bit of context, it will make the review > simpler ;) When a child process calls hal::RegisterSystemTimeChangeObserver, we should pass this call to parent process to register the child listener in parent process's list. Then the time is changed, parent process can callback to child process. If not, in OOP the child process cannot receive the callback when time is changed.

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp >--- a/hal/Hal.cpp >+++ b/hal/Hal.cpp >@@ -417,23 +417,30 @@ InitializeSystemTimeChangeObserver() > } > } > > void > RegisterSystemTimeChangeObserver(SystemTimeObserver *aObserver) > { > AssertMainThread(); > InitializeSystemTimeChangeObserver(); >+ if (InSandbox()) { >+ hal_sandbox::EnableSystemTimeChangeNotifications(); >+ } >+ > sSystemTimeObservers->AddObserver(aObserver); > } > > void > UnregisterSystemTimeChangeObserver(SystemTimeObserver *aObserver) > { > AssertMainThread(); >+ if (InSandbox()) { >+ hal_sandbox::DisableSystemTimeChangeNotifications(); >+ } > sSystemTimeObservers->RemoveObserver(aObserver); > } I think this code should be written like BatteryObserversManager. With this patch, we will call EnableSystemTimeChangeNotifications() once for every time change observer we add. But we only want to call it once. >diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp > class HalParent : public PHalParent > , public BatteryObserver > , public NetworkObserver > , public ISensorObserver > , public WakeLockObserver > , public ScreenConfigurationObserver > , public SwitchObserver >+ , public SystemTimeObserver > { > public: > virtual void > ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE > { > // NB: you *must* unconditionally unregister your observer here, > // if it *may* be registered below. > hal::UnregisterBatteryObserver(this); >@@ -560,16 +573,30 @@ public: > if (!AppProcessHasPermission(this, "time")) { > return false; > } > *aTimezoneSpec = hal::GetTimezone(); > return true; > } > > virtual bool >+ RecvEnableSystemTimeChangeNotifications() MOZ_OVERRIDE >+ { >+ hal::RegisterSystemTimeChangeObserver(this); >+ return true; >+ } >+ >+ virtual bool >+ RecvDisableSystemTimeChangeNotifications() MOZ_OVERRIDE >+ { >+ hal::UnregisterSystemTimeChangeObserver(this); >+ return true; >+ } It doesn't look like you /do/ anything with the SystemTimeObserver. What happens when HalParent receives a notification that the system time has changed?

(In reply to Justin Lebar [:jlebar] from comment #7) > I think this code should be written like BatteryObserversManager. With this > patch, we will call EnableSystemTimeChangeNotifications() once for every time > change observer we add. But we only want to call it once. Yes, I will change it in the next version. > > >+ RecvEnableSystemTimeChangeNotifications() MOZ_OVERRIDE > >+ { > >+ hal::RegisterSystemTimeChangeObserver(this); > >+ return true; > >+ } > >+ > >+ virtual bool > >+ RecvDisableSystemTimeChangeNotifications() MOZ_OVERRIDE > >+ { > >+ hal::UnregisterSystemTimeChangeObserver(this); > >+ return true; > >+ } > > It doesn't look like you /do/ anything with the SystemTimeObserver. What > happens when HalParent receives a notification that the system time has > changed? The patch is to register the child listener to parent process. Without doing this, when HalParent gets a system time change notifications, HalParent will not callback to child listener. Because parent does not know the listeners of child processes.

> Without doing > this, when HalParent gets a system time change notifications, HalParent will > not callback to child listener. Because parent does not know the listeners of > child processes. I think I understand you, and if I do, that's all correct. But the question is, what happens when HalParent::Notify(const SystemTimeChange&) (from SystemTimeObserver) is called? In fact, does this even compile without that method being defined?

I am not sure if the answer is what you want. (In reply to Justin Lebar [:jlebar] from comment #9) > I think I understand you, and if I do, that's all correct. > > But the question is, what happens when HalParent::Notify(const > SystemTimeChange&) (from SystemTimeObserver) is called? In fact, does this When HalParent::Notify() is called, it sends the message to child process and HalChild gets this message through RecvNotifySystemTimeChange then sends to its listeners. > even compile without that method being defined? Do you mean HalParent::Notify()? HalParent must have this function since it inherits from SystemTimeObserver in the patch. Because SystemTimeObserver instantiates ObserverList template and ObserverList has pure virtual function, Notify(). In original version, if we delete HalParent::Notify(), compiler does not complain because it does not inherit from SystemTimeObserver.

I'm still confused... > class HalParent : [...] > + , public SystemTimeObserver But then in attachment 662956 [details] [diff] [review], HalParent doesn't have a Notify(const SystemTimeChange&) method. So since we agree that Notify(const SystemTimeChange&) is pure virtual in SystemTimeObserver, how does this work? > When HalParent::Notify() is called [...] But that method doesn't exist in the patch. Maybe you forgot to qrefresh before attaching the patch?

(In reply to Justin Lebar [:jlebar] from comment #11) > But then in attachment 662956 [details] [diff] [review], HalParent doesn't > have a Notify(const SystemTimeChange&) method. So since we agree that > Notify(const SystemTimeChange&) is pure virtual in SystemTimeObserver, how > does this work? > > > When HalParent::Notify() is called [...] > > But that method doesn't exist in the patch. > > Maybe you forgot to qrefresh before attaching the patch? Ah, I got it! Notify(const SystemTimeChange&) is implemented in attachment 714358 [details] [diff] [review] so that I don't need to implement it here. And that makes you confused. Sorry for that I should give more detail information.

Ah, I see! What's confusing is, HalParent::Notify(SystemTimeChange&) is there, but HalParent does not currently inherit from SystemTimeObserver! So nobody is currently calling that method, right? Sorry, I should have looked through the code more carefully; that's not your fault.

Will look at this again once we're using the BatteryObserversManager-style code. Sorry again for the confusion with this interface; I did not expect to see that method sitting around there!

Comment on attachment 663334 [details] [diff] [review] patch V2 Perfect, except we also need an (empty) implementation of Enable/DisableSystemTimeChangeNotifications as a fallback for other systems, right? r=me, but please push a fixed version to try (-b d -p all -u mochitest-1 would probably be sufficient) so we an ensure that we got the fallback build bits correct. I can check this in once the try results come back.

Justin, here is the try log, please check it. Thanks. https://tbpl.mozilla.org/?tree=Try&rev=6c984104e48e

Looks great: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a8ade9e7af8