host records leaking PRAddrInfo

RESOLVED FIXED

Status

()

Core
Networking: DNS
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox17 wontfix, firefox18- affected, firefox19+ fixed, firefox20 fixed, firefox-esr10 wontfix, firefox-esr17 wontfix)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
How many of these are likely to be alive, and how big are they?
Whiteboard: [MemShrink]
(Assignee)

Comment 2

5 years ago
Created attachment 694729 [details] [diff] [review]
fix v1.0
Attachment #694729 - Flags: review?(sworkman)
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Updated

5 years ago
status-firefox-esr10: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
Summary: host records possibly leaking PRAddrInfo → host records leaking PRAddrInfo
(Assignee)

Comment 4

5 years ago
Fixed on mozilla-central by patch for bug 807678. Leaving this bug open for the branches.
status-firefox17: affected → wontfix
status-firefox20: affected → fixed
Depends on: 807678
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.
Attachment #694729 - Flags: review?(sworkman) → review+
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
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
Attachment #694729 - Flags: approval-mozilla-beta?
Attachment #694729 - Flags: approval-mozilla-aurora?
(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 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
Attachment #694729 - Flags: approval-mozilla-beta?
Attachment #694729 - Flags: approval-mozilla-beta-
Attachment #694729 - Flags: approval-mozilla-aurora?
Attachment #694729 - Flags: approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox18: ? → -
tracking-firefox19: ? → +
(Assignee)

Comment 10

5 years ago
pushed to mozilla-aurora

http://hg.mozilla.org/releases/mozilla-aurora/rev/aaaca8c41d6f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
status-firefox19: affected → fixed

Updated

5 years ago
status-firefox-esr10: affected → wontfix
status-firefox-esr17: affected → wontfix
You need to log in before you can comment on or make changes to this bug.