download manager leaks observer service and itself in cycle

VERIFIED WORKSFORME

Status

()

--
trivial
VERIFIED WORKSFORME
13 years ago
10 years ago

People

(Reporter: dbaron, Assigned: sdwilsh)

Tracking

({memory-leak})

Trunk
x86
Linux
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

The download manager (toolkit version) leaks itself and the observer service (and thus a bunch of other services) in a cycle, since it calls AddObserver in init, which gives the observer service a strong reference to it, and it breaks both halves of the cycle in its destructor, which will never be called.
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → Firefox1.6-
Created attachment 202120 [details] [diff] [review]
patch

I need to double-check that the comment I added is true.
Attachment #202120 - Flags: review?(beng)
Severity: normal → trivial
Note that this has become a bit more urgent since this is half the cause for balsa being orange, since the session saver, which landed yesterday (bug 328159), eagerly instantiates the download manager in order to resume any in-progress downloads.

However, to fix the assertions, it looks like I'm going to have to remove the RemoveObserver calls as well.  I suppose that isn't too harmful, but it doesn't feel right; I always prefer removing observers to not removing them.
Then again, the bug as described here no longer exists since bug 326491 landed.

Benjamin, how are services *supposed* to use the service manager now?  Should they now just AddObserver strongly and not worry about it?  (That seems dangerous if the code ever becomes a non-service...)
Er, should have said:  how are services supposed to use the observer service now?  In particular, should they try to remove themselves as observers?  If so, when?
I checked in a temporary fix (just commenting out the RemoveObserver calls), which should fix the orange until I have some idea of what the new rules are supposed to be.

Comment 7

13 years ago
Which assertions were being triggered? It should be perfectly safe to call .removeObserver from the "xpcom-shutdown" notification and I recommend doing that even though it is no longer necessary on trunk. Making observerservice calls after xpcom-shutdown is where we start to (rightly) fire assertions.
~nsDownloadManager (called from FreeServices) was calling RemoveObserver.  That seems like it ought to be allowed, IMO, since it's a lot easier than setting up an observer.
(except for the cycles aspect, anyway)

Comment 10

13 years ago
(In reply to comment #8)
> ~nsDownloadManager (called from FreeServices) was calling RemoveObserver.  That
> seems like it ought to be allowed, IMO, since it's a lot easier than setting up
> an observer.

except that there's a catch-22 there... how can ~nsDownloadManager run if the observer service has a strong reference to it? ;-)

in general, services that are observers should not need to call removeObserver since they are going to be removed automatically by the observer service at xpcom-shutdown.  so, adding explicit calls to removeObserver is just code-bloat.
(In reply to comment #10)
> except that there's a catch-22 there... how can ~nsDownloadManager run if the
> observer service has a strong reference to it? ;-)

Because, once bug 326491 landed, this bug, as described in comment 0, no longer existed.  Before bug 326491, that's the problem described in comment 0.
(Assignee)

Comment 12

12 years ago
So, I'm working on the download manager code right now and see this bit.  Is the consensus that we do not need to remove the observers (even though it feels right?)?
(Assignee)

Comment 13

12 years ago
Er, so I didn't look at the patch first.  Any reason why that never got committed?
(Assignee)

Comment 14

12 years ago
Created attachment 265639 [details] [diff] [review]
v2.0

Updated for the newer stuff.
Assignee: dbaron → sdwilsh
Attachment #202120 - Attachment is obsolete: true
Attachment #265639 - Flags: review?(gavin.sharp)
(Assignee)

Updated

12 years ago
Whiteboard: [patch]
(Assignee)

Comment 15

12 years ago
Comment on attachment 265639 [details] [diff] [review]
v2.0

Gavin is going to be out of town for a while.
Attachment #265639 - Flags: review?(gavin.sharp) → review?(mano)
See previous comments, this is no longer necessary.
Marking WFM, needs verification though.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 18

12 years ago
Alright, well we should at least remove the #ifdef 0 then...

Comment 19

12 years ago
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070522 Minefield/3.0a5pre

I ran with XPCOM_MEM_LEAK_LOG, played around with the Download Manager and didn't see any leaks related to the DM or the obsservice in the logs. ->VERIFIED
Status: RESOLVED → VERIFIED
Priority: P2 → --
Target Milestone: Firefox 2 → ---
Version: unspecified → Trunk
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.