nsHostRecord mallocs some extra data outside of its boundaries, then assigns its internal pointers to point to this data. We can do better by just storing these members as normal nsCStrings.

Comment on attachment 8937401 [details] Bug 1425807 - Convert nsHostKey members to nsCString https://reviewboard.mozilla.org/r/208070/#review213880 ::: netwerk/dns/nsHostResolver.h:48 (Diff revision 1) > - const char *netInterface; > - const char *originSuffix; > + const nsCString netInterface; > + const nsCString originSuffix; > + > + nsHostKey(const nsACString& host, uint16_t flags, > + uint16_t af, const nsCString& netInterface, > + const nsCString& originSuffix) netInterface and originSuffix should also be `const nsACString&` ::: netwerk/dns/nsHostResolver.cpp:165 (Diff revision 1) > // this macro filters out any flags that are not used when constructing the > // host key. the significant flags are those that would affect the resulting > // host record (i.e., the flags that are passed down to PR_GetAddrInfoByName). > #define RES_KEY_FLAGS(_f) ((_f) & nsHostResolver::RES_CANON_NAME) > > -nsHostRecord::nsHostRecord(const nsHostKey *key) > +bool nsHostKey::operator==(const nsHostKey& other) const { Format according to coding style: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style ``` return_type Class::method() { // code } ``` ::: netwerk/dns/nsHostResolver.cpp:174 (Diff revision 1) > + netInterface == other.netInterface && > + originSuffix == other.originSuffix; > +} > + > +size_t nsHostKey::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const { > + size_t n = 0; Same here - coding style. ::: netwerk/dns/nsHostResolver.cpp:346 (Diff revision 1) > size_t > nsHostRecord::SizeOfIncludingThis(MallocSizeOf mallocSizeOf) const > { > size_t n = mallocSizeOf(this); > > - // The |host| field (inherited from nsHostKey) actually points to extra > + n += SizeOfExcludingThis(mallocSizeOf); Either rename SizeOfExcludingThis to something else, or override it in nsHostRecord to include addr, callbacks, etc... ::: netwerk/dns/nsHostResolver.cpp:1561 (Diff revision 1) > // We don't pay attention to address literals, only resolved domains. > // Also require a host. > auto entry = static_cast<nsHostDBEnt*>(iter.Get()); > nsHostRecord* rec = entry->rec; > MOZ_ASSERT(rec, "rec should never be null here!"); > - if (!rec || !rec->addr_info || !rec->host) { > + if (!rec || !rec->addr_info || rec->host.IsEmpty()) { It checked whether host was null, not empty. Remove that condition. ::: netwerk/dns/nsHostResolver.cpp:1561 (Diff revision 1) > // We don't pay attention to address literals, only resolved domains. > // Also require a host. > auto entry = static_cast<nsHostDBEnt*>(iter.Get()); > nsHostRecord* rec = entry->rec; > MOZ_ASSERT(rec, "rec should never be null here!"); > - if (!rec || !rec->addr_info || !rec->host) { > + if (!rec || !rec->addr_info || rec->host.IsEmpty()) { It checked whether host was null, not empty. Remove that condition.

Comment on attachment 8937401 [details] Bug 1425807 - Convert nsHostKey members to nsCString https://reviewboard.mozilla.org/r/208070/#review213944 ::: netwerk/dns/nsHostResolver.h:55 (Diff revision 2) > + , flags(flags) > + , af(af) > + , netInterface(netInterface) > + , originSuffix(originSuffix) { > + } > + There are some failures on Try. You should look into those before we can land this ::: netwerk/dns/nsHostResolver.cpp:175 (Diff revision 2) > + netInterface == other.netInterface && > + originSuffix == other.originSuffix; > +} > + > +size_t > +nsHostKey::SizeOfExcludingThis(mozilla::MallocSizeOf mallocSizeOf) const { The { should go on the next line.

Comment on attachment 8937401 [details] Bug 1425807 - Convert nsHostKey members to nsCString https://reviewboard.mozilla.org/r/208070/#review214002 Looks good. Thanks!

Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/966836866e8a Convert nsHostKey members to nsCString r=valentin

