1183781 - Small leak in DNS.cpp and nsHostResolver.cpp

Status: RESOLVED WORKSFORME

(Core :: Networking: DNS, defect, P2)

Description

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

Attachment: leak.txt

http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-asan/1436489018/mozilla-inbound_ubuntu64-asan_vm_test-mochitest-other-bm53-tests1-linux64-build420.txt.gz

Comment 1

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

Attachment: i - Fix leak in nsHostResolver

Comment 2

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

ASAN proving the leak is gone: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb7317f560a8
Unit tests on win/mac: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe81e01323bb

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8634059 [details] [diff] [review]
i - Fix leak in nsHostResolver

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

Looks good, thanks Valentin. r=me with some nits.

Per our IRC chat, please file a new bug to convert these raw pointers to nsAutoPtr or something like that.

::: netwerk/dns/nsHostResolver.cpp
@@ +863,5 @@
>                               LOG_HOST(host, netInterface),
>                               (af == PR_AF_INET) ? "AF_INET" : "AF_INET6"));
>  
> +                        if (he->rec->addr_info) {
> +                            // prevent leak

Put the comment before the if block and change to...

// Ensure existing |addr_info| in |he| is cleared before copying from |unSpecHe|.

@@ +870,1 @@
>                          he->rec->addr_info = nullptr;

Please put the "= nullptr" in the if block.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60d045b1c1a6

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60d045b1c1a6

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out at KaiRo's request for causing bug 1186435.
https://hg.mozilla.org/mozilla-central/rev/2ddec2dedced

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=36451e86c4d5

Comment 8

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

Attachment: Small leak in DNS.cpp and nsHostResolver.cpp

* makes AddrInfo extend nsISupports
* uses RefPtr for managing AddrInfo and nsHostResolver in nsHostResolver.cpp

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8689496 [details] [diff] [review]
Small leak in DNS.cpp and nsHostResolver.cpp

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

LGTM - couple of really minor style nits. r=me.

::: netwerk/dns/GetAddrInfo.cpp
@@ +362,5 @@
>    }
>  
>    bool filterNameCollision = !(aFlags & nsHostResolver::RES_ALLOW_NAME_COLLISION);
> +  RefPtr<AddrInfo> ai(new AddrInfo(aCanonHost, prai, disableIPv4,
> +                      filterNameCollision, canonName));

nit: filterNameCollision should align with aCanonHost on the line above since they're both in the list of params in AddrInfo's constructor.

::: netwerk/dns/nsHostResolver.cpp
@@ +1244,5 @@
>  
>          rec->negative = !rec->addr_info;
>          PrepareRecordExpiration(rec);
>          rec->resolving = false;
>          

nit: Can you remove this whitespace while you're updating the code please?

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9b6a1567ef6b27735e19b661e895753db58e902
Bug 1183781 - Small leak in DNS.cpp and nsHostResolver.cpp r=sworkman

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b9b6a1567ef6

Comment 13

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

The leak still exists since we backed out the patch fixing this about 3 times already.
I'm currently investigating it.

Comment 14

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 15

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

The leak was fixed a while ago, and it is no longer triggered in automation.
I'll open a new bug for converting the pointers to smart pointers.