823837 - host records leaking PRAddrInfo

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Josh Aas]

I was looking at host resolver memory management for another patch and it seems like the the addr_info (PRAddrInfo) belonging to host records (nsHostRecord) is never freed.

I could be missing something, filing this bug to look into it. A patch should just call PR_FreeAddrInfo from the nsHostRecord destructor.

Comment 1

[Nicholas Nethercote [inactive]]

How many of these are likely to be alive, and how big are they?

Comment 2

[Josh Aas]

Attachment: fix v1.0

Comment 3

[Josh Aas]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> How many of these are likely to be alive, and how big are they?

There is one for each resolved host, and we cache up to 400 of these at any given time. Size is hard for me to calculate, and it depends on the actual DNS results, but a very rough estimate might be 300-500 bytes per entry. That cache is freed on shutdown, and individual items can be freed when they are forced out of the cache to make room for others.

Comment 4

[Josh Aas]

Fixed on mozilla-central by patch for bug 807678. Leaving this bug open for the branches.

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 694729 [details] [diff] [review]
fix v1.0

Review of attachment 694729 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. For m-c, after your patch for bug 807678, I think we need a nullptr check in nsHostResolver::OnLookupComplete for 'old_addr_info', no?

I'm a little surprised that PR_FreeAddrInfo wasn't there before in the destructor. It looks like it resolves to 'PR_Free' or 'freeaddrinfo' - I don't know the ins and outs of NSPR, but it seems like PR_Free could have been called during destruction, but freeaddrinfo being called seems more doubtful. Has the leak been there for a long time, then? Not so important now ...

I'm also wondering for m-c if you should put MOZ_C|DTOR(nsHostRecord), MOZ_C|DTOR(AddrInfo) (and whatever other classes are involved), to do a bit of memory tracking. Might be helpful for future debugging.

Comment 6

[Josh Aas]

(In reply to Steve Workman [:sworkman] from comment #5)

> Looks good. For m-c, after your patch for bug 807678, I think we need a
> nullptr check in nsHostResolver::OnLookupComplete for 'old_addr_info', no?

I don't think so. 'delete' is null-safe.

> I'm a little surprised that PR_FreeAddrInfo wasn't there before in the
> destructor. It looks like it resolves to 'PR_Free' or 'freeaddrinfo' - I
> don't know the ins and outs of NSPR, but it seems like PR_Free could have
> been called during destruction, but freeaddrinfo being called seems more
> doubtful. Has the leak been there for a long time, then? Not so important
> now ...

I don't understand most of this paragraph. I didn't dig all the way back but it seems like this leak has been around for a long time.

> I'm also wondering for m-c if you should put MOZ_C|DTOR(nsHostRecord),
> MOZ_C|DTOR(AddrInfo) (and whatever other classes are involved), to do a bit
> of memory tracking. Might be helpful for future debugging.

Agreed, I'll look into it.

Comment 7

[Josh Aas]

Comment on attachment 694729 [details] [diff] [review]
fix v1.0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none, leak has been around a long time
User impact if declined: Continue to leak DNS addrinfo structures, which could add up to quite a bit of memory over time.
Testing completed (on m-c, etc.): Fix has been on m-c for 4-5 days.
Risk to taking this patch (and alternatives if risky): Not much risk
String or UUID changes made by this patch: none

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Josh Aas (Mozilla Corporation) from comment #6)
> (In reply to Steve Workman [:sworkman] from comment #5)
> 
> > Looks good. For m-c, after your patch for bug 807678, I think we need a
> > nullptr check in nsHostResolver::OnLookupComplete for 'old_addr_info', no?
> 
> I don't think so. 'delete' is null-safe.

Ah, of course. I've been using ref ptrs too much.

> > I'm a little surprised that PR_FreeAddrInfo wasn't there before in the
> > destructor. It looks like it resolves to 'PR_Free' or 'freeaddrinfo' - I
> > don't know the ins and outs of NSPR, but it seems like PR_Free could have
> > been called during destruction, but freeaddrinfo being called seems more
> > doubtful. Has the leak been there for a long time, then? Not so important
> > now ...
> 
> I don't understand most of this paragraph. I didn't dig all the way back but
> it seems like this leak has been around for a long time.

Yeah, I was rambling a little, sorry. But you answered the question.

> > I'm also wondering for m-c if you should put MOZ_C|DTOR(nsHostRecord),
> > MOZ_C|DTOR(AddrInfo) (and whatever other classes are involved), to do a bit
> > of memory tracking. Might be helpful for future debugging.
> 
> Agreed, I'll look into it.

Cool. Thanks.

Comment 9

[bhavana bajaj [:bajaj]]

Comment on attachment 694729 [details] [diff] [review]
fix v1.0

We are already very close to release Fx 18 with beta 6 released on Friday,hence this will not be able to make into Fx18, approving on aurora though as it is low risk & helps avoid mem leaks

Comment 10

[Josh Aas]

pushed to mozilla-aurora

http://hg.mozilla.org/releases/mozilla-aurora/rev/aaaca8c41d6f