1420677 - Convert nsHostResolver.addr_info to a UniquePtr

Status: RESOLVED FIXED

(Core :: Networking: DNS, enhancement, P3)

Description

[jthemphill]

See Bug 1185120. Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d822df2aa2608cdeb34d1f6a1a7893247daefccb

Comment 1

[jthemphill]

Attachment: 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.

Comment 2

[jthemphill]

Err, I mean we'll be able to land Bug 1417827.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

(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?

Comment 4

[jthemphill]

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

Comment 5

[Valentin Gosu [:valentin] (he/him)]

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>`

Comment 6

[jthemphill]

This can be checked in once the patch for Bug 1420673 is checked in.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27719294cb73
Convert nsHostResolver.addr_info to a UniquePtr. r=valentin

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/27719294cb73

Comment 9

[jthemphill]

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!

Comment 10

[Valentin Gosu [:valentin] (he/him)]

That seems to be the case. Anyway, thanks for working on all these patches! It's proved to be very useful.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1420677 - Make AddrInfo.mAddresses an AutoCleanLinkedList r=dragana

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1420677 - Make AddrHostRecord.addr_info a RefPtr r=dragana

Depends on D22960

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edff1f39d426
Make AddrInfo.mAddresses an AutoCleanLinkedList r=dragana
https://hg.mozilla.org/integration/autoland/rev/cf114035c79f
Make AddrHostRecord.addr_info a RefPtr r=dragana

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/edff1f39d426
https://hg.mozilla.org/mozilla-central/rev/cf114035c79f

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (Bug 1420677) for causing Bug 1534550
Backout link: https://hg.mozilla.org/mozilla-central/rev/4d15e90af575a68815975bac8c2b602f78d76ee8

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (Bug 1420677) for causing Bug 1534550
https://hg.mozilla.org/mozilla-central/rev/4d15e90af575

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1420677 - Make sure to update addr_info_gencnt each time we change addr_info r=dragana!

When changing addr_info we didn't always update addr_info_gencnt, so when it the old AddrInfo was freed, even though we lock in nsDNSRecord::GetNextAddr, mIter would still point to the old AddrInfo.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9243110b784
Make AddrHostRecord.addr_info a RefPtr r=dragana
https://hg.mozilla.org/integration/autoland/rev/661bfa5ea263
Make AddrInfo.mAddresses an AutoCleanLinkedList r=dragana
https://hg.mozilla.org/integration/autoland/rev/1d783ed68779
Make sure to update addr_info_gencnt each time we change addr_info r=dragana

Comment 20

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/a9243110b784
https://hg.mozilla.org/mozilla-central/rev/661bfa5ea263
https://hg.mozilla.org/mozilla-central/rev/1d783ed68779