Backed out for Build Bustages backout: https://hg.mozilla.org/integration/autoland/rev/ce813e8a46832f31a3c696880a8554e63b923fb8 push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=966836866e8afe423a01a9926834a26cb3d3e2b0 failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=152246084&repo=autoland&lineNumber=26688 [task 2017-12-19T01:01:25.758Z] 01:01:25 INFO - /builds/worker/workspace/build/src/netwerk/dns/nsHostResolver.cpp:1490:40: error: cannot convert 'const nsCString {aka const nsTString<char>}' to 'const char*' for argument '4' to 'nsresult mozilla::net::GetAddrInfo(const char*, uint16_t, uint16_t, const char*, mozilla::net::AddrInfo**, bool)' 26689 [task 2017-12-19T01:01:25.758Z] 01:01:25 INFO - getTtl); 26690 [task 2017-12-19T01:01:25.759Z] 01:01:25 INFO - ^ 26691 [task 2017-12-19T01:01:25.759Z] 01:01:25 INFO - /builds/worker/workspace/build/src/config/rules.mk:1040: recipe for target 'nsHostResolver.o' failed 26692 [task 2017-12-19T01:01:25.759Z] 01:01:25 INFO - gmake[4]: *** [nsHostResolver.o] Error 1 26693 [task 2017-12-19T01:01:25.759Z] 01:01:25 INFO - gmake[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/netwerk/dns' 26694 [task 2017-12-19T01:01:25.759Z] 01:01:25 INFO - gmake[4]: *** Waiting for unfinished jobs....

MozReview-Commit-ID: 9dJf09Icumw

Hi Natalia, I just posted a patch that fixes the build error. Could you checkin both patches, please? This bug is supposed to fix some leaks caused by bug 1424834 (which I relanded just before trying to land this one) Thanks!

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fc0b6106be47 Convert nsHostKey members to nsCString. r=valentin

Backed out in https://hg.mozilla.org/integration/autoland/rev/917b5ee4fe13 for static bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=152264130&repo=autoland

You should be able to reopen the review, add the fix from the patch I uploaded, and do another try-push for linux64 and maybe asan. The problem with the leaks was fixed in bug 1424834.

Comment on attachment 8937401 [details] Bug 1425807 - Convert nsHostKey members to nsCString Apparently I have to clear the review flag before pushing an updated patch?

Comment on attachment 8937401 [details] Bug 1425807 - Convert nsHostKey members to nsCString https://reviewboard.mozilla.org/r/208070/#review215072 C/C++ static analysis found 3 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: netwerk/dns/nsHostResolver.h:72 (Diff revision 4) > public: > NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsHostRecord) > > /* instantiates a new host record */ > static nsresult Create(const nsHostKey *key, nsHostRecord **record); > + nsHostRecord(const nsHostKey& key); Error: Bad implicit conversion constructor for 'nshostrecord' [clang-tidy: mozilla-implicit-constructor] ::: netwerk/dns/nsHostResolver.h:72 (Diff revision 4) > public: > NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsHostRecord) > > /* instantiates a new host record */ > static nsresult Create(const nsHostKey *key, nsHostRecord **record); > + nsHostRecord(const nsHostKey& key); Error: Bad implicit conversion constructor for 'nshostrecord' [clang-tidy: mozilla-implicit-constructor] ::: netwerk/dns/nsHostResolver.cpp:185 (Diff revision 4) > + n += netInterface.SizeOfExcludingThisIfUnshared(mallocSizeOf); > + n += originSuffix.SizeOfExcludingThisIfUnshared(mallocSizeOf); > + return n; > +} > + > +nsHostRecord::nsHostRecord(const nsHostKey& key) Error: Bad implicit conversion constructor for 'nshostrecord' [clang-tidy: mozilla-implicit-constructor]

You should declare the constructors and destructors as explicit and private.

Destructor is already declared as private by the refcounting macro.

The 3 issues still open in reviewboard had to be marked as fixed before this can land.

Marked as fixed.

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/5fc9e32e0988 Convert nsHostKey members to nsCString r=valentin