Proxy Settings Test Diagnostic Tool implementation

RESOLVED INCOMPLETE

Status

()

Core
Networking
RESOLVED INCOMPLETE
5 years ago
2 years ago

People

(Reporter: Catalin Iordache, Assigned: valentin)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 14 obsolete attachments)

12.38 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 781397 [details] [diff] [review]
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.
(Reporter)

Updated

5 years ago
Attachment #781397 - Flags: feedback?(valentin.gosu)
(Reporter)

Comment 1

5 years ago
Created attachment 782009 [details] [diff] [review]
ProxySettingsDiagnosticToolv2.patch

Dashboard implements now also nsiProxyInfo interface.
Still get  undefined reference to all functions from the interfaces.
Attachment #781397 - Attachment is obsolete: true
Attachment #781397 - Flags: feedback?(valentin.gosu)
Attachment #782009 - Flags: feedback?(valentin.gosu)
(Assignee)

Comment 2

5 years ago
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.
Attachment #782009 - Flags: feedback?(valentin.gosu)
(Reporter)

Comment 3

5 years ago
Created attachment 782279 [details] [diff] [review]
ProxySettingsDiagnosticToolv3.patch

Made changes that were specified in feedback
Attachment #782009 - Attachment is obsolete: true
Attachment #782279 - Flags: feedback?(valentin.gosu)
(Reporter)

Comment 4

5 years ago
Also there are no more errors on build; code now compiles
Still not tested.
(Reporter)

Comment 5

5 years ago
Created attachment 783686 [details] [diff] [review]
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.
Attachment #782279 - Attachment is obsolete: true
Attachment #782279 - Flags: feedback?(valentin.gosu)
Attachment #783686 - Flags: feedback?(valentin.gosu)
(Reporter)

Comment 6

5 years ago
Created attachment 785517 [details] [diff] [review]
ProxySettingsDiagnosticToolv5.patch

Nsresult string error resolved, thanks to Robert Bindars patch;
Patch was rebased -> ready for review
Attachment #783686 - Attachment is obsolete: true
Attachment #783686 - Flags: feedback?(valentin.gosu)
Attachment #785517 - Flags: review?(valentin.gosu)
Attachment #785517 - Flags: review?(mcmanus)
(Assignee)

Comment 7

5 years ago
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.
Attachment #785517 - Flags: review?(valentin.gosu)
(Reporter)

Comment 8

5 years ago
Created attachment 787780 [details] [diff] [review]
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)
Attachment #785517 - Attachment is obsolete: true
Attachment #785517 - Flags: review?(mcmanus)
Attachment #787780 - Flags: review?(valentin.gosu)
Attachment #787780 - Flags: review?(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.
Attachment #787780 - Flags: review?(mcmanus) → review-
(Reporter)

Comment 10

5 years ago
Created attachment 791658 [details] [diff] [review]
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
Attachment #787780 - Attachment is obsolete: true
Attachment #787780 - Flags: review?(valentin.gosu)
Attachment #791658 - Flags: review?(valentin.gosu)
Attachment #791658 - Flags: review?(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.
Attachment #791658 - Flags: review?(mcmanus) → review-
(Reporter)

Comment 12

5 years ago
Created attachment 792405 [details] [diff] [review]
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
Attachment #791658 - Attachment is obsolete: true
Attachment #791658 - Flags: review?(valentin.gosu)
Attachment #792405 - Flags: review?(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
Attachment #792405 - Flags: review?(mcmanus) → review-
(Reporter)

Comment 14

5 years ago
Created attachment 792461 [details] [diff] [review]
ProxySettingsDiagnosticToolV9.patch
Attachment #792405 - Attachment is obsolete: true
Attachment #792461 - Flags: review?(mcmanus)
(Reporter)

Comment 15

5 years ago
Created attachment 792462 [details] [diff] [review]
ProxySettingsDiagnosticToolV9.patch
Attachment #792461 - Attachment is obsolete: true
Attachment #792461 - Flags: review?(mcmanus)
Attachment #792462 - Flags: review?(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;
Attachment #792462 - Flags: review?(mcmanus) → review+
(Reporter)

Comment 17

5 years ago
Created attachment 792645 [details] [diff] [review]
ProxySettingsDiagnosticToolBackendPart.patch

Backend part: done.
Attachment #792462 - Attachment is obsolete: true
(Reporter)

Comment 18

5 years ago
Created attachment 793595 [details] [diff] [review]
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
Attachment #792645 - Attachment is obsolete: true
(Reporter)

Comment 19

5 years ago
Created attachment 793598 [details] [diff] [review]
ProxySettingsDiagnosticToolFrontendPart.patch

Frontend of Proxy Settings Diagnostic Tool
Attachment #793598 - Flags: feedback?(valentin.gosu)
Attachment #793598 - Flags: feedback?(ttaubert)
Attachment #793598 - Flags: feedback?(gavin.sharp)
(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 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.
Attachment #793598 - Flags: feedback?(ttaubert)
Assignee: nobody → catalinn.iordache
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
(Reporter)

Comment 22

5 years ago
Created attachment 793986 [details] [diff] [review]
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
Attachment #793598 - Attachment is obsolete: true
Attachment #793598 - Flags: feedback?(valentin.gosu)
Attachment #793598 - Flags: feedback?(gavin.sharp)
Attachment #793986 - Flags: feedback?(ttaubert)
(Reporter)

Updated

5 years ago
Blocks: 905325
(Reporter)

Updated

5 years ago
No longer blocks: 905325
Depends on: 905325

Updated

5 years ago
Duplicate of this bug: 910941
(Reporter)

Comment 24

5 years ago
Created attachment 798623 [details] [diff] [review]
ProxySettingsDiagnosticTool.patch

I thought about implementing also the backend part in JS. Also in this way the rechability of a proxy is tested too.
Attachment #793595 - Attachment is obsolete: true
Attachment #793986 - Attachment is obsolete: true
Attachment #793986 - Flags: feedback?(ttaubert)
Attachment #798623 - Flags: review?(ttaubert)
Attachment #798623 - Flags: feedback?(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.
Attachment #798623 - Flags: feedback?(mcmanus) → feedback+
(Reporter)

Updated

5 years ago
Attachment #798623 - Flags: review?(ttaubert)
Attachment #798623 - Flags: feedback?(ttaubert)
Attachment #798623 - Flags: feedback+
(Reporter)

Updated

5 years ago
Attachment #798623 - Flags: feedback?(ttaubert) → review?(ttaubert)
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.
Attachment #798623 - Flags: review?(ttaubert)
(Assignee)

Comment 27

4 years ago
Using Bug802398 to move the buttons on the left side and split them into sections.
Assignee: catalinn.iordache → valentin.gosu
Blocks: 802398
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.