315421 - download manager leaks observer service and itself in cycle

Status: VERIFIED WORKSFORME

(Toolkit :: Downloads API, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I need to double-check that the comment I added is true.

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 202120 [details] [diff] [review]
patch

r=[e-mail]

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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...)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Benjamin Smedberg]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

~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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(except for the cycles aspect, anyway)

Comment 10

[Darin Fisher]

(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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 12

[Shawn Wilsher :sdwilsh]

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?)?

Comment 13

[Shawn Wilsher :sdwilsh]

Er, so I didn't look at the patch first.  Any reason why that never got committed?

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

Updated for the newer stuff.

Comment 15

[Shawn Wilsher :sdwilsh]

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

Gavin is going to be out of town for a while.

Comment 16

[Asaf Romano (gone)]

See previous comments, this is no longer necessary.

Comment 17

[Asaf Romano (gone)]

Marking WFM, needs verification though.

Comment 18

[Shawn Wilsher :sdwilsh]

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

Comment 19

[Adam Guthrie]

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