Closed
Bug 1382913
Opened 7 years ago
Closed 7 years ago
Removing a non-existing observer should fail
Categories
(Firefox :: Sync, enhancement, P3)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: markh, Assigned: markh)
Details
Attachments
(1 file)
It's possible to Svc.Obs.remove(...) an observer that was never added. This typically means the code is confused in some way, and may have bad hidden consequences (eg, if you really are trying to remove an observer that was added but screwed up the remove() call, the observer may fire unexpectedly and have unintended consequences). So I propose we do this in 2 steps: * step 1, probably in another bug is to: ** add an "else" block at http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/services/common/observers.js#67 to throw an error, then fix up any test failures caused by it. ** replace throwing an error with a Cu.reportError (ie, so if this is hit in cases not covered by tests, it doesn't actually cause failures, but at least we've identified some cases of badness) ** consider having that module also log.error in a way that the reports end up in the sync logs. * step 2, probably in a couple of months: replace the Cu.reportError/log.error calls with a real exception that does cause failure.
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
This was trivial so I did it. I did some testing and I think just throwing (rather than warning) should be fine.
Assignee: nobody → markh
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8889720 [details] Bug 1382913 - throw an error when a non existing observer is removed from observers.js. https://reviewboard.mozilla.org/r/160784/#review166048 *polishes rubber stamp* I think `nsObserverService::RemoveObserver` does the same for nonexistent observers.
Attachment #8889720 -
Flags: review?(kit) → review+
Pushed by mhammond@skippinet.com.au: https://hg.mozilla.org/integration/autoland/rev/1c8ae3d9f7c2 throw an error when a non existing observer is removed from observers.js. r=kitcambridge
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1c8ae3d9f7c2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in
before you can comment on or make changes to this bug.
Description
•