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

RESOLVED FIXED in Firefox 59

Status

()

RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: valentin, Assigned: valentin)

Tracking

(Blocks: 1 bug)

Trunk
mozilla59

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8936353 - Flags: review?(n.nethercote)

Comment 3

a year ago
mozreview-review
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)
Attachment #8936352 - Flags: review?(honzab.moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
(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

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

https://reviewboard.mozilla.org/r/207074/#review213786
Attachment #8936353 - Flags: review?(n.nethercote) → review+

Comment 8

a year ago
Pushed by valentin.gosu@gmail.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/49cc08386f17
https://hg.mozilla.org/mozilla-central/rev/2503df83bbd9
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Status: RESOLVED → REOPENED
status-firefox59: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla59 → ---

Comment 11

a year ago
Pushed by valentin.gosu@gmail.com:
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
(Assignee)

Comment 12

a year ago
(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.
Flags: needinfo?(valentin.gosu)

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b69f2b7d373
https://hg.mozilla.org/mozilla-central/rev/00e1d58aedfe
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Status: RESOLVED → REOPENED
status-firefox59: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla59 → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

a year ago
Pushed by valentin.gosu@gmail.com:
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
(Assignee)

Comment 19

a year ago
Should be fixed in latest push.
Flags: needinfo?(valentin.gosu)

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d95e61ced980
https://hg.mozilla.org/mozilla-central/rev/8e004c02a872
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Updated

11 months ago
See Also: → bug 1452417
You need to log in before you can comment on or make changes to this bug.