898237 - Proxy Settings Test Diagnostic Tool implementation

Status: RESOLVED INCOMPLETE

(Core :: Networking, defect)

Description

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolv0.1.patch

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'm working on Networking Dashboard -> project is filed here: https://wiki.mozilla.org/Necko/Dashboard
This is the implementation of Proxy Settings Test Diagnostic Tool.

This version wasn't tested yet. It's intented for feedback from my mentor.

Comment 1

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolv2.patch

Dashboard implements now also nsiProxyInfo interface.
Still get  undefined reference to all functions from the interfaces.

Comment 2

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

Comment on attachment 782009 [details] [diff] [review]
ProxySettingsDiagnosticToolv2.patch

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

::: netwerk/base/public/nsIDashboard.idl
@@ +40,5 @@
>      /* When true, the service will log websocket events */
>      attribute boolean enableLogging;
> +
> +    /* Request Proxy Settings Information for URL */
> +    void requestProxySettings(in nsIURI uri, in NetDashboardCallback cb);

Have the URI be a string at this point. You can convert it to a nsURI in the implementation.

::: netwerk/base/src/Dashboard.cpp
@@ +423,5 @@
> +
> +    }
> +    else {
> +        answer = false;
> +        error.Assign(NS_LITERAL_STRING("THE PROXY SETTINGS TEST HAS FAILED"));

We will need the actual error for this.

::: netwerk/base/src/Dashboard.h
@@ +120,5 @@
> +    {
> +        nsCOMPtr<nsIProtocolProxyService> serv;
> +        nsCOMPtr<NetDashboardCallback> cb;
> +        nsCOMPtr<nsICancelable> mCancel;
> +        nsIThread* thread;   

Trailing white space.

Comment 3

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolv3.patch

Made changes that were specified in feedback

Comment 4

[Catalin Iordache]

Also there are no more errors on build; code now compiles
Still not tested.

Comment 5

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticTool4.patch

In case of error, callback will become now null;
Changed service for proxy-service;
This version was tested and it's working as intended.

Comment 6

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolv5.patch

Nsresult string error resolved, thanks to Robert Bindars patch;
Patch was rebased -> ready for review

Comment 7

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

Comment on attachment 785517 [details] [diff] [review]
ProxySettingsDiagnosticToolv5.patch

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

::: netwerk/base/public/nsIDashboard.idl
@@ +4,5 @@
>  
>  #include "nsISupports.idl"
>  #include "nsIDashboardEventNotifier.idl"
>  
> +interface nsIURI;

You don't need nsIURI

::: netwerk/base/src/Dashboard.cpp
@@ +481,5 @@
> +            return rv;
> +        }
> +    }
> +
> +    nsIURI *uri;

use a nsCOMPtr<nsIURI> here

@@ +483,5 @@
> +    }
> +
> +    nsIURI *uri;
> +    nsCOMPtr<nsIIOService> ioService = mozilla::services::GetIOService();
> +    rv = ioService->NewURI(uriString, nullptr, nullptr, &uri);

Use NS_NewURI(getter_AddRefs(uri), uriString);
http://mxr.mozilla.org/mozilla-central/source/content/base/src/WebSocket.cpp#976

