795517 - Consider to split nsIObserverService::addObserver to nsIObserverService::addOwningObserver and nsIObserverService::addWeakObserver

Status: NEW

(Core :: XPCOM, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Currently it is way too easy to accidentally pass false as the last parameter and cause
runtime leaks, which may not show up during shutdown because observer service ends
up releasing the observers during shutdown.

Because addons use addObserver, we could perhaps add first new methods, convert
all our code to use them, and add then a warning about use of addObserver.
Then, after a release or two, remove it.

Comment 1

[Mounir Lamouri (:mounir)]

That's a great idea. Alternatively, we could take an enum value but given XPIDL limitations, it might be less clear.

Comment 2

[Nathan Froyd [:froydnj]]

Was about to file a new bug on this after running into issues in bug 1339845 today.



Strawman proposal:

1. Change nsIObserverService.idl to make addObserver [binaryname(AddObserverFromScript)].  Make corresponding changes in nsObserverService.  Feel free to bikeshed a scarier name so people won't call it from C++.

2. Add %{C++ block to nsIObserverService.idl:

  nsresult AddStrongObserver(nsIObserver* aObserver, char* aTopic);
  nsresult AddWeakObserver(nsIObserver* aObserver, char* aTopic);

with the obvious implementations of those methods calling AddObserverFromScript.

3. Rewrite the entire tree to use Add{Strong,Weak}Observer.

Sound plausible?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

How would that help with JS implemented observers?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Olli Pettay [:smaug] from comment #3)
> How would that help with JS implemented observers?

It wouldn't, but you gotta start somewhere...maybe when we move everything to webextensions, we can go through and make a nice API for JS, since only our own chrome code will be using that?

Comment 5

[Gabriele Svelto [:gsvelto]]

Just stumbled upon this while working on bug 1435683. I'll probably just go ahead and do this in the new interface. My personal preference would be for addObserver() to use weak references by default and have a separate addStrongObserver() for those who really do want the observer service to own their objects. The reason for this is that our code is scattered with comments such as "this object is kept alive by the observer service" above calls deliberately using strong references. This makes it sound like most users do not expect the observer service to hold strong references by default.