58736 - nsHTTPIndex::CommonInit leaks RDFServiceImpl

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

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

nsHTTPIndex::CommonInit leaks references to the RDFService when called multiple
times, since it does a GetService into a global that can already contain an
owning reference.  This leak was introduced in revision 1.51 on August 14 to fix
bug 47927.

Comment 1

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

Attachment: patch that fixes the problem, although you might want to do a little more cleanup

Comment 2

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

If you don't want to do any other cleanup of |Init| and |CommonInit|, and you
think that patch is right, feel free to review the patch and assign the bug to
me...

Comment 3

[Robert John Churchill]

Attachment: diff for nsDirectoryViewer.cpp

Comment 4

[Robert John Churchill]

Attachment: diff for nsDirectoryViewer.h

Comment 5

[Robert John Churchill]

Due to the nature of nsDirectoryViewer where it can act both as a singleton and 
not, the RDF service variable shouldn't be a shared global; instead, make it a 
method variable.

dbaron, care to try out the attached diff(s) ?

Comment 6

[Robert John Churchill]

Attachment: diff #2 for nsDirectoryViewer

Comment 7

[Robert John Churchill]

Oops, no need for nsServiceManager::ReleaseService() when using do_GetService... 
so use second diff for nsDirectoryViewer.cpp

Comment 8

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

Patch works fine -- doesn't leak.  A few comments:

 * weird whitespace stuff going on.  Looks like tabs->spaces in some places and
spaces->tabs in others.  I guess it should probably be tabs->spaces (but you
don't need to hit stuff you didn't change otherwise).

 * No need to null out the nsCOMPtr in the destructor - it will be freed at the
end of the dtor anyway.

 * You can use do_GetService(..., &rv) to propagate the return value the way you
did before, rather than make up NS_ERROR_NULL_POINTER.

With that fixed, r=dbaron if you want it.

Comment 9

[Robert John Churchill]

Chris (Waterson), more r= bug love needed.  :)

Comment 10

[Chris Waterson]

[r,sr]=waterson, pick one!

Comment 11

[Robert John Churchill]

Fixed.

Comment 12

[Tom Everingham]

verified