Closed Bug 635552 Opened 11 years ago Closed 11 years ago

nsIAlertsIconListener removes non-weak observer in destructor


(Core :: Widget: Gtk, defect)

Not set





(Reporter: jdm, Assigned: jdm)



(Keywords: assertion, memory-leak)


(1 file, 2 obsolete files)

This causes assertions on lots of mochitest runs, and is quite illogical.
Blocks: 529664
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?
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.
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 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.
Attachment #513815 - Attachment is obsolete: true
(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.
Keywords: mlk
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 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."

The rest looks good thanks.
Attachment #513928 - Flags: review?(karlt)
Attachment #513928 - Flags: review-
Attachment #513928 - Flags: feedback+
Good call.  I read through CancelAndForgetObserver previously, but just assumed that it was a strong ref.  Sigh.
Attachment #513935 - Flags: review?(karlt)
Attachment #513928 - Attachment is obsolete: true
Attachment #513935 - Flags: review?(karlt) → review+
Depends on: post2.0
Assignee: nobody → josh
Keywords: checkin-needed
Blocks: mlk-fx5+
Closed: 11 years ago
No longer depends on: post2.0
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
No longer blocks: mlk-fx5+
You need to log in before you can comment on or make changes to this bug.