Closed Bug 1382913 Opened 3 years ago Closed 3 years ago

Removing a non-existing observer should fail


(Firefox :: Sync, enhancement, P3)




Firefox 56
Tracking Status
firefox56 --- fixed


(Reporter: markh, Assigned: markh)



(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 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.
Priority: -- → P3
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 on attachment 8889720 [details]
Bug 1382913 - throw an error when a non existing observer is removed from observers.js.

*polishes rubber stamp* I think `nsObserverService::RemoveObserver` does the same for nonexistent observers.
Attachment #8889720 - Flags: review?(kit) → review+
Pushed by
throw an error when a non existing observer is removed from observers.js. r=kitcambridge
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.