Last Comment Bug 808171 - setting enabled results in double notifications
: setting enabled results in double notifications
Status: VERIFIED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: 18 Branch
: All All
: -- normal (vote)
: Firefox 19
Assigned To: Mark Hammond [:markh]
:
: Shane Caraveo (:mixedpuppy)
Mentors:
Depends on:
Blocks: 807217
  Show dependency treegraph
 
Reported: 2012-11-02 13:38 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2012-12-04 14:53 PST (History)
4 users (show)
markh: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
setEnabled fix (1.06 KB, patch)
2012-11-02 13:49 PDT, Shane Caraveo (:mixedpuppy)
gavin.sharp: review+
Details | Diff | Splinter Review
Alternative that has the pref observer check if the state really needs to change. (2.00 KB, patch)
2012-11-06 04:49 PST, Mark Hammond [:markh]
felipc: review+
gavin.sharp: feedback+
Details | Diff | Splinter Review
v3 (2.82 KB, patch)
2012-11-06 15:39 PST, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
Rollup patch for aurora/beta (1.23 KB, patch)
2012-11-06 16:53 PST, :Felipe Gomes (needinfo me!)
felipc: review+
gavin.sharp: approval‑mozilla‑aurora+
gavin.sharp: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-11-02 13:38:44 PDT
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) 2012-11-02 13:49:25 PDT
Created attachment 677874 [details] [diff] [review]
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) 2012-11-02 13:51:03 PDT
https://tbpl.mozilla.org/?tree=Try&rev=fdc19c9d9061
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-11-02 14:01:17 PDT
Could also add a simple test that toggling notifications are only sent once etc.
Comment 4 Shane Caraveo (:mixedpuppy) 2012-11-02 16:15:07 PDT
new try at https://tbpl.mozilla.org/?tree=Try&rev=b0e8cda8eb82
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-11-03 06:21:00 PDT
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] 2012-11-06 00:32:10 PST
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] 2012-11-06 04:49:53 PST
Created attachment 678702 [details] [diff] [review]
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: gavin@gavinsharp.com] 2012-11-06 08:53:51 PST
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: gavin@gavinsharp.com] 2012-11-06 09:02:35 PST
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) 2012-11-06 13:19:45 PST
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) 2012-11-06 15:39:20 PST
Created attachment 678954 [details] [diff] [review]
v3
Comment 12 Mark Hammond [:markh] 2012-11-06 15:54:13 PST
Comment on attachment 678954 [details] [diff] [review]
v3

The _initialized change is too risky for beta - going with the original.
Comment 14 :Felipe Gomes (needinfo me!) 2012-11-06 16:48:19 PST
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 me!) 2012-11-06 16:53:28 PST
Created attachment 678986 [details] [diff] [review]
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 me!) 2012-11-06 18:59:08 PST
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 me!) 2012-11-06 19:33:40 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/d275b56629b1
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 14:41:27 PST
Does this have or need tests?
Comment 20 Mark Hammond [:markh] 2012-12-04 14:53:36 PST
No tests, but the problems it addresses aren't really user-facing.  It's been working fine so I'll verify.

Note You need to log in before you can comment on or make changes to this bug.