1424834 - Replace nsHostRecord.callbacks with LinkedList<RefPtr<nsResolveHostCallback>>

Status: RESOLVED FIXED

(Core :: Networking: DNS, enhancement)

Description

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

Currently the callbacks list is a PRCList, and we use manual AddRefing in a bunch of places. This makes difficult to reason about the correctness of the code.
Rewriting it with LinkedList<RefPtr<nsResolveHostCallback>> would make it much clearer, and would clarify who "owns" the callbacks at any time.

Comment 1

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

Attachment: Bug 1424834 - Replace nsHostRecord.callbacks with LinkedList<RefPtr<nsResolveHostCallback>>

* nsResolveHostCallback extends nsISupports (for addref-ing and because nsDNSAsyncRequest implements nsICancelable)
* nsResolveHostCallback extends LinkedListElement<RefPtr<nsResolveHostCallback>> so the list can properly manage references
* nsDNSAsyncRequest and nsDNSSyncRequest properly implement nsISupports and use RefPtr to manage lifetimes

Review commit: https://reviewboard.mozilla.org/r/207072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207072/

Comment 2

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

Attachment: Bug 1424834 - LinkedList::sizeOfExcludingThis should use ConstRawType instead of T*

Review commit: https://reviewboard.mozilla.org/r/207074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207074/

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8936352 [details]
Bug 1424834 - Replace nsHostRecord.callbacks with LinkedList<RefPtr<nsResolveHostCallback>>

https://reviewboard.mozilla.org/r/207072/#review213272

