nsIAlertsIconListener removes non-weak observer in destructor

RESOLVED FIXED

Status

()

Core
Widget: Gtk
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

({assertion, mlk})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
This causes assertions on lots of mochitest runs, and is quite illogical.
(Assignee)

Updated

7 years ago
Blocks: 529664
(Assignee)

Comment 1

7 years ago
Created attachment 513815 [details] [diff] [review]
Remove strong observers outside of destructor.
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?
(Assignee)

Comment 3

7 years ago
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?
(Assignee)

Comment 5

7 years ago
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
(Assignee)

Comment 6

7 years ago
(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
(Assignee)

Comment 7

7 years ago
Created attachment 513928 [details] [diff] [review]
Avoid leaking nsAlertsIconObserver objects by using a weak reference.
Attachment #513928 - Flags: review?(karlt)
(Assignee)

Comment 8

7 years ago
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."
http://hg.mozilla.org/mozilla-central/annotate/b5a403b079d3/modules/libpr0n/public/imgILoader.idl#l80

The rest looks good thanks.
Attachment #513928 - Flags: review?(karlt)
Attachment #513928 - Flags: review-
Attachment #513928 - Flags: feedback+
(Assignee)

Comment 10

7 years ago
Created attachment 513935 [details] [diff] [review]
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.
Attachment #513935 - Flags: review?(karlt)
(Assignee)

Updated

7 years ago
Attachment #513928 - Attachment is obsolete: true
Attachment #513935 - Flags: review?(karlt) → review+
(Assignee)

Updated

7 years ago
Depends on: 610267
Assignee: nobody → josh
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Blocks: 640452
http://hg.mozilla.org/projects/cedar/rev/a3b18039e32f
Keywords: checkin-needed
Whiteboard: fixed-in-cedar

Comment 12

7 years ago
http://hg.mozilla.org/mozilla-central/rev/a3b18039e32f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
No longer depends on: 610267
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Blocks: 659856
No longer blocks: 640452
You need to log in before you can comment on or make changes to this bug.