Last Comment Bug 823837 - host records leaking PRAddrInfo
: host records leaking PRAddrInfo
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: Networking: DNS (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Josh Aas
:
Mentors:
Depends on: 807678
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-20 23:13 PST by Josh Aas
Modified: 2013-02-12 12:07 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
affected
+
fixed
fixed
wontfix
wontfix


Attachments
fix v1.0 (677 bytes, patch)
2012-12-21 00:00 PST, Josh Aas
sworkman: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta-
Details | Diff | Review

Description Josh Aas 2012-12-20 23:13:29 PST
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 [:njn] (on vacation until July 11) 2012-12-20 23:51:02 PST
How many of these are likely to be alive, and how big are they?
Comment 2 Josh Aas 2012-12-21 00:00:56 PST
Created attachment 694729 [details] [diff] [review]
fix v1.0
Comment 3 Josh Aas 2012-12-21 06:31:17 PST
(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 2012-12-24 08:06:51 PST
Fixed on mozilla-central by patch for bug 807678. Leaving this bug open for the branches.
Comment 5 Steve Workman [:sworkman] (please use needinfo) 2012-12-26 14:28:40 PST
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 2012-12-28 08:33:44 PST
(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 2012-12-28 08:36:28 PST
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] (please use needinfo) 2012-12-28 11:26:33 PST
(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] 2012-12-29 16:50:01 PST
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 2012-12-30 02:51:42 PST
pushed to mozilla-aurora

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

Note You need to log in before you can comment on or make changes to this bug.