947721 - DNS CancelAsyncRequest broken for most callers

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Jason Duell]

Attachment: patch that needs some debugging--worked, but now test failing.

So it turns out that DNS CancelAsyncRequest doesn't actually work in any cases where a 'target' has been passed in (i.e. most cases).  The issue is that CancelAsyncRequest uses a {hostname, flags, listener} key to search outstanding requests, but it will never find one, since in the non-null target case, AsyncResolve will have wrapped the listener in a proxy object:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp#681

The comparison function nsDNSAsyncRequest::EqualsAsyncListener() doesn't take the wrapper into account.

I have a patch with a test that shows the issue, and that fixes regular parent process DNS.  I still need to crank out a solution to child processes (which depends on bug 945066).

Looks like this has been broken since bug 622232 landed in January?

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: Fix v2

Comment 2

[Jason Duell]

Comment on attachment 8439323 [details] [diff] [review]
Fix v2

Steve is going to take this review.  Apparently he wrote the code in question...

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8439323 [details] [diff] [review]
Fix v2

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

Good work. Mainly some style comments and a question/suggestion about hash table criteria.

::: netwerk/dns/ChildDNSService.cpp
@@ +20,5 @@
>  //-----------------------------------------------------------------------------
>  // ChildDNSService
>  //-----------------------------------------------------------------------------
>  
> +ChildDNSService *gChildDNSService =  nullptr;

If you use GetSingleton rather than this global in other files, you can keep this as static.

@@ +44,5 @@
>  
>  ChildDNSService::ChildDNSService()
>    : mFirstTime(true)
>    , mOffline(false)
> +  , mHashtableLock("PandingRequestsHashtableLock")

s/PandingRequestsHashtableLock/DNSPendingRequestsLock/

@@ +86,5 @@
>      NS_GetMainThread(getter_AddRefs(mainThread));
>      target = do_QueryInterface(mainThread);
>    }
> +
> +  // in request's hash table we need to store original listeners

nit: Please capitalize comments, and end with a '.'.

@@ +87,5 @@
>      target = do_QueryInterface(mainThread);
>    }
> +
> +  // in request's hash table we need to store original listeners
> +  nsIDNSListener  *listener2 = listener;

Please remove the extra space after nsIDNSListener.

s/listener2/originalListener/

@@ +97,5 @@
>    }
>  
>    nsRefPtr<DNSRequestChild> childReq =
>      new DNSRequestChild(nsCString(hostname), flags, listener, target);
> +  mHashtableLock.Lock();

Please use MutexAutoLock here instead of calling Lock and Unlock directly; and s/mHashtableLock/mPendingRequestsLock/ ...

{
  MutexAutoLock(mPendingRequestsLock);
  mPendingRequests.Put(...);
}

Check out other examples of MutexAutoLock in the code.

