808171 - setting enabled results in double notifications

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

In SocialService.enabled we end up calling _setEnabled twice, since we directly call it as well as calling it from a pref observer.  This results in a bunch of double notifications and various other problems which are more visible when disabling the feature.  We will just remove the direct call to _setEnabled and let the pref observer do that.  (per irc with gavin/jaws)

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: setEnabled fix

Found this double-calling of setEnabled while testing bug 807217.  This issue caused the tests in the patch for bug 807217 to fail.

Comment 2

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=fdc19c9d9061

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Could also add a simple test that toggling notifications are only sent once etc.

Comment 4

[Shane Caraveo (:mixedpuppy)]

new try at https://tbpl.mozilla.org/?tree=Try&rev=b0e8cda8eb82

Comment 5

[Ryan VanderMeulen [:RyanVM]]

That Try run is positively failtastic. Please don't request checkin until your run is finished and verified to be OK.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 677874 [details] [diff] [review]
setEnabled fix

Doesn't this patch make us dependent on the order of observers?  eg, IIUC, isn't anyone who adds a pref observer before us going to see the wrong value for Social.enabled?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: Alternative that has the pref observer check if the state really needs to change.

This is an alternative that doesn't have the drawback I mentioned in the previous comment - the pref observer instead just checks if the state needs to be changed.

Note I also swapped the _setEnabled/Services.prefs.setBoolPref calls - by doing the _setEnabled() first, we ensure that all other observers for this pref will see the correct Social.enabled.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 678702 [details] [diff] [review]
Alternative that has the pref observer check if the state really needs to change.

People who want to observe the social service's state changes should probably do so by watching the social:pref-changed topic, but you're right that this is more robust - I like it.

Why not put the check in _setEnabled, though?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Ah, Shane points out that there is such a check, but not in safe mode - I never really understood that code. Seems like that kind of "override" logic should be in the front-end somewhere.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 678702 [details] [diff] [review]
Alternative that has the pref observer check if the state really needs to change.

This is the patch I'm using underneath bug 807217

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: v3

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 678954 [details] [diff] [review]
v3

The _initialized change is too risky for beta - going with the original.

Comment 13

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d534f9a6a3

Comment 14

[:Felipe Gomes (needinfo for replies!)]

I pushed a follow-up to address gavin's comment 8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3bec8c880a7

Comment 15

[:Felipe Gomes (needinfo for replies!)]

Attachment: Rollup patch for aurora/beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Social
User impact if declined: required for bug 807217
Testing completed (on m-c, etc.): landed on inbound
Risk to taking this patch (and alternatives if risky): small, avoids double notifications that would confuse tests and potentially real-world scenarios as well
String or UUID changes made by this patch: none

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

Patch for branches, includes the follow-up

Comment 16

[:Felipe Gomes (needinfo for replies!)]

This was part of bug 807217 but was done in a separate bug. Lukas gave approval on #fx-team to land this for the go-to build of beta 5

https://hg.mozilla.org/releases/mozilla-beta/rev/e449c8cd821a

Comment 17

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d275b56629b1

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e3d534f9a6a3
https://hg.mozilla.org/mozilla-central/rev/e3bec8c880a7

Comment 19

[u279076]

Does this have or need tests?

Comment 20

[Mark Hammond [:markh] [:mhammond]]

No tests, but the problems it addresses aren't really user-facing.  It's been working fine so I'll verify.