setting enabled results in double notifications

VERIFIED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: markh)

Tracking

18 Branch
Firefox 19
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox17 fixed, firefox18 fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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)
(Reporter)

Comment 1

5 years ago
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.
Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
Attachment #677874 - Flags: review?(gavin.sharp)
(Reporter)

Comment 2

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=fdc19c9d9061
Attachment #677874 - Flags: review?(gavin.sharp) → review+
Could also add a simple test that toggling notifications are only sent once etc.
(Reporter)

Comment 4

5 years ago
new try at https://tbpl.mozilla.org/?tree=Try&rev=b0e8cda8eb82
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
That Try run is positively failtastic. Please don't request checkin until your run is finished and verified to be OK.
Keywords: checkin-needed
(Assignee)

Comment 6

5 years ago
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?
(Assignee)

Comment 7

5 years ago
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 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?
Attachment #678702 - Flags: feedback+
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.
(Reporter)

Comment 10

5 years ago
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
Attachment #678702 - Flags: review?(gavin.sharp)
Attachment #678702 - Flags: review?(gavin.sharp) → review+
(Reporter)

Comment 11

5 years ago
Created attachment 678954 [details] [diff] [review]
v3
Attachment #678954 - Flags: review?(mhammond)
(Assignee)

Comment 12

5 years ago
Comment on attachment 678954 [details] [diff] [review]
v3

The _initialized change is too risky for beta - going with the original.
Attachment #678954 - Attachment is obsolete: true
Attachment #678954 - Attachment is patch: true
Attachment #678954 - Flags: review?(mhammond)
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d534f9a6a3
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

5 years ago
Assignee: mixedpuppy → mhammond
I pushed a follow-up to address gavin's comment 8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3bec8c880a7
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
Attachment #678986 - Flags: review+
Attachment #678986 - Flags: approval-mozilla-beta?
Attachment #678986 - Flags: approval-mozilla-aurora?
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
status-firefox17: --- → fixed
status-firefox18: --- → affected
https://hg.mozilla.org/releases/mozilla-aurora/rev/d275b56629b1
status-firefox18: affected → fixed
Attachment #678986 - Flags: approval-mozilla-beta?
Attachment #678986 - Flags: approval-mozilla-beta+
Attachment #678986 - Flags: approval-mozilla-aurora?
Attachment #678986 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/e3d534f9a6a3
https://hg.mozilla.org/mozilla-central/rev/e3bec8c880a7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Does this have or need tests?
Flags: in-testsuite?
(Assignee)

Comment 20

5 years ago
No tests, but the problems it addresses aren't really user-facing.  It's been working fine so I'll verify.
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite-
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.