@@ +119,5 @@
>    }
>  
> +  nsRefPtr<DNSRequestChild> childReq;
> +  mHashtableLock.Lock();
> +  if (mPendingRequestsHash.Get(aListener,getter_AddRefs(childReq))) {

nit: space after ',' in param list.

Also, I think we need to hash on hostname, flags and listener here. Currently, you're effectively remembering one request for each unique listener, but a listener may be waiting on multiple requests. Thus, it may be better to remember every request using hostname, flags and listener as the hash key. Otherwise, if one request completes it will remove itself from the pending request table and other requests with the same listener will not be cancelable.

What do you think?

@@ +156,5 @@
> +
> +void
> +ChildDNSService::removeRequest(nsIDNSListener *listener)
> +{
> +  mHashtableLock.Lock();

MutexAutoLock.

::: netwerk/dns/ChildDNSService.h
@@ +34,5 @@
>    virtual ~ChildDNSService();
>  
>    static ChildDNSService* GetSingleton();
> +  
> +  void removeRequest(nsIDNSListener* listener);

nit: Upper case for functions -> RemoveRequest.
nit: Whitespace.

I think we can rename this too. RemoveRequest sounds like we might be canceling the request, but really we just want the ChildDNSService to remove it from the pending queue, so how about NotifyRequestDone? I think we could also change the param to be a DNSRequestChild object - I think we need to hash on more data than just the listener for this (see other comments), so if we have the object, we can get all the data we need.

@@ +39,5 @@
>  private:
>    bool mFirstTime;
>    bool mOffline;
>    bool mDisablePrefetch;
> +  nsRefPtrHashtable<nsPtrHashKey<nsIDNSListener>, DNSRequestChild> mPendingRequestsHash;

Please add a short comment to explain why this hash table is needed.
You can also just call it mPendingRequests.

@@ +40,5 @@
>    bool mFirstTime;
>    bool mOffline;
>    bool mDisablePrefetch;
> +  nsRefPtrHashtable<nsPtrHashKey<nsIDNSListener>, DNSRequestChild> mPendingRequestsHash;
> +  Mutex mHashtableLock;  

Whitespace.

@@ +45,3 @@
>  };
>  
> +extern ChildDNSService *gChildDNSService;

Rather than extern'ing this, you could just use ChildDNSService::GetSingleton. That way you can keep the global var as a static, and ensure that the singleton is initialized when you use it.

::: netwerk/dns/DNSListenerProxy.cpp
@@ +10,5 @@
>  
>  namespace mozilla {
>  namespace net {
>  
> +NS_IMPL_ISUPPORTS(DNSListenerProxy, nsIDNSListener, nsIDNSListenerProxy)

nit: Put these on separate lines. It may reduce the number of lines of code to be changed in a future patch. (Although, it's unlikely that this file will be touched a lot! Still, it's a good practice to get into).

NS_IMPL_ISUPPORTS(DNSListenerProxy
                , nsIDNSListener
                , nsIDNSListenerProxy)

::: netwerk/dns/DNSListenerProxy.h
@@ +17,5 @@
>  
>  namespace mozilla {
>  namespace net {
>  
> +class DNSListenerProxy MOZ_FINAL 

Whitespace.

::: netwerk/dns/DNSRequestChild.cpp
@@ +192,5 @@
> +{
> +  // we can only do IPDL on the main thread
> +  if (!NS_IsMainThread()) {
> +    NS_DispatchToMainThread(
> +      NS_NewRunnableMethod(this, &DNSRequestChild::CancelRequestRun));

If you use an nsRunnable class instead of a function, then you don't need to add the three new member vars to DNSRequestChild. Instead, you can add them to the nsRunnable. Just add a class definition above this function:

class CancelDNSRequestEvent : public nsRunnable
{
...
private:
  nsresult mReason_forCancel;
  ...
};

Then you should only need CancelRequest() and the nsRunnable class.

@@ +211,5 @@
> +    return;
> +  }
> +  //we remove it here because the request is already answered we do not need to cancel it any more
> +  //we will also try to remove it in ReleaseIPDLReference() just in case something crashes
> +  gChildDNSService->removeRequest(mListener);

// Request is done thus it cannot be canceled; remove it from the hash table.

@@ +255,5 @@
>    return true;
>  }
>  
> +void 
> +DNSRequestChild::ReleaseIPDLReference() 

Whitespace.

::: netwerk/dns/DNSRequestParent.cpp
@@ +56,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIDNSService> dns = do_GetService(NS_DNSSERVICE_CONTRACTID, &rv);
> +  if (NS_SUCCEEDED(rv)) {
> +    rv = dns->CancelAsyncResolve(hostName, flags, this, reason);
> +  } 

Whitespace.

::: netwerk/dns/DNSRequestParent.h
@@ +26,5 @@
>    virtual ~DNSRequestParent();
>  
>    void DoAsyncResolve(const nsACString  &hostname, uint32_t flags);
>  
> +  bool RecvCancelDNSRequest(const nsCString& hostName,

Add a comment here similar to the one I suggest in PDNSRequest.ipdl. Something to explain why we pass the args in the IPDL message instead of storing them in DNSRequestParent.

::: netwerk/dns/PDNSRequest.ipdl
@@ +22,2 @@
>    // constructor in PNecko takes AsyncResolve args that initialize request
> +  CancelDNSRequest(nsCString hostName, uint32_t flags, nsresult reason);

Add a newline before the message definition here.

Also, add a comment for CancelDNSRequest:
// Pass args here rather than storing them in the parent; they are only needed
// if the request is to be canceled.

(While reviewing the patch I was considering different approaches including an IPDL channel for ChildDNSService->DNSService, or a CancelDNSRequest message with no args and storing them in DNSRequestParent, but your approach seems to be optimal for memory. So, let's write a comment to explain the choice).

::: netwerk/dns/nsIDNSListener.idl
@@ +33,5 @@
> +/**
> + * nsIDNSListenerProxy: 
> + *
> + * Must be implemented by classes that wrap the original listener passed to
> + * nsIDNService.AsyncResolve, so we have access to original listener for

s/nsIDNService/nsIDNSService/

@@ +34,5 @@
> + * nsIDNSListenerProxy: 
> + *
> + * Must be implemented by classes that wrap the original listener passed to
> + * nsIDNService.AsyncResolve, so we have access to original listener for
> + * comparison purposes. 

nit: Whitespace at end of lines.

@@ +39,5 @@
> + */
> +[uuid(60eff0e4-6f7c-493c-add9-1cbea59063ad)]
> +interface nsIDNSListenerProxy : nsISupports
> +{
> +  readonly attribute nsIDNSListener wrappedListener;

Please add a short comment like: "/* The original nsIDNSListener which requested hostname resolution. */".

Also, maybe s/wrappedListener/originalListener/?

::: netwerk/test/unit/test_dns_cancel.js
@@ +20,5 @@
> +  }
> +};
> +
> +
> +function randomAddress(length) {

nit: One empty line between functions.

@@ +23,5 @@
> +
> +function randomAddress(length) {
> +    var chars = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
> +    var result = '';
> +    for (var i = length; i > 0; --i) result += chars[Math.round(Math.random() * (chars.length - 1))];

Please add braces:

for (...) {
  result += ...
}

@@ +24,5 @@
> +function randomAddress(length) {
> +    var chars = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
> +    var result = '';
> +    for (var i = length; i > 0; --i) result += chars[Math.round(Math.random() * (chars.length - 1))];
> +    return result;

nit: Indentation doesn't match the rest of the file.

@@ +29,5 @@
> +}
> +
> +
> +function run_test() {
> +  var address = randomAddress(30)

';'

@@ +33,5 @@
> +  var address = randomAddress(30)
> +  var threadManager = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
> +  var mainThread = threadManager.currentThread;
> +  dns.asyncResolve(address, 0, listener, mainThread);
> +  // immediately cancel request

nit: Capitalize and '.' in comments.

Can you add to this test to have multiple requests and listeners please?
-- 1 listener with 2 requests; 1 request should succeed, the other should be canceled.
-- 2 listeners with 1 request each; 1 succeeds, the other is canceled.

This is to make sure that the right request (i.e. hostname and flags) and listener combination is canceled.

@@ +38,5 @@
> +  dns.cancelAsyncResolve(address, 0, listener, Cr.NS_ERROR_ABORT);
> +
> +  do_test_pending();
> +}
> +

nit: Remove empty line at end of file.

Comment 4

[Dragana Damjanovic [:dragana]]

> @@ +119,5 @@
> >    }
> >  
> > +  nsRefPtr<DNSRequestChild> childReq;
> > +  mHashtableLock.Lock();
> > +  if (mPendingRequestsHash.Get(aListener,getter_AddRefs(childReq))) {
> 
> nit: space after ',' in param list.
> 
> Also, I think we need to hash on hostname, flags and listener here.
> Currently, you're effectively remembering one request for each unique
> listener, but a listener may be waiting on multiple requests. Thus, it may
> be better to remember every request using hostname, flags and listener as
> the hash key. Otherwise, if one request completes it will remove itself from
> the pending request table and other requests with the same listener will not
> be cancelable.
> 
> What do you think?
> 

This was one of my first bugs, I had a bit wrong idea how listeners work. 

we will need to hash on listener, hostname and flags

can you give me a hint hot it is done. I can think of something (make a unique string that hash on, or something)but it is better to do it in usual way.
I saw classes for nsStringHashKey, nsUint32HashKey, for RefPtr ...is there any helper class for a mix..
I was looking through the code but it is hard to find

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

No problem. If you look at http://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsHostResolver.cpp#312 you can see how the code for the DNS Cache itself - it's a hash table that uses hostname, flags and address family as the hash key. For some background, we use helper functions from HashFunctions.h and hash table setup from pldhash.h. We define a set of operation functions, set pointers to them in gHostDB_ops and then initialize the hash table with those op pointers in PL_DHashTableInit which is called in nsHostResolver:::Init.

Feel free to ask questions if you need.

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: Fix v3

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8455152 [details] [diff] [review]
Fix v3

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

Getting there. Mostly cleanup stuff in the main code; some more work needed in the test script.

::: netwerk/dns/ChildDNSService.cpp
@@ +18,5 @@
>  namespace net {
>  
>  //-----------------------------------------------------------------------------
> +
> +struct RequestHashKey

//----...
// Data structures for Request Hash Table
//----...

@@ +186,5 @@
> +    RequestHashKey key = {nsCString(hostname), originalFlags, originalListener};
> +    RequestHashEnt *he = static_cast<RequestHashEnt *>
> +                                     (PL_DHashTableOperate(&mPendingRequests,
> +                                                           &key, PL_DHASH_ADD));
> +    if (!he) {

So, I think we need to use PL_DHASH_ENTRY_IS_FREE here instead of null checking.

See http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.h#438

(nsHostResolver doesn't look right according to the instructions. You can fix that one if you want :) and I'll review it quickly).

@@ +213,5 @@
> +  RequestHashKey key = {nsCString(aHostname), aFlags, aListener};
> +  RequestHashEnt *he = static_cast<RequestHashEnt *>
> +                                  (PL_DHashTableOperate(&mPendingRequests,
> +                                                        &key, PL_DHASH_LOOKUP));
> +  if (he && he->mRequest) {

PL_DHASH_ENTRY_IS_BUSY

@@ +255,5 @@
> +  aFlags &= ~RESOLVE_OFFLINE;
> +  nsCOMPtr<nsIDNSListener> originalListener = aListener;
> +  nsCOMPtr<nsIDNSListenerProxy> wrapper = do_QueryInterface(aListener);
> +  if (wrapper) {
> +    wrapper->GetOriginalListener(getter_AddRefs(originalListener));

Do you think we can be sure that there is an originalListener in the wrapper? Seems like it to me. Assuming you think so too, let's add:

if (NS_WARN_IF(!originalListener)) {
  MOZ_ASSERT(originalListener);
  return;
}

Debug builds will crash; release builds will fail silently.

::: netwerk/dns/DNSListenerProxy.cpp
@@ +12,5 @@
>  namespace net {
>  
> +NS_IMPL_ISUPPORTS(DNSListenerProxy
> +                , nsIDNSListener
> +                , nsIDNSListenerProxy)

nit: commas don't need to be at the start of the line for this one; not quite like an initializer list. But good job for thinking ahead.

@@ +35,5 @@
> +NS_IMETHODIMP
> +DNSListenerProxy::GetOriginalListener(nsIDNSListener **aOriginalListener)
> +{
> +  *aOriginalListener = mListener;
> +  NS_IF_ADDREF(*aOriginalListener);

You can just do NS_IF_ADDREF(*aOriginalListener = mListener) here.

::: netwerk/dns/DNSRequestChild.h
@@ +39,5 @@
>    void CallOnLookupComplete();
> +  void CancelRequest(nsresult aReason);
> +
> +private:
> +  class CancelDNSRequestEvent : public nsRunnable

You can just put this class in mozilla::net in the cpp. Then you don't need to include nsThreadUtils in this file.

@@ +45,5 @@
> +  public:
> +    CancelDNSRequestEvent(DNSRequestChild* dnsReq, nsresult aReason)
> +      :  dnsReqest(dnsReq), mReasonForCancel(aReason)
> +    {
> +    }

Separate lines for params.
One space after ':'
{}

@@ +55,5 @@
> +                                      mReasonForCancel);
> +      return NS_OK;
> +    }
> +  private:
> +    nsRefPtr<DNSRequestChild> dnsReqest;

s/dnsReqest/mDnsRequest/

::: netwerk/dns/nsIDNSListener.idl
@@ +29,5 @@
>                            in nsresult      aStatus);
>  };
> +
> +
> +/**

nit: one line space, not two.

::: netwerk/test/unit/test_dns_cancel.js
@@ +1,3 @@
> +var dns = Cc["@mozilla.org/network/dns-service;1"].getService(Ci.nsIDNSService);
> +
> +var address1 = 'mozilla.org';

Our test infrastructure doesn't allow external access, so it's best not to rely on getting an address resolution for "mozilla.org" here. You could use the same hostname for the first two test cases, and vary the flags instead.

s/address/hostname/ for these vars.

@@ +14,5 @@
> +        do_check_true(answer == "127.0.0.1" || answer == "::1");
> +
> +        do_test_finished();
> +      } catch(ex) {
> +        do_throw("dns wrong request canceled: " + ex);

Looks like the exception thrown here is for getNextAddrAsString, not checking the address itself.

do_throw("Exception getting DNS record: " + ex);

@@ +22,5 @@
> +        // The other request is canceled.
> +        do_check_eq(inStatus, Cr.NS_ERROR_ABORT);
> +
> +        do_test_finished();
> +      } catch(ex) {

For both results, you should check that inRequest matches the return value of asyncResolve. So, you'll need to store the return values when you call asyncResolve.

@@ +44,5 @@
> +      do_check_neq(inStatus, Cr.NS_ERROR_ABORT);
> +
> +      do_test_finished();
> +    } catch(ex) {
> +      do_throw("dns request aborted: " + ex);

I don't think you need this try/catch here. do_check_neq and do_test_finished will cause the whole test to fail if they throw anything, which is what you want.

@@ +65,5 @@
> +  dns.asyncResolve(address2, 0, listener1, mainThread);
> +  dns.asyncResolve(address3, 0, listener2, mainThread);
> +
> +  // Immediately cancel request.
> +  dns.cancelAsyncResolve(address1, 0, listener1, Cr.NS_ERROR_ABORT);

As well as calling cancelAsyncResolve on the DNS Service, you should also store the result of asyncResolve (which is an nsICancelable), and call cancel on that.

request = dns.asyncResolve(...);
request.cancel(Cr.NS_ERROR_ABORT);

@@ +67,5 @@
> +
> +  // Immediately cancel request.
> +  dns.cancelAsyncResolve(address1, 0, listener1, Cr.NS_ERROR_ABORT);
> +
> +  do_test_pending();

do_test_finished should be called the same amount of times as do_test_pending, so add do_test_pending for each asyncResolve that you start.

Another way to do it is to avoid do_test_pending/finished and use add_test/run_next_test. It's not so important for this test, but it's nice in larger test scripts because the output in the test logs is a bit more helpful.

What you do is:

add_test(function test_cancel_dns_case_0() {
  dns.asyncResolve(address1, 0, listener1, mainThread);
}

Do something similar for each test case; probably in the second case you'll want to call dns.cancelAsyncResolve too.

Call run_next_test at the end of run_test, and it will call the first test function.

Then call run_next_test in the success branch of listener1.onLookupComplete; s/do_test_finished/run_next_test/. (Failure cases will stop the script, of course.

This is nicer in the test logs because it will say "Starting test_cancel_dns_case_0" when it is running that test. If you have to debug something, this is of course very useful :) and you don't have to add do_print statements yourself.

Like I wrote, it's not so important for this small file, so you can just call do_test_pending 3 times. Or if you want nice logs, use add_test/run_next_test :)

(If you really want to be fancy, you can use add_task instead of add_test. It involves generators rather than functions, the yield statement and Promises. But I think that is overkill and really not worth it!)

Comment 8

[Dragana Damjanovic [:dragana]]

> @@ +186,5 @@
> > +    RequestHashKey key = {nsCString(hostname), originalFlags, originalListener};
> > +    RequestHashEnt *he = static_cast<RequestHashEnt *>
> > +                                     (PL_DHashTableOperate(&mPendingRequests,
> > +                                                           &key, PL_DHASH_ADD));
> > +    if (!he) {
> 
> So, I think we need to use PL_DHASH_ENTRY_IS_FREE here instead of null
> checking.
> 
> See http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/pldhash.h#438
> 
> (nsHostResolver doesn't look right according to the instructions. You can
> fix that one if you want :) and I'll review it quickly).
> 

I will file  a new bug for this.


> 
> ::: netwerk/test/unit/test_dns_cancel.js
> @@ +1,3 @@
> > +var dns = Cc["@mozilla.org/network/dns-service;1"].getService(Ci.nsIDNSService);
> > +
> > +var address1 = 'mozilla.org';
> 
> Our test infrastructure doesn't allow external access, so it's best not to
> rely on getting an address resolution for "mozilla.org" here. You could use
> the same hostname for the first two test cases, and vary the flags instead.
> 

Resolving "localhost" is too fast, so it is not possible to cancel request on time.
So I have left mozilla.org but I do not rely on this being resolved, I just check if it is canceled or not.



> > +
> > +  // Immediately cancel request.
> > +  dns.cancelAsyncResolve(address1, 0, listener1, Cr.NS_ERROR_ABORT);
> 
> As well as calling cancelAsyncResolve on the DNS Service, you should also
> store the result of asyncResolve (which is an nsICancelable), and call
> cancel on that.
> 
> request = dns.asyncResolve(...);
> request.cancel(Cr.NS_ERROR_ABORT);
> 

I  need to call cancelAsyncResolve to test it as well :)



> @@ +67,5 @@
> > +
> > +  // Immediately cancel request.
> > +  dns.cancelAsyncResolve(address1, 0, listener1, Cr.NS_ERROR_ABORT);
> > +
> > +  do_test_pending();
> 
> do_test_finished should be called the same amount of times as
> do_test_pending, so add do_test_pending for each asyncResolve that you start.
> 
> Another way to do it is to avoid do_test_pending/finished and use
> add_test/run_next_test. It's not so important for this test, but it's nice
> in larger test scripts because the output in the test logs is a bit more
> helpful.
> 
> What you do is:
> 
> add_test(function test_cancel_dns_case_0() {
>   dns.asyncResolve(address1, 0, listener1, mainThread);
> }
> 
> Do something similar for each test case; probably in the second case you'll
> want to call dns.cancelAsyncResolve too.
> 
> Call run_next_test at the end of run_test, and it will call the first test
> function.
> 
> Then call run_next_test in the success branch of listener1.onLookupComplete;
> s/do_test_finished/run_next_test/. (Failure cases will stop the script, of
> course.
> 
> This is nicer in the test logs because it will say "Starting
> test_cancel_dns_case_0" when it is running that test. If you have to debug
> something, this is of course very useful :) and you don't have to add
> do_print statements yourself.
> 
> Like I wrote, it's not so important for this small file, so you can just
> call do_test_pending 3 times. Or if you want nice logs, use
> add_test/run_next_test :)
> 
> (If you really want to be fancy, you can use add_task instead of add_test.
> It involves generators rather than functions, the yield statement and
> Promises. But I think that is overkill and really not worth it!)


Thanks for the explanation this is really useful.
I decided to leave it only with one test with 3 checks to be sure that we canceled the right thing.
I could do 3 separate tests that do the same thing just the checks are different (different requests are checked), but the test is too simple to make it too complicated. I have added some comments so it is easier to understand what is tested.

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: fix v4

Comment 10

[Josh Matthews [:jdm]]

Using PLDHashTable in new code is strongly discouraged. Can we use one of the types from https://developer.mozilla.org/en-US/docs/XPCOM_hashtable_guide instead?

Comment 11

[Dragana Damjanovic [:dragana]]

(In reply to Josh Matthews [:jdm] from comment #10)
> Using PLDHashTable in new code is strongly discouraged. Can we use one of
> the types from
> https://developer.mozilla.org/en-US/docs/XPCOM_hashtable_guide instead?


We need a hash key that is a combination of a string, a uint16 and a pointer, similar as in nsHostResolver

Comment 12

[Steve Workman [:sworkman] (INACTIVE)]

Ah, I wasn't of that Josh. Dragana, nsHttpConnectionMgr uses an nsClassHashTable for the connection table, mCT. It uses a string hash key defined in nsHttpConnectionInfo - check it out. A string is formed from the hostname + some flags. Now, I don't think we can use that, but it's an example of nsClassHashTable. The link that Josh suggested mentions that you can create your own hash key class if needed - we already have nsHostKey, so let's see if we can't make that work.

Can you look at the definition for nsStringHashKey and see if you change struct nsHostKey to be a class nsHostKey : public PDLHashEntry?

Comment 13

[Dragana Damjanovic [:dragana]]

Attachment: fix v4

Since we are using this hash key only here, I implemented a simple solution, something I have suggested sometime ago. nsHttpConnectionInfo use the same thing.

I have not used nsHostKey because a lot of changes would be needed in nsHostResolver. :)

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 8460084 [details] [diff] [review]
fix v4

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

tiny drive by comment - didn't read the whole patch

::: netwerk/dns/ChildDNSService.cpp
@@ +49,5 @@
>  ChildDNSService::~ChildDNSService()
>  {
>  }
> +nsCString
> +ChildDNSService::getDNSRecordHashKey(nsCString host,

const nsctring &host - avoid all the new ctors

@@ +53,5 @@
> +ChildDNSService::getDNSRecordHashKey(nsCString host,
> +                                     uint32_t flags,
> +                                     nsIDNSListener* listener)
> +{
> +  nsCString key;

nsautocstring

Comment 15

[Dragana Damjanovic [:dragana]]

Attachment: fix v4

Comment 16

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8460192 [details] [diff] [review]
fix v4

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

Almost there... :)

::: netwerk/dns/ChildDNSService.cpp
@@ +42,5 @@
>  
>  ChildDNSService::ChildDNSService()
>    : mFirstTime(true)
>    , mOffline(false)
> +  , mPendingRequestsLock("DNSPandingRequestsLock")

s/DNSPandingRequestsLock/DNSPendingRequestsLock/

@@ +63,5 @@
> +  key.Append(host.get());
> +  key.AppendLiteral("flags:");
> +  key.AppendInt(flags);
> +  key.AppendPrintf("listener:%p", listener);
> +  return key;

Do you need the label strings "host:", "flags:" and "listener:" here?

@@ +190,5 @@
> +    }
> +  }
> +
> +  MutexAutoLock lock(mPendingRequestsLock);
> +  mPendingRequests.Remove(getDNSRecordHashKey(nsCString(aHostname),

I think we need to have a counter for the number of times a request was made with the same listener. This current patch will store one request per listener per host/flags. And for most cases this should be fine. However, DNSService allows for multiple requests for the same host/flags/listener combination. Each entry in nsHostResolver's hash table has a list of callbacks, and there's nothing at present to stop callbacks from being duplicates of each other.

nsHTMLDNSPrefetch is an example. It may request the same hostname to be resolved with the same flags, yet it only has one static listener. If multiple links are removed from the page, we need to remove the exact number of callbacks from the hashtable's entry in nsDNSService. If a single link is removed from a page, we want to remove one of those requests from the callback list, not all of them.

So, we need to be able to keep multiple hostname/flags/listener entries. I think a counter per entry should be enough here, because of the way CancelAsyncRequest works. So, when you currently call mPendingRequests.Put, you should check if there's an existing entry, and then increment the counter. Then decrement here in NotifyRequestDone; the entry can be removed when the counter reaches 0. Kinda like refcounting.

Caveat: I *think* nsRefPtrHashtable only keeps one refcount to the object. It looks like that from a quick reading of the code - Remove just removes it from the table and I think the caller is responsible for Releasing it. Anyway, check that out.

::: netwerk/dns/ChildDNSService.h
@@ +39,4 @@
>  private:
>    virtual ~ChildDNSService();
>  
> +  nsAutoCString getDNSRecordHashKey(const nsCString &host, uint32_t flags, nsIDNSListener* listener);

Here, I would suggest the following:

void MOZ_ALWAYS_INLINE GetDNSRecordHashKey(const nsACString &aHost, uint32_t aFlags, nsIDNSListener* aListener, nsACString &aHashKey);

This return by reference avoids copying from the local nsAutoCString to the return nsAutoCString. AND the caller of the function can use whatever nsACString type.

(nits: UpperCaseCamelCase for function names; 'aArg' for params/args).

::: netwerk/dns/DNSRequestChild.cpp
@@ +152,5 @@
> +
> +class CancelDNSRequestEvent : public nsRunnable
> +{
> +public:
> +  CancelDNSRequestEvent(DNSRequestChild* dnsReq, nsresult aReason)

s/dnsReq/aDnsReq/

@@ +153,5 @@
> +class CancelDNSRequestEvent : public nsRunnable
> +{
> +public:
> +  CancelDNSRequestEvent(DNSRequestChild* dnsReq, nsresult aReason)
> +    : mDnsReqest(dnsReq)

s/Reqest/Request/

::: netwerk/test/unit/test_dns_cancel.js
@@ +16,5 @@
> +      do_check_eq(inStatus, Cr.NS_ERROR_ABORT);
> +
> +      do_test_finished();
> +    }
> +    else if (inRequest == request2) {

} else if (...) {  // same line.

@@ +58,5 @@
> +  request3 = dns.asyncResolve(hostname, 0, listener2, mainThread);
> +
> +  // Immediately cancel request using dns.cancelAsyncResolve (this function
> +  // needs to be tested).
> +  dns.cancelAsyncResolve(hostnameToCancel, 0, listener1, Cr.NS_ERROR_ABORT);

Still need to add a 4th case to test request4.Cancel(Cr.NS_ERRROR_ABORT);

Comment 17

[Dragana Damjanovic [:dragana]]

Attachment: fix v5

Comment 18

[Dragana Damjanovic [:dragana]]

> 
> @@ +190,5 @@
> > +    }
> > +  }
> > +
> > +  MutexAutoLock lock(mPendingRequestsLock);
> > +  mPendingRequests.Remove(getDNSRecordHashKey(nsCString(aHostname),
> 
> I think we need to have a counter for the number of times a request was made
> with the same listener. This current patch will store one request per
> listener per host/flags. And for most cases this should be fine. However,
> DNSService allows for multiple requests for the same host/flags/listener
> combination. Each entry in nsHostResolver's hash table has a list of
> callbacks, and there's nothing at present to stop callbacks from being
> duplicates of each other.
> 
> nsHTMLDNSPrefetch is an example. It may request the same hostname to be
> resolved with the same flags, yet it only has one static listener. If
> multiple links are removed from the page, we need to remove the exact number
> of callbacks from the hashtable's entry in nsDNSService. If a single link is
> removed from a page, we want to remove one of those requests from the
> callback list, not all of them.
> 
> So, we need to be able to keep multiple hostname/flags/listener entries. I
> think a counter per entry should be enough here, because of the way
> CancelAsyncRequest works. So, when you currently call mPendingRequests.Put,
> you should check if there's an existing entry, and then increment the
> counter. Then decrement here in NotifyRequestDone; the entry can be removed
> when the counter reaches 0. Kinda like refcounting.
> 
> Caveat: I *think* nsRefPtrHashtable only keeps one refcount to the object.
> It looks like that from a quick reading of the code - Remove just removes it
> from the table and I think the caller is responsible for Releasing it.
> Anyway, check that out.
> 


I cannot use just counter because all DNSRequestChild-s are different objects. I used array (I hope I got the right one :) ). The other possibility is to move all this logic of more of the same (host, flags, listener) requests into DNSRequestChild/Parent, there is less objects constructed, but I think this is more complicated. Are such "duplicate" request happening very often?

> 
> @@ +58,5 @@
> > +  request3 = dns.asyncResolve(hostname, 0, listener2, mainThread);
> > +
> > +  // Immediately cancel request using dns.cancelAsyncResolve (this function
> > +  // needs to be tested).
> > +  dns.cancelAsyncResolve(hostnameToCancel, 0, listener1, Cr.NS_ERROR_ABORT);
> 
> Still need to add a 4th case to test request4.Cancel(Cr.NS_ERRROR_ABORT);

I added 2 more test, but I had to use something else not localhost(it is too fast). Hosts does not need to be resolved just not canceled, can I count on that?

Comment 19

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8461005 [details] [diff] [review]
fix v5

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

::: netwerk/dns/ChildDNSService.cpp
@@ +119,5 @@
> +      ent->AppendElement(childReq);
> +    } else {
> +      ent = new nsTArray<nsRefPtr<DNSRequestChild>>();
> +      ent->AppendElement(childReq);
> +      mPendingRequests.Put(key, ent);

Very good. This looks great.

minor: s/ent/hashEntry/ - just a little clearer that we're getting/putting entries into a hash table. Same elsewhere in the file, please.

@@ +145,5 @@
> +  nsCString key;
> +  GetDNSRecordHashKey(aHostname, aFlags, aListener, key);
> +  if (mPendingRequests.Get(key, &ent)) {
> +    // We cancel just one.
> +    ent->ElementAt(0)->Cancel(aReason);

Cool.

@@ +200,5 @@
> +  nsTArray<nsRefPtr<DNSRequestChild>> *ent;
> +
> +  if (mPendingRequests.Get(key, &ent)) {
> +    int inx;
> +    if ((inx = ent->IndexOf(aDnsRequest))) {

s/inx/idx/

@@ +202,5 @@
> +  if (mPendingRequests.Get(key, &ent)) {
> +    int inx;
> +    if ((inx = ent->IndexOf(aDnsRequest))) {
> +      ent->RemoveElementAt(inx);
> +      if (ent->Length() == 0) {

There is also an IsEmpty() function - it does the same check though :)

::: netwerk/dns/DNSRequestChild.cpp
@@ +161,5 @@
> +  NS_IMETHOD Run()
> +  {
> +    // Send request to Parent process.
> +    mDnsRequest->SendCancelDNSRequest(mDnsRequest->mHost, mDnsRequest->mFlags,
> +                                    mReasonForCancel);

nit: indentation.

::: netwerk/test/unit/test_dns_cancel.js
@@ +63,5 @@
> +  var threadManager = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager);
> +  var mainThread = threadManager.currentThread;
> +
> +  // This one will be canceled with cancelAsyncResolve.
> +  requestList1Canceled1 = dns.asyncResolve(hostname2, 0, listener1, mainThread);

Not sure about using real hostnames here. But we can check with Nathan Froyd about that.

In the meantime, some things to try:

Pass in nsIDNSService.RESOLVE_BYPASS_CACHE as one of the flags. This will ensure that the OS resolver is called on a separate thread, and thus that OnLookupComplete is not called in the same context as asyncResolve. Probably not perfect though - no guarantee that OnLookupComplete won't be called immediately after asyncResolve.

Nonetheless, try calling cancelAsyncResolve immediately after, rather than waiting until the end of run_test.

See what happens with multiple test runs on the try server. If we get intermittent failures, that's not good.

I'm not sure it's perfect though - there might still be races, because the OS isn't going to take long to return an IP address for localhost. Still, the context switching

@@ +65,5 @@
> +
> +  // This one will be canceled with cancelAsyncResolve.
> +  requestList1Canceled1 = dns.asyncResolve(hostname2, 0, listener1, mainThread);
> +
> +  // This one will no canceled.

... not be canceled.

@@ +72,5 @@
> +  // This one will be canceled with cancel(Cr.NS_ERROR_ABORT).
> +  requestList1Canceled2 = dns.asyncResolve(hostname1, 0, listener1, mainThread);
> +  requestList1Canceled2.cancel(Cr.NS_ERROR_ABORT);
> +
> +  // This one is no canceled.

... will not be canceled.

Comment 20

[Steve Workman [:sworkman] (INACTIVE)]

Nathan - I know we're not supposed to open external connections in automated tests, but what is the situation with DNS requests? Is there a local DNS server setup, or are the requests blocked or what?

Some background: Dragana is writing tests to verify that a DNS cancel function is working correctly, but lookups for localhost are getting responses too quickly. Is it ok if she writes an xpcshell test that asks for an IP address for mozilla.org?

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to Steve Workman [:sworkman] from comment #20)
> Nathan - I know we're not supposed to open external connections in automated
> tests, but what is the situation with DNS requests? Is there a local DNS
> server setup, or are the requests blocked or what?

DNS is OK regardless, if I understand correctly, because the DNS requests will never go through the Necko codepath that blocks non-local connects.  The crash messages for connections for non-local hostnames are capable of displaying IP addresses, so we are resolving those hostnames, even if we can't connect to them.

Simply asking for an IP address for mozilla.org should be just fine.

Comment 22

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8461005 [details] [diff] [review]
fix v5

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

Thanks, Nathan.

Dragana, since Nathan has ok'd DNS requests, go ahead and use mozilla.org etc. in your tests. r=me with those nits fixed :)

Comment 23

[Dragana Damjanovic [:dragana]]

Attachment: fix 6

thanks.

Comment 24

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8466245 [details] [diff] [review]
fix 6

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

The additional changes you made to ReleaseIPDLReference look more or less right. There's just a simpler way to do it :)

r=me with that fixed.

::: netwerk/dns/DNSRequestChild.cpp
@@ +260,5 @@
> +  // Request is done or destroyed. Remove it from the hash table.
> +  ChildDNSService *dnsServiceChild =
> +    already_AddRefed<ChildDNSService>(ChildDNSService::GetSingleton()).take();
> +  dnsServiceChild->NotifyRequestDone(this);
> +  NS_RELEASE(dnsServiceChild);

nsRefPtr<ChildDNSService> childDNSService =
  dont_AddRef(ChildDNSService::GetSingleton());

The pointer will be released automatically when it goes out of scope.

How did this compile and work in the previous patch?

Comment 25

[Dragana Damjanovic [:dragana]]

> 
> ::: netwerk/dns/DNSRequestChild.cpp
> @@ +260,5 @@
> > +  // Request is done or destroyed. Remove it from the hash table.
> > +  ChildDNSService *dnsServiceChild =
> > +    already_AddRefed<ChildDNSService>(ChildDNSService::GetSingleton()).take();
> > +  dnsServiceChild->NotifyRequestDone(this);
> > +  NS_RELEASE(dnsServiceChild);
> 
> nsRefPtr<ChildDNSService> childDNSService =
>   dont_AddRef(ChildDNSService::GetSingleton());
> 
> The pointer will be released automatically when it goes out of scope.
> 
> How did this compile and work in the previous patch?


I do not know, but it did :). It also compiles on try server on some machines ...

Comment 26

[Dragana Damjanovic [:dragana]]

Attachment: fix 7

Comment 27

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8469085 [details] [diff] [review]
fix 7

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

Looks good r=me.

Comment 28

[Dragana Damjanovic [:dragana]]

https://tbpl.mozilla.org/?tree=Try&rev=2268a331be07

Comment 29

[Ryan VanderMeulen [:RyanVM]]

(In reply to Dragana Damjanovic from comment #28)
> https://tbpl.mozilla.org/?tree=Try&rev=2268a331be07

Those mochitest-e10s failures look pretty consistent, no?

Comment 30

[Dragana Damjanovic [:dragana]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #29)
> (In reply to Dragana Damjanovic from comment #28)
> > https://tbpl.mozilla.org/?tree=Try&rev=2268a331be07
> 
> Those mochitest-e10s failures look pretty consistent, no?

I will take a look

Comment 31

[Dragana Damjanovic [:dragana]]

Attachment: fix 8

Comment 32

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Dragana Damjanovic from comment #31)
> Created attachment 8475954 [details] [diff] [review]
> fix 8

Hi Dragana - Can you provide some explanation about the changes here? I know we talked offline last week, but it's not clear to me from the diff why you need these changes. Thanks!

Comment 33

[Dragana Damjanovic [:dragana]]

(In reply to Steve Workman [:sworkman] from comment #32)
> (In reply to Dragana Damjanovic from comment #31)
> > Created attachment 8475954 [details] [diff] [review]
> > fix 8
> 
> Hi Dragana - Can you provide some explanation about the changes here? I know
> we talked offline last week, but it's not clear to me from the diff why you
> need these changes. Thanks!

The oranges was due to the child sending a cancelRequest and in the same time (while this message is queued) the parent calls __delete__. I think this was the reason for the tests to fail. 
The problem was that only the child can end the communication after receiving on LookupComplete and not the parent.
Sorry that i have not explained, the reasons are really not that obvious.

Comment 34

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8475954 [details] [diff] [review]
fix 8

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

Thanks for the explanation. Looks good. Couple of minor questions; r=me.

::: netwerk/dns/DNSRequestChild.cpp
@@ +251,5 @@
>        NS_NewRunnableMethod(this, &DNSRequestChild::CallOnLookupComplete);
>      mTarget->Dispatch(event, NS_DISPATCH_NORMAL);
>    }
>  
> +  Send__delete__(this);

Do you need to use "unused <<" here?

::: netwerk/dns/DNSRequestParent.cpp
@@ +43,5 @@
>                             getter_AddRefs(unused));
>    }
>  
>    if (NS_FAILED(rv) && !mIPCClosed) {
> +    unused << SendLookupCompleted(DNSRequestResponse(rv));

mIPCClosed = true?

Comment 35

[Dragana Damjanovic [:dragana]]

Attachment: fix v 9

Comment 36

[Dragana Damjanovic [:dragana]]

https://tbpl.mozilla.org/?tree=Try&rev=1937b87975d3

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd877592b35

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6dd877592b35