@@ +510,5 @@
> +    nsString &type = dict.mType.Value();
> +    nsString &error = dict.mError.Value();
> +    bool &answer = dict.mAnswer.Value();
> +
> +    if (!NS_FAILED(status) && pi != nullptr) {

Use NS_SUCCEEDED instead of !NS_FAILED

@@ +517,5 @@
> +        pi->GetHost(stringaux);
> +        hostname = NS_ConvertASCIItoUTF16(stringaux);
> +        pi->GetType(stringaux);
> +        type = NS_ConvertASCIItoUTF16(stringaux);
> +        int32_t intaux;

You can use dict.mPort directly here.

Comment 8

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolV6.patch

Made changes that were specified in review. (not using intaux can't be done due to GetPort function, which wants an int32_t and mPort which is uint32_t)

Comment 9

[Patrick McManus [:mcmanus]]

Comment on attachment 787780 [details] [diff] [review]
ProxySettingsDiagnosticToolV6.patch

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

I'm sorry about the review lag - I somehow lost track of the notification that this was waiting for me. (I saw it originally but somehow lost it along the way)

is there another patch that calls this new binding?

::: netwerk/base/src/Dashboard.cpp
@@ +465,5 @@
>      return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +Dashboard::RequestProxySettings(const nsAString &uriString, NetDashboardCallback* cb)

type *var, not type* var

@@ +487,5 @@
> +    if (NS_FAILED(rv)) {
> +        mProxy.cb = nullptr;
> +        return rv;
> +    }
> +    mProxy.serv->AsyncResolve(uri, 0, this, getter_AddRefs(mProxy.mCancel));

what if this NS_FAILED ?

@@ +529,5 @@
> +        mProxy.cb = nullptr;
> +        return NS_ERROR_FAILURE;
> +    }
> +    mProxy.cb->OnDashboardDataAvailable(val);
> +    mProxy.cb = nullptr;

I would make this the first thing in the function so you don't need to worry about an early return (and you can remove the duplicated one a couple lines eariler)

also release the stored cancelable reference at that point.

::: netwerk/base/src/Dashboard.h
@@ +138,5 @@
> +    struct ProxySettingsInfo
> +    {
> +        nsCOMPtr<nsIProtocolProxyService> serv;
> +        nsCOMPtr<NetDashboardCallback> cb;
> +        nsCOMPtr<nsICancelable> mCancel;

shouldn't this cancel object be used in the dtor if there is a request outstanding?

I think that should be true for the DNS cancel object too (not part of this patch - but I've just been reading this module).

@@ +139,5 @@
> +    {
> +        nsCOMPtr<nsIProtocolProxyService> serv;
> +        nsCOMPtr<NetDashboardCallback> cb;
> +        nsCOMPtr<nsICancelable> mCancel;
> +        nsIThread* thread;

I don't think the thread member is used anywhere. also put the * on the variable.

@@ +140,5 @@
> +        nsCOMPtr<nsIProtocolProxyService> serv;
> +        nsCOMPtr<NetDashboardCallback> cb;
> +        nsCOMPtr<nsICancelable> mCancel;
> +        nsIThread* thread;
> +    };

in general these structs would be a little nicer as

struct foo
{
  vars;
} mFoo;

instead of declaring the struct in one place and a declaration of a member in another. But make sure to fix all of them instead of just the new one - a mix would be worse :)

also they seem a little confused if they should use mVar or just var notation - going back and forth. just choose one.

Comment 10

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolV7.patch

NetDashboardCallback* cb was modified to NetDashboardCallback *cb;
I can't make mProxy.cb null at the beginning of OnProxyAvailable function because I need cb. I choose to make nsICancelable request null first and cb where it was done before;
In dtor I made cancel object null; For DNS Lookup Tool I will file another patch;
I've decided to keep the structures as they are for this patch and make another one were I would change all structures as Patrick said, to make them a little nicer

Comment 11

[Patrick McManus [:mcmanus]]

Comment on attachment 791658 [details] [diff] [review]
ProxySettingsDiagnosticToolV7.patch

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

in the previous review I asked if there was another patch that makes use of this binding, and if so where is it?

::: netwerk/base/public/nsIDashboard.idl
@@ +51,5 @@
>       * aHost: host name */
>      void requestDNSLookup(in ACString aHost, in NetDashboardCallback cb);
> +
> +    /* Request Proxy Settings Information for URL */
> +    void requestProxySettings(in AString uriString, in NetDashboardCallback cb);

ACString

::: netwerk/base/src/Dashboard.cpp
@@ +25,5 @@
>  }
>  
>  Dashboard::~Dashboard()
>  {
> +    mProxy.cancel = nullptr;

You actually need to call cancel() on this :)

@@ +486,5 @@
> +    if (NS_FAILED(rv)) {
> +        mProxy.cb = nullptr;
> +        return rv;
> +    }
> +    mProxy.serv->AsyncResolve(uri, 0, this, getter_AddRefs(mProxy.cancel));

you didn't address this review comment. What if AsyncResolve() fails?

move mProxy.cb = cb down to right before the return NS_OK; then you can just do the early return thing easier.

::: netwerk/base/src/Dashboard.h
@@ +130,4 @@
>      struct DnsLookup
>      {
>          nsCOMPtr<nsIDNSService> serv;
>          nsCOMPtr<nsICancelable> mCancel;

this cancel event is also not ->Cancel()'d in the dtor.. please fix that up in this patch or file another one.

Comment 12

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolV8.patch

There is another patch that makes use of this binding and which I'll soon put it here.
For DNS lookup tool I will file another bug

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 792405 [details] [diff] [review]
ProxySettingsDiagnosticToolV8.patch

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

::: netwerk/base/src/Dashboard.cpp
@@ +25,5 @@
>  }
>  
>  Dashboard::~Dashboard()
>  {
> +    mProxy.cancel->Cancel(NS_ERROR_ABORT);

missing null check

@@ +496,5 @@
> +Dashboard::OnProxyAvailable(nsICancelable *request, nsIURI *uri, nsIProxyInfo *pi, nsresult status)
> +{
> +    AutoSafeJSContext cx;
> +
> +    request = nullptr;

I think you mean mProxy.cancel = nullptr.. you can also assert request == mProxy.cancel

Comment 14

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolV9.patch

Comment 15

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolV9.patch

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 792462 [details] [diff] [review]
ProxySettingsDiagnosticToolV9.patch

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

please make the change I indicate for OnProxyAvaialbe. You don't need to re-request a new review.

as we discussed please don't checkin without a reviewed frontend patch too.

::: netwerk/base/src/Dashboard.cpp
@@ +25,5 @@
>  }
>  
>  Dashboard::~Dashboard()
>  {
> +    if (mProxy.cancel != nullptr)

nit: styleguide says to just do if (mProxy.cancel)

@@ +497,5 @@
> +Dashboard::OnProxyAvailable(nsICancelable *request, nsIURI *uri, nsIProxyInfo *pi, nsresult status)
> +{
> +
> +    if (request == mProxy.cancel)
> +        mProxy.cancel = nullptr;

MOZ_ASSERT(request == mProxy.cancel);
mProxy.cancel = nullptr;

Comment 17

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolBackendPart.patch

Backend part: done.

Comment 18

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolBackendPart.patch

It looks like mProxy.cb has to be != nullptr when AsyncResolve it's called. Also, because of the coding style, pi it's better than pi != nullptr

Comment 19

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolFrontendPart.patch

Frontend of Proxy Settings Diagnostic Tool

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to Catalin Iordache from comment #18)
> Created attachment 793595 [details] [diff] [review]
> ProxySettingsDiagnosticToolBackendPart.patch
> 
> It looks like mProxy.cb has to be != nullptr when AsyncResolve it's called.

it looks like OnProxyAvailable() is called on the AsyncResolve() stack and you deref cb in OnProxyAvailable.. I actually didn't think that was possible for that api. interesting - but in general you want to allow that as an optimization (it obviously won't do that all the time - sometimes it really needs to block and be async, like when PAC resolution is used) otherwise you force a trip through the event queue for no reason - and the event queue can be a long and lonely trip.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 793598 [details] [diff] [review]
ProxySettingsDiagnosticToolFrontendPart.patch

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

This patch as well should be based off the patch from bug 905325 and this bug should then probably depend on it.

::: toolkit/content/aboutNetworking.js
@@ +176,5 @@
> +function proxySettingsTest() {
> +  let uri = document.getElementById("proxyInput").value;
> +  if (!uri)
> +    return;
> +  gDashboard.requestProxySettings(uri, proxySettingsTestCallback);

Nit: if (uri) { ... }

::: toolkit/content/aboutNetworking.xhtml
@@ +100,5 @@
>              </table>
>          </div>
> +
> +        <div id="proxysettingstest" class="tab">
> +            <button onclick="proxySettingsTest()">&aboutNetworking.proxySettingsTest;</button>

This listener should be registered in the JS file.

::: toolkit/locales/en-US/chrome/global/aboutNetworking.dtd
@@ +29,5 @@
>  <!ENTITY aboutNetworking.messagesReceived      "Messages Received">
>  <!ENTITY aboutNetworking.bytesSent             "Bytes Sent">
>  <!ENTITY aboutNetworking.bytesReceived         "Bytes Received">
> +<!ENTITY aboutNetworking.proxy				   "Proxy">
> +<!ENTITY aboutNetworking.proxySettingsTest	   "Test">

Nit: indentation is off.

Comment 22

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolFrontendPartV2.patch

Made changes that were specified in feedback. Also if NS_NewUri or AsyncResolve would fail, I made up something for printing the failed error

Comment 24

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticTool.patch

I thought about implementing also the backend part in JS. Also in this way the rechability of a proxy is tested too.

Comment 25

[Patrick McManus [:mcmanus]]

Comment on attachment 798623 [details] [diff] [review]
ProxySettingsDiagnosticTool.patch

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

I didn't review this line by line.. but the approach seems good.

::: toolkit/content/aboutNetworking.js
@@ +227,5 @@
> +    return this;
> +  },
> +
> +  onProxyAvailable : function (inRequest, inUri, inRecord, inStatus) {
> +    this.nextFunction(inRecord);

probably need to check inStatus here

@@ +233,5 @@
> +};
> +
> +function doProxy() {
> +  let url = document.getElementById("pingUrl").value;
> +  var uri = gIOService.newURI(url, null, gIOService.newURI("http://", null, null));

we need, at least the option, of looking up arbitrary urls. proxy resolution can change based on scheme, port, url, etc.. not just hostname. though I think its fine to have the default do just what you have here.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 798623 [details] [diff] [review]
ProxySettingsDiagnosticTool.patch

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

Same thing as for bug 907050. Do we really want to have this feature as a tab in between live data that may refresh automatically? It might make sense to introduce another section below the current one that is named 'Toolbox' or the like? That could hold all kinds of different tools like hostname lookup, proxy settings, pings, and stuff you'll want to add in the future that is not table-based data.

Comment 27

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

Using Bug802398 to move the buttons on the left side and split them into sections.