::: netwerk/dns/nsDNSService2.cpp:1071
(Diff revision 1)
> -            rv = syncReq.mStatus;
> +            rv = syncReq->mStatus;
>          else {
> -            NS_ASSERTION(syncReq.mHostRecord, "no host record");
> -            auto *rec = new nsDNSRecord(syncReq.mHostRecord);
> -            if (!rec)
> +            NS_ASSERTION(syncReq->mHostRecord, "no host record");
> +            RefPtr<nsDNSRecord> rec = new nsDNSRecord(syncReq->mHostRecord);
> +            if (!rec) {
>                  rv = NS_ERROR_OUT_OF_MEMORY;

just for the cleanup sake, the !rec branch can probably be removed

::: netwerk/dns/nsHostResolver.h:167
(Diff revision 1)
>     ~nsHostRecord();
>  };
>  
> -/**
> - * ResolveHost callback object.  It's PRCList members are used by
> - * the nsHostResolver and should not be used by anything else.
> +class nsResolveHostCallback
> +    : public mozilla::LinkedListElement<RefPtr<nsResolveHostCallback>>
> +    , public nsISupports

please add a comment that nsISupports here is implemented by the classes that derive from this one and you need (IIUC) to be able to do RefPtr over this class.

::: netwerk/dns/nsHostResolver.cpp:984
(Diff revision 1)
>      }
>      if (result) {
>          callback->OnResolveHostComplete(this, result, status);
> +        if (callback->isInList()) {
> +            callback->remove();
> +        }

is there any reason why the removal shouldn't happen before you call on the callback?  (reentrance protection)

::: netwerk/dns/nsHostResolver.cpp:1031
(Diff revision 1)
> -    if (rec)
> +    if (rec) {
>          callback->OnResolveHostComplete(this, rec, status);
> +
> +        if (callback->isInList()) {
> +            callback->remove();
> +        }

same here

::: netwerk/dns/nsHostResolver.cpp:1387
(Diff revision 1)
>              }
>  #endif
>          }
>      }
>  
> -    if (!PR_CLIST_IS_EMPTY(&cbs)) {
> +    if (!cbs.isEmpty()) {

I think you don't need this condition?

::: netwerk/dns/nsHostResolver.cpp:1388
(Diff revision 1)
>  #endif
>          }
>      }
>  
> -    if (!PR_CLIST_IS_EMPTY(&cbs)) {
> -        PRCList *node = cbs.next;
> +    if (!cbs.isEmpty()) {
> +        for (nsResolveHostCallback* c = cbs.getFirst(); c; c = c->removeAndGetNext()) {

probably make |c| a refptr

::: netwerk/dns/nsHostResolver.cpp:1419
(Diff revision 1)
>      nsHostKey key = { host, flags, af, netInterface, originSuffix.get() };
>      auto he = static_cast<nsHostDBEnt*>(mDB.Search(&key));
>      if (he) {
>          nsHostRecord* recPtr = nullptr;
> -        PRCList *node = he->rec->callbacks.next;
> -        // Remove the first nsDNSAsyncRequest callback which matches the
> +
> +        for (nsResolveHostCallback* c : he->rec->mCallbacks) {

here probably as well

::: netwerk/dns/nsHostResolver.cpp:1423
(Diff revision 1)
> -            if (callback && (callback->EqualsAsyncListener(aListener))) {
> -                // Remove from the list of callbacks
> -                PR_REMOVE_LINK(callback);
>                  recPtr = he->rec;
> -                callback->OnResolveHostComplete(this, recPtr, status);
> +                c->OnResolveHostComplete(this, recPtr, status);
> +                c->remove();

probably remove before calling? (same reentrancy case)

Comment 4

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

Comment on attachment 8936352 [details]
Bug 1424834 - Replace nsHostRecord.callbacks with LinkedList<RefPtr<nsResolveHostCallback>>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207072/diff/1-2/

Comment 5

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

Comment on attachment 8936353 [details]
Bug 1424834 - LinkedList::sizeOfExcludingThis should use ConstRawType instead of T*

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207074/diff/1-2/

Comment 6

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

(In reply to Honza Bambas (:mayhemer) from comment #3)
> ::: netwerk/dns/nsHostResolver.cpp:984
> (Diff revision 1)
> >      }
> >      if (result) {
> >          callback->OnResolveHostComplete(this, result, status);
> > +        if (callback->isInList()) {
> > +            callback->remove();
> > +        }

right.

> ::: netwerk/dns/nsHostResolver.cpp:1031
> > +    if (rec) {
> >          callback->OnResolveHostComplete(this, rec, status);
> > +        if (callback->isInList()) {
> > +            callback->remove();
> same here

I got rid of this remove, since if rec != null, the callback was already removed above.

> ::: netwerk/dns/nsHostResolver.cpp:1388
> > +    if (!cbs.isEmpty()) {
> > +        for (nsResolveHostCallback* c = cbs.getFirst(); c; c = c->removeAndGetNext()) {
> 
> probably make |c| a refptr

This actually doesn't need to be a refPtr, since cbs is on the stack, and the list still references objects until they are removed.

> 
> ::: netwerk/dns/nsHostResolver.cpp:1419
> > +        for (nsResolveHostCallback* c : he->rec->mCallbacks) {
> here probably as well

True, we probably need it here.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8936353 [details]
Bug 1424834 - LinkedList::sizeOfExcludingThis should use ConstRawType instead of T*

https://reviewboard.mozilla.org/r/207074/#review213786

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49cc08386f17
Replace nsHostRecord.callbacks with LinkedList<RefPtr<nsResolveHostCallback>> r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/2503df83bbd9
LinkedList::sizeOfExcludingThis should use ConstRawType instead of T* r=njn

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/49cc08386f17
https://hg.mozilla.org/mozilla-central/rev/2503df83bbd9

Comment 10

[Cosmin Sabou [:CosminS]]

Backed out for leaks at netwerk/dns/nsHostResolver.cpp:352

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2503df83bbd93ae1b1090f4e63a176ff68a20cfb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=pending&filter-resultStatus=running&filter-resultStatus=success&filter-searchStr=954bc3698db85a600b6154cab42aa39c15ec77d6

https://treeherder.mozilla.org/logviewer.html#?job_id=152070724&repo=autoland&lineNumber=3808

https://hg.mozilla.org/mozilla-central/rev/5572465c08a9ce0671dcd01c721f9356fcd53a65

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b69f2b7d373
Replace nsHostRecord.callbacks with LinkedList<RefPtr<nsResolveHostCallback>> r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/00e1d58aedfe
LinkedList::sizeOfExcludingThis should use ConstRawType instead of T* r=njn

Comment 12

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

(In reply to Cosmin Sabou [:cosmin_sabou] from comment #10)
> Backed out for leaks at netwerk/dns/nsHostResolver.cpp:352

This issue should be fixed by bug 1425807 comment 7 which I just pushed to autoland.

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9b69f2b7d373
https://hg.mozilla.org/mozilla-central/rev/00e1d58aedfe

Comment 14

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1424834) for AddressSanitizer failure r=backout a=backout 

https://treeherder.mozilla.org/logviewer.html#?job_id=152325414&repo=mozilla-central&lineNumber=3814

https://hg.mozilla.org/mozilla-central/rev/048f69ab51e2845408403ca436ad1c62a9245912

Comment 15

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

Comment on attachment 8936352 [details]
Bug 1424834 - Replace nsHostRecord.callbacks with LinkedList<RefPtr<nsResolveHostCallback>>

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207072/diff/2-3/

Comment 16

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

Comment on attachment 8936353 [details]
Bug 1424834 - LinkedList::sizeOfExcludingThis should use ConstRawType instead of T*

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207074/diff/2-3/

Comment 17

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

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d95e61ced980
Replace nsHostRecord.callbacks with LinkedList<RefPtr<nsResolveHostCallback>> r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/8e004c02a872
LinkedList::sizeOfExcludingThis should use ConstRawType instead of T* r=njn

Comment 19

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

Should be fixed in latest push.

Comment 20

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d95e61ced980
https://hg.mozilla.org/mozilla-central/rev/8e004c02a872