777200 - SettingsChangeNotifier should only notify processes with settings-change listeners

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

Currently they're broadcast to all processes, if I'm reading things correctly.  We should only allow processes with settings-read permissions to listen to settings changes.

Christoph/Gregor, one of you guys want to take this?

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

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

This doesn't completely address the problem here, but Gregor says another patch is on the way.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 645811 [details] [diff] [review]
patch

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

::: dom/settings/SettingsChangeNotifier.jsm
@@ +38,5 @@
>  
>    receiveMessage: function(aMessage) {
>      debug("receiveMessage");
>      let msg = aMessage.json;
> +    let mm = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);

The QI is no longer needed (and nsIFrameMessageManager doesn't exist anymore anyway.)

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

This versions makes some simplifications:

We have 2 notification systems for setting changes:
onsettingschange is called whenever a setting changes
addObserver can be used to listen to a single setting change.

It gets complicated to track all additions and removal of listeners so once somebody adds an observer, the process always gets notified.
I think the use-case of removing observers is pretty rare and we can ignore this.

If somebody adds an observer for a single setting, we also notify the process if another setting changed. We might want to change this if it becomes a problem.

Comment 5

[Gregor Wagner [:gwagner]]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=7cd75849be4d

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Gregor Wagner [:gwagner] from comment #4)
> It gets complicated to track all additions and removal of listeners so once
> somebody adds an observer, the process always gets notified.
> I think the use-case of removing observers is pretty rare and we can ignore
> this.

Won't you have to deal with child processes dying (as in bug 791910)? Also, isn't it a little weird to send messages twice to a manager that happens to have two observers?

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to Blake Kaplan (:mrbkap) from comment #6)
> (In reply to Gregor Wagner [:gwagner] from comment #4)
> > It gets complicated to track all additions and removal of listeners so once
> > somebody adds an observer, the process always gets notified.
> > I think the use-case of removing observers is pretty rare and we can ignore
> > this.
> 
> Won't you have to deal with child processes dying (as in bug 791910)?

Thats bug 777508.

 Also,
> isn't it a little weird to send messages twice to a manager that happens to
> have two observers?

Right we don't want this. I thought I have a test for it but let me check again.

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: patch

Fixed the multiple register problem.

Comment 9

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24e6cf3628ae

I also fixed the issue from bug 792846.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/24e6cf3628ae

Comment 11

[Philipp von Weitershausen [:philikon]]

This was landed before bug 777508 (and obviously doesn't make use of that notification), so I'm removing the dependency here.