635552 - nsIAlertsIconListener removes non-weak observer in destructor

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Josh Matthews [:jdm]]

This causes assertions on lots of mochitest runs, and is quite illogical.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Remove strong observers outside of destructor.

Comment 2

[Karl Tomlinson (:karlt)]

Can you spell out please why removing the observer in the destructor is illogical and what assertions it causes?

It would feel a bit awkward to me to not remove the observers when the object is destroyed, or does the observer hold a reference to the object to prevent that from happening?

Comment 3

[Josh Matthews [:jdm]]

The PR_FALSE argument to AddObserver means that the service holds a strong ref to the observer, ergo nsAlertsIconObserver won't be destroyed until a corresponding RemoveObserver occurs or the nsObserverService destroys all observers.  The assertion that occurs is something like "Observer service being used after shutdown", which happens when the observer service is being shut down and purges all observers.

Comment 4

[Karl Tomlinson (:karlt)]

OK, thanks.  I agree; it is illogical.

Are we leaking a nsAlertsIconObserver for every nsAlertsService::ShowAlertNotification then?

Should addObserver be used with ownsWeak = PR_TRUE?

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 513815 [details] [diff] [review]
Remove strong observers outside of destructor.

Now that I read the rest of this code, this is still not optimal.

Comment 6

[Josh Matthews [:jdm]]

(In reply to comment #4)
> Are we leaking a nsAlertsIconObserver for every
> nsAlertsService::ShowAlertNotification then?
> 
> Should addObserver be used with ownsWeak = PR_TRUE?

Yeah, it looks like a leak is definitely in the books.  I think a weak observer would do be correct thing to avoid it; I'll put together a patch.

Comment 7

[Josh Matthews [:jdm]]

Attachment: Avoid leaking nsAlertsIconObserver objects by using a weak reference.

Comment 8

[Josh Matthews [:jdm]]

Karl, if roc should be reviewing this instead then just redirect it to him, please.  I've verified that the observer now dies as soon as the alert disappears, and everything also works correctly if Firefox closes while an alert is showing.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 513928 [details] [diff] [review]
Avoid leaking nsAlertsIconObserver objects by using a weak reference.

> nsAlertsIconListener::~nsAlertsIconListener()
> {
>-  MOZ_COUNT_DTOR(nsAlertsIconListener);
>-
>-  if (mIconRequest)
>-    mIconRequest->CancelAndForgetObserver(NS_BINDING_ABORTED);

|this| is passed as imgIDecoderObserver to loadImage with mIconRequest
and the comment below makes me think the CancelAndForgetObserver is necessary:

"libpr0n does NOT keep a strong ref to the observer; this prevents
 reference cycles. This means that callers of loadImage should
 make sure to Cancel() the resulting request before the observer
 goes away."
http://hg.mozilla.org/mozilla-central/annotate/b5a403b079d3/modules/libpr0n/public/imgILoader.idl#l80

The rest looks good thanks.

Comment 10

[Josh Matthews [:jdm]]

Attachment: Avoid leaking nsAlertsIconObserver objects by using a weak reference.

Good call.  I read through CancelAndForgetObserver previously, but just assumed that it was a strong ref.  Sigh.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/projects/cedar/rev/a3b18039e32f

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a3b18039e32f