Comment on attachment 8939108 [details] Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable https://reviewboard.mozilla.org/r/209524/#review215242 Yeah this looks great. I think maybe there are a few merge conflicts? ::: netwerk/dns/nsHostResolver.cpp:534 (Diff revision 1) > PRCList *node = mEvictionQ.next; > while (node != &mEvictionQ) { > nsHostRecord *rec = static_cast<nsHostRecord *>(node); > node = node->next; > PR_REMOVE_AND_INIT_LINK(rec); > - mDB.Remove(static_cast<nsHostKey *>(rec)); > + mRecordDB.Remove(*(nsHostKey *) rec); static_cast? ::: netwerk/dns/nsHostResolver.cpp:1262 (Diff revision 1) > else { > // remove first element on mEvictionQ > nsHostRecord *head = > static_cast<nsHostRecord *>(PR_LIST_HEAD(&mEvictionQ)); > PR_REMOVE_AND_INIT_LINK(head); > - mDB.Remove(static_cast<nsHostKey *>(head)); > + mRecordDB.Remove(*(nsHostKey *)head); static_cast? ::: netwerk/dns/nsHostResolver.cpp:1328 (Diff revision 1) > } > } > > // If there are no more callbacks, remove the hash table entry > if (recPtr && recPtr->mCallbacks.isEmpty()) { > - mDB.Remove(static_cast<nsHostKey *>(recPtr)); > + mRecordDB.Remove(*(nsHostKey *)recPtr); static_cast?

Comment on attachment 8939108 [details] Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable https://reviewboard.mozilla.org/r/209524/#review215244 Err, didn't mean to tamper with the review flag.

Comment on attachment 8939108 [details] Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable https://reviewboard.mozilla.org/r/209524/#review215246

Comment on attachment 8939107 [details] Bug 1427373 - Use a refPtr for rec so that CompleteLookup doesn't need to release it https://reviewboard.mozilla.org/r/209522/#review215486 won't we then leak the rec at https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/netwerk/dns/nsHostResolver.cpp#601 ?

Comment on attachment 8939108 [details] Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable https://reviewboard.mozilla.org/r/209524/#review215488 ::: netwerk/dns/nsHostResolver.cpp:534 (Diff revision 1) > PRCList *node = mEvictionQ.next; > while (node != &mEvictionQ) { > nsHostRecord *rec = static_cast<nsHostRecord *>(node); > node = node->next; > PR_REMOVE_AND_INIT_LINK(rec); > - mDB.Remove(static_cast<nsHostKey *>(rec)); > + mRecordDB.Remove(*(nsHostKey *) rec); yep, why not s_c ? ::: netwerk/dns/nsHostResolver.cpp:682 (Diff revision 1) > > nsHostKey key(nsCString(host), flags, af, nsCString(netInterface), > originSuffix); > - auto he = static_cast<nsHostDBEnt*>(mDB.Add(&key, fallible)); > + RefPtr<nsHostRecord>& entry = mRecordDB.GetOrInsert(key); > + if (!entry) { > + entry = new nsHostRecord(key); this is weird according https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/xpcom/ds/nsBaseHashtable.h#119 this should not be needed. ::: netwerk/dns/nsHostResolver.cpp:690 (Diff revision 1) > + RefPtr<nsHostRecord> rec = entry; > // if the record is null, the hash table OOM'd. > - if (!he) { > + if (!rec) { > LOG((" Out of memory: no cache entry for host [%s%s%s].\n", > LOG_HOST(host, netInterface))); > rv = NS_ERROR_OUT_OF_MEMORY; I think this is no longer needed according https://dxr.mozilla.org/mozilla-central/rev/351c75ab74c9a83db5c0662ba271b49479adb1f1/xpcom/ds/nsTHashtable.h#449 ::: netwerk/dns/nsHostResolver.cpp:1320 (Diff revision 1) > nsHostRecord* recPtr = nullptr; > > - for (RefPtr<nsResolveHostCallback> c : he->rec->mCallbacks) { > + for (RefPtr<nsResolveHostCallback> c : rec->mCallbacks) { > if (c->EqualsAsyncListener(aListener)) { > c->remove(); > - recPtr = he->rec; > + recPtr = rec.get(); you probably don't need .get()?

