Closed Bug 890513 Opened 7 years ago Closed 6 years ago

Diagnostic tool: DNS Lookup implementation

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: catalinn.iordache, Assigned: catalinn.iordache)

Details

Attachments

(1 file, 8 obsolete files)

Attached patch dnslookup.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130627161625

Steps to reproduce:

I've implemented DNS Lookup diagnostic tool. I have modified nsIDashboard.idl, Dashboard.cpp and Dashboard.h
Attachment #771637 - Flags: review?(valentin.gosu)
Attachment #771637 - Flags: review?(mcmanus)
Comment on attachment 771637 [details] [diff] [review]
dnslookup.patch

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

Hi Catalin,

This implementation is not yet complete.
Ping me on IRC if you need more assistance.

::: netwerk/base/public/nsIDashboard.idl
@@ +39,5 @@
>      attribute boolean enableLogging;
> +
> +    /* DNS resolver for host name
> +     * aHost: host name */
> +    void RequestDNSLookup(in ACString aHost);

Methods in the idl should start with a lower case letter.

::: netwerk/base/src/Dashboard.cpp
@@ +354,5 @@
>  NS_IMETHODIMP
> +Dashboard::RequestDNSLookup(const nsACString &aHost) {
> +
> +    nsresult rv;
> +    nsICancelable *cancel;

You might want to save this object in the dashboard, outside this method.

@@ +355,5 @@
> +Dashboard::RequestDNSLookup(const nsACString &aHost) {
> +
> +    nsresult rv;
> +    nsICancelable *cancel;
> +    nsCOMPtr<nsPIDNSService> dns = do_GetService(NS_DNSSERVICE_CONTRACTID);

You can use mDns.serv here. ( Similar to http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/Dashboard.cpp#361 )

@@ +363,5 @@
> +
> +NS_IMETHODIMP
> +Dashboard::OnLookupComplete(nsICancelable *aRequest, nsIDNSRecord *aRecord, nsresult aStatus) {
> +
> +    return aStatus;

This method should do something.
Attachment #771637 - Flags: review?(valentin.gosu)
Comment on attachment 771637 [details] [diff] [review]
dnslookup.patch

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

I'll delegate this to valentin
Attachment #771637 - Flags: review?(mcmanus)
Attached patch dnslookupv2.patch (obsolete) — Splinter Review
Make changes that were specified in the review, over the first patch
Attachment #771776 - Flags: review?(valentin.gosu)
Attachment #771776 - Flags: review?(mcmanus)
Comment on attachment 771776 [details] [diff] [review]
dnslookupv2.patch

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

::: netwerk/base/src/Dashboard.cpp
@@ +357,5 @@
> +        mDns.serv = do_GetService("@mozilla.org/network/dns-service;1", &rv);
> +        if(NS_FAILED(rv))
> +            return rv;
> +    }
> +    mDns.serv->AsyncResolve(aHost, 0, this, nullptr, &mDns.mAction);

use a nsCOMPtr and getter_Addrefs for nsICancelable

@@ +358,5 @@
> +        if(NS_FAILED(rv))
> +            return rv;
> +    }
> +    mDns.serv->AsyncResolve(aHost, 0, this, nullptr, &mDns.mAction);
> +    printf("DNS RESOLVER CALL\n");

dont leave this in

@@ +363,5 @@
> +    return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +Dashboard::OnLookupComplete(nsICancelable *aRequest, nsIDNSRecord *aRecord, nsresult aStatus) {

doesn't the dashboard need to do something useful here?

also clear the cancelable event

@@ +367,5 @@
> +Dashboard::OnLookupComplete(nsICancelable *aRequest, nsIDNSRecord *aRecord, nsresult aStatus) {
> +
> +    if(NS_FAILED(aStatus)) {
> +
> +        printf("DNS RESOLVER: FAILED\n");

dont leave this in

@@ +368,5 @@
> +
> +    if(NS_FAILED(aStatus)) {
> +
> +        printf("DNS RESOLVER: FAILED\n");
> +        return aStatus;

you want to return NS_OK, because your callback copleted ok.. its just the resolution was unsuccessful.

@@ +371,5 @@
> +        printf("DNS RESOLVER: FAILED\n");
> +        return aStatus;
> +    }
> +
> +    printf("DNS RESOLVER: IT WORKS!\n");

dont leave this in

::: netwerk/base/src/Dashboard.h
@@ +104,5 @@
>      {
>          nsCOMPtr<nsIDNSService> serv;
>          nsTArray<DNSCacheEntries> data;
>          nsCOMPtr<NetDashboardCallback> cb;
> +        nsICancelable *mAction;

nscomptr, and probably wants to be called mCancel.. and I would expect the dtor to call mAction->Cancel() if mAction wasn't null.
Attachment #771776 - Flags: review?(mcmanus) → review-
Attached patch DNS LookUp diagnostic tool v0.3 (obsolete) — Splinter Review
Made changes that were specified by Patrick McManus in his review.

This isn't the final product. There is still work to be done.
Attachment #773270 - Flags: review?(valentin.gosu)
Hay Catalin - just fyi "This isn't the final product. There is still work to be done.".. you want to use the feedback? flag for that instead of the review flag simply because a r+ means eligible for checkin.

but posting work in progress patches for feedback is awesome and not done enough - so thanks!
Attachment #773270 - Flags: review?(valentin.gosu) → feedback?(valentin.gosu)
Attachment #773270 - Flags: feedback?(valentin.gosu)
Attachment #773270 - Flags: feedback?(valentin.gosu)
Attachment #771776 - Flags: review?(valentin.gosu)
Patrick McManus - thank you for the advices
Comment on attachment 773270 [details] [diff] [review]
DNS LookUp diagnostic tool v0.3

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

Hi Catalin,

This looks pretty good. Just a fey things regarding the coding style and the next steps you should take.
(Also, when uploading other patches, please mark the old ones as obsolete)

::: netwerk/base/public/nsIDashboard.idl
@@ +36,5 @@
>      void requestDNSInfo(in NetDashboardCallback cb);
>  
>      /* When true, the service will log websocket events */
>      attribute boolean enableLogging;
> +    

Trailing white space.

@@ +39,5 @@
>      attribute boolean enableLogging;
> +    
> +    /* DNS resolver for host name
> +     * aHost: host name */
> +    void requestDNSLookup(in ACString aHost);

Next step: add a callback parameter.

::: netwerk/base/src/Dashboard.cpp
@@ +348,5 @@
>      return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +Dashboard::RequestDNSLookup(const nsACString &aHost) {

Please keep to the same coding style. { should go on the next line.

@@ +362,5 @@
> +}
> +
> +NS_IMETHODIMP
> +Dashboard::OnLookupComplete(nsICancelable *aRequest, nsIDNSRecord *aRecord, nsresult aStatus) {
> +

Next step: This method should create a new JS object (declare it in the webidl), and pass it to the callback function.
Attachment #773270 - Flags: feedback?(valentin.gosu) → feedback+
Call back parameter added for function requestDNSLookup
Other changes that were specified in feedback by Valentin
Attachment #771637 - Attachment is obsolete: true
Attachment #771776 - Attachment is obsolete: true
Attachment #773270 - Attachment is obsolete: true
Attachment #773945 - Flags: feedback?(valentin.gosu)
Comment on attachment 773945 [details] [diff] [review]
DNS LookUp diagnostic tool v0.3.5

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

You can go ahead and create the JS object to pass to the callback.
I think a status message and a sequence<DOMString> for the dns results would be ok for now.

::: netwerk/base/src/Dashboard.cpp
@@ +351,5 @@
>  NS_IMETHODIMP
> +Dashboard::RequestDNSLookup(const nsACString &aHost, NetDashboardCallback* cb)
> +{
> +    if(mDns.cb)
> +        return NS_ERROR_FAILURE;

I would recommend using another field for this callback. Right now, if the dashboard is auto-refreshing, and you trigger a Lookup, it would probably fail at this line.
Attachment #773945 - Flags: feedback?(valentin.gosu) → feedback+
Assignee: nobody → catalinn.iordache
Creating JS object to pass at callback.
Attachment #773945 - Attachment is obsolete: true
Attachment #774793 - Flags: feedback?(valentin.gosu)
Comment on attachment 774793 [details] [diff] [review]
DNS LookUp diagnostic tool v0.4.1

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

::: netwerk/base/public/nsIDashboard.idl
@@ +36,5 @@
>      void requestDNSInfo(in NetDashboardCallback cb);
>  
>      /* When true, the service will log websocket events */
>      attribute boolean enableLogging;
> +    

Remove white space.

::: netwerk/base/src/Dashboard.cpp
@@ +374,5 @@
> +    mozilla::dom::DNSLookupDict dict;
> +    dict.mAddress.Construct();
> +    dict.mError.Construct();
> +    dict.mAnswer.Construct();
> +    

Remove white space.

@@ +380,5 @@
> +    nsString &error = dict.mError.Value();
> +    bool &answer = dict.mAnswer.Value();
> +
> +    if(!NS_FAILED(aStatus)) {
> +        

Remove white space.

@@ +385,5 @@
> +        answer = true;
> +        bool hasMore;
> +        aRecord->HasMore(&hasMore);
> +        while(hasMore) {
> +

Do not leave an empty line after { or before }. It looks weird :)
Attachment #774793 - Flags: feedback?(valentin.gosu) → feedback+
Attached patch DNSLookUpV0.4.2 (obsolete) — Splinter Review
White spaces were removed
Attachment #774793 - Attachment is obsolete: true
Attached patch DNSLookUpV0.4.3 (obsolete) — Splinter Review
boolean answer was initialized (answer = true) if only at callback wasn't returning a error. Now in case of error answer variable will be false, at it should be
Attachment #775607 - Attachment is obsolete: true
Attachment #776626 - Flags: feedback?(valentin.gosu)
Comment on attachment 776626 [details] [diff] [review]
DNSLookUpV0.4.3

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

This looks good. Wait for Robert to submit the nsresult -> String patch, and then ask for r?
Comment on attachment 776626 [details] [diff] [review]
DNSLookUpV0.4.3

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

This looks good. Wait for Robert to submit the nsresult -> String patch, and then ask for r?
Attachment #776626 - Flags: feedback?(valentin.gosu) → feedback+
the new repo contains Robert nsresult string patch -> this patch was rebaised and finished; not tested on try server
Attachment #776626 - Attachment is obsolete: true
Attachment #784459 - Flags: feedback?(valentin.gosu)
Comment on attachment 784459 [details] [diff] [review]
DNSLookupDiagnosticToolVbeta.patch

rebased;
tested;

redy for review
Attachment #784459 - Flags: feedback?(valentin.gosu) → review?(valentin.gosu)
Comment on attachment 784459 [details] [diff] [review]
DNSLookupDiagnosticToolVbeta.patch

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

Just a few coding style issues:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
r=valentin.gosu

::: netwerk/base/src/Dashboard.cpp
@@ +13,5 @@
>  namespace mozilla {
>  namespace net {
>  
> +NS_IMPL_ISUPPORTS5(Dashboard, nsIDashboard, nsIDashboardEventNotifier,
> +                              nsITransportEventSink, nsITimerCallback, 

Trailing white space.

@@ +405,5 @@
>  
> +NS_IMETHODIMP
> +Dashboard::RequestDNSLookup(const nsACString &aHost, NetDashboardCallback* cb)
> +{
> +    if(mDnsup.cb)

if (mDnsup.cp) - please insert a space after the "if". The same for the rest of the if instructions.

@@ +413,5 @@
> +    mDnsup.thread = NS_GetCurrentThread();
> +
> +    if(!mDnsup.serv) {
> +        mDnsup.serv = do_GetService("@mozilla.org/network/dns-service;1", &rv);
> +        if(NS_FAILED(rv)){

Space after "if" and before "{"

@@ +436,5 @@
> +    Sequence<nsString> &addresses = dict.mAddress.Value();
> +    nsString &error = dict.mError.Value();
> +    bool &answer = dict.mAnswer.Value();
> +
> +    if(!NS_FAILED(aStatus)) {

Space after "if"

@@ +447,5 @@
> +           CopyASCIItoUTF16(nextAddress, *addresses.AppendElement());
> +           aRecord->HasMore(&hasMore);
> +        }
> +    }
> +    else {

} else { - same line
Attachment #784459 - Flags: review?(valentin.gosu) → review+
Made changes that were mentioned in review.
r=valentin.gosu

https://tbpl.mozilla.org/?tree=Try&rev=4a51d9e9c0a3
Attachment #784459 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb974275ed1f
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.