Closed Bug 792443 Opened 7 years ago Closed 7 years ago

hal::RegisterSystemTimeChangeObserver should register the observer through sandbox

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: slee, Assigned: slee)

References

Details

Attachments

(1 file, 3 obsolete files)

hal::RegisterSystemTimeChangeObserver does not register the observer through 
sandbox.
Attached patch patch (obsolete) — Splinter Review
Hi Mounir,

Would you help me review this patch? Or suggest someone review it?

Thanks.
Assignee: nobody → slee
Attached patch patch (obsolete) — Splinter Review
delete unused log
Attachment #662954 - Attachment is obsolete: true
Attachment #662956 - Flags: review?(mounir)
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.
Blocks: 714358
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 ;)
Version: unspecified → Trunk
(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.
Attachment #662956 - Flags: review?(mounir) → review?(justin.lebar+bug)
>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?
Attachment #662956 - Flags: review?(justin.lebar+bug) → review-
(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!
Attached patch patch V2 (obsolete) — Splinter Review
Attachment #662956 - Attachment is obsolete: true
Attachment #663334 - Flags: review?(justin.lebar+bug)
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.
Attachment #663334 - Flags: review?(justin.lebar+bug) → review+
Attached patch patch V3Splinter Review
Justin, here is the try log, please check it. Thanks.
https://tbpl.mozilla.org/?tree=Try&rev=6c984104e48e
Attachment #663334 - Attachment is obsolete: true
Attachment #663820 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4a8ade9e7af8
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.