(In reply to Honza Bambas (:mayhemer) from comment #7) > ::: netwerk/dns/nsHostResolver.cpp:682 > > + RefPtr<nsHostRecord>& entry = mRecordDB.GetOrInsert(key); > > + if (!entry) { > > + entry = new nsHostRecord(key); > > this is weird according > https://dxr.mozilla.org/mozilla-central/rev/ > 351c75ab74c9a83db5c0662ba271b49479adb1f1/xpcom/ds/nsBaseHashtable.h#119 > > this should not be needed. Technically true, but mRecordDB is a nsRefPtrHashtable, meaning that when not found, it allocates a refPtr and returns a reference to it. I still need to assign it. I'll remove the `if (!rec)` branch, since the allocation is now infallible.

(In reply to Honza Bambas (:mayhemer) from comment #6) > Comment on attachment 8939107 [details] > Bug 1427373 - Use a refPtr for rec so that CompleteLookup doesn't need to > release it > > https://reviewboard.mozilla.org/r/209522/#review215486 > > won't we then leak the rec at > https://dxr.mozilla.org/mozilla-central/rev/ > 351c75ab74c9a83db5c0662ba271b49479adb1f1/netwerk/dns/nsHostResolver.cpp#601 ? This is blocking r. (In reply to Valentin Gosu [:valentin] (PTO until Jan 8) from comment #8) > (In reply to Honza Bambas (:mayhemer) from comment #7) > > ::: netwerk/dns/nsHostResolver.cpp:682 > > > + RefPtr<nsHostRecord>& entry = mRecordDB.GetOrInsert(key); > > > + if (!entry) { > > > + entry = new nsHostRecord(key); > > > > this is weird according > > https://dxr.mozilla.org/mozilla-central/rev/ > > 351c75ab74c9a83db5c0662ba271b49479adb1f1/xpcom/ds/nsBaseHashtable.h#119 > > > > this should not be needed. > > Technically true, but mRecordDB is a nsRefPtrHashtable, meaning that when > not found, it allocates a refPtr and returns a reference to it. I still need > to assign it. > I'll remove the `if (!rec)` branch, since the allocation is now infallible. I'd rather check the final patch once again. thanks.

Comment on attachment 8939108 [details] Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable https://reviewboard.mozilla.org/r/209524/#review217208

(In reply to Honza Bambas (:mayhemer) from comment #11) > (In reply to Honza Bambas (:mayhemer) from comment #6) > > Comment on attachment 8939107 [details] > > Bug 1427373 - Use a refPtr for rec so that CompleteLookup doesn't need to > > release it > > > > https://reviewboard.mozilla.org/r/209522/#review215486 > > > > won't we then leak the rec at > > https://dxr.mozilla.org/mozilla-central/rev/ > > 351c75ab74c9a83db5c0662ba271b49479adb1f1/netwerk/dns/nsHostResolver.cpp#601 ? > > This is blocking r. I updated the patch to deal with the leak.

Comment on attachment 8939107 [details] Bug 1427373 - Use a refPtr for rec so that CompleteLookup doesn't need to release it https://reviewboard.mozilla.org/r/209522/#review217556 thanks!

Comment on attachment 8939108 [details] Bug 1427373 - Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable https://reviewboard.mozilla.org/r/209524/#review217578 in one way rb is a mess. I missed you have updated the patch in the meantime. thanks.

Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/autoland/rev/aaa8ca204b8b Use a refPtr for rec so that CompleteLookup doesn't need to release it r=mayhemer https://hg.mozilla.org/integration/autoland/rev/d739ece3bea2 Convert nsHostResolver.mDB from PLDHashTable to nsRefPtrHashtable r=mayhemer

https://hg.mozilla.org/mozilla-central/rev/aaa8ca204b8b https://hg.mozilla.org/mozilla-central/rev/d739ece3bea2