Convert nsHostResolver.addr_info to a UniquePtr

REOPENED
Assigned to

Status

()

Core
Networking: DNS
P3
enhancement
REOPENED
2 months ago
a month ago

People

(Reporter: jthemphill, Assigned: jthemphill, Mentored)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment)

(Assignee)

Updated

2 months ago
Blocks: 1185120
Depends on: 1420673
(Assignee)

Comment 1

2 months ago
Created attachment 8931915 [details] [diff] [review]
addr_info.patch

This patch depends on the patch I submitted for Bug 1420673. I can switch the order pretty easily. I'm not sure how to make MozReview work with stacked patches and stacked bugs (I got the error "abort: cannot submit reviews referencing multiple bugs"), so I'm just submitting this the old-fashioned way.

Hopefully after we convert these over we'll be able to land Bug 1403802 without any memory leak complaints.
Attachment #8931915 - Flags: review?(valentin.gosu)
(Assignee)

Comment 2

2 months ago
Err, I mean we'll be able to land Bug 1417827.
(Assignee)

Updated

2 months ago
Blocks: 1417827
(In reply to jthemphill from comment #2)
> Err, I mean we'll be able to land Bug 1417827.

Does your push include the patch from Bug 1417827? Just to confirm that the leak was actually caused by this?
Assignee: nobody → jthemphill
Whiteboard: [necko-triaged]
(Assignee)

Comment 4

2 months ago
Yep! The tryserver commit hash for my push is d822df2aa260. You can see the `NSCString`s here: https://hg.mozilla.org/try/file/d822df2aa2608cdeb34d1f6a1a7893247daefccb/netwerk/dns/DNS.h#l153
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8931915 [details] [diff] [review]
addr_info.patch

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

You can submit individual patches for review by doing `hg push review -c <changeset_id>`
Attachment #8931915 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 6

2 months ago
This can be checked in once the patch for Bug 1420673 is checked in.
Keywords: checkin-needed

Comment 7

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/27719294cb73
Convert nsHostResolver.addr_info to a UniquePtr. r=valentin
Keywords: checkin-needed

Comment 8

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/27719294cb73
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1422173
(Assignee)

Comment 9

2 months ago
Okay, I guess the memory leak was here: https://hg.mozilla.org/integration/mozilla-inbound/file/tip/netwerk/dns/nsHostResolver.cpp#l873

The leak occurred because we had set the pointer to null but didn't free the underlying data. Now that we're using UniquePtr, we're good to go!
That seems to be the case. Anyway, thanks for working on all these patches! It's proved to be very useful.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: -- → P3
Blocks: 1423945
You need to log in before you can comment on or make changes to this bug.