Closed Bug 315421 Opened 17 years ago Closed 15 years ago

download manager leaks observer service and itself in cycle

Categories

(Toolkit :: Downloads API, defect)

x86
Linux
defect
Not set
trivial

Tracking

()

VERIFIED WORKSFORME

People

(Reporter: dbaron, Assigned: sdwilsh)

Details

(Keywords: memory-leak)

Attachments

(1 file, 1 obsolete file)

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-
Attached patch patch (obsolete) — Splinter Review
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.
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)
(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.
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?)?
Er, so I didn't look at the patch first.  Any reason why that never got committed?
Attached patch v2.0Splinter Review
Updated for the newer stuff.
Assignee: dbaron → sdwilsh
Attachment #202120 - Attachment is obsolete: true
Attachment #265639 - Flags: review?(gavin.sharp)
Whiteboard: [patch]
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
Closed: 15 years ago
Resolution: --- → WORKSFORME
Attachment #265639 - Flags: review?(mano)
Alright, well we should at least remove the #ifdef 0 then...
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.