Last Comment Bug 890597 - Ping Diagnostic Tool
: Ping Diagnostic Tool
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: 21 Branch
: x86_64 Linux
: -- enhancement (vote)
: mozilla25
Assigned To: Robert Bindar
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-06 02:16 PDT by Robert Bindar
Modified: 2013-07-31 14:07 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testing behavior for a new openned connection (4.73 KB, patch)
2013-07-06 02:52 PDT, Robert Bindar
no flags Details | Diff | Splinter Review
Ping diagnostic tool v1 (10.17 KB, patch)
2013-07-12 02:55 PDT, Robert Bindar
valentin.gosu: feedback+
Details | Diff | Splinter Review
ping diagnostic tool (11.29 KB, patch)
2013-07-23 09:59 PDT, Robert Bindar
no flags Details | Diff | Splinter Review
ping diagnostic tool (11.58 KB, patch)
2013-07-25 12:48 PDT, Robert Bindar
valentin.gosu: review+
Details | Diff | Splinter Review
ping diagnostic tool (11.59 KB, patch)
2013-07-28 07:36 PDT, Robert Bindar
no flags Details | Diff | Splinter Review
ping diagnostic tool (11.59 KB, patch)
2013-07-30 00:12 PDT, Robert Bindar
no flags Details | Diff | Splinter Review
ping_diagnostic_tool (11.54 KB, patch)
2013-07-31 01:11 PDT, Robert Bindar
no flags Details | Diff | Splinter Review

Description Robert Bindar 2013-07-06 02:16:04 PDT
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130620122336
Comment 1 Robert Bindar 2013-07-06 02:52:52 PDT
Created attachment 771758 [details] [diff] [review]
testing behavior for a new openned connection

I need some feedback on this start patch. I want to know if it's ok how I've approached this.
I also need to know if the timeouts about we talked on irc reduces to closing the
socket and the stream when the time for the connection to reach is up.
Comment 2 Patrick McManus [:mcmanus] 2013-07-06 06:15:20 PDT
(In reply to Robert Bindar from comment #1)
> Created attachment 771758 [details] [diff] [review]
> testing behavior for a new openned connection
> 
> I need some feedback on this start patch. I want to know if it's ok how I've
> approached this.

sure.. I wouldn't let arbitrary protocol strings through.. e.g. udp doesn't make any sense because it doesn't generate CONNECTED events.. and starttls is going to require an application layer protocol to indicate when starttls has taken place. so I'd limit it to ssl and tcp

> I also need to know if the timeouts about we talked on irc reduces to
> closing the
> socket and the stream when the time for the connection to reach is up.

yep.. close them with NS_ERROR_ABORT and then free the objects
Comment 3 Robert Bindar 2013-07-06 06:27:42 PDT
Thanks a lot, this was very helpful.
Comment 4 Robert Bindar 2013-07-12 02:55:40 PDT
Created attachment 774548 [details] [diff] [review]
Ping diagnostic tool v1

I'm currently waiting for an answer from bsmedberg about mapping the error codes to corresponding strings.
I need to know if it looks ok by now and what I should improve before asking for a review.
Comment 5 Valentin Gosu [:valentin] 2013-07-15 02:10:18 PDT
Comment on attachment 774548 [details] [diff] [review]
Ping diagnostic tool v1

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

::: netwerk/base/public/nsIDashboard.idl
@@ +39,5 @@
> +     *      ex: nullptr for TCP, "starttls", "ssl", "udp".
> +     * aHost: the host's name
> +     * aPort: the port which the connection will open on
> +     * aTimeout: the timespan before the connection will be timed out */
> +     void requestNewConnectionStatus(in ACString aHost, in unsigned long aPort,

Maybe requestConnection would be better?

::: netwerk/base/src/Dashboard.cpp
@@ +562,5 @@
> +                             uint64_t aProgress, uint64_t aProgressMax)
> +{
> +    printf("aStatus: %d\n", NS_ERROR_GET_CODE(aStatus));
> +    printf("aProgress: %ld\n", aProgress);
> +    printf("aProgressMax: %ld\n", aProgressMax);

We shouldn't leave printf's in.

@@ +563,5 @@
> +{
> +    printf("aStatus: %d\n", NS_ERROR_GET_CODE(aStatus));
> +    printf("aProgress: %ld\n", aProgress);
> +    printf("aProgressMax: %ld\n", aProgressMax);
> +    if (aStatus == NS_NET_STATUS_CONNECTED_TO) {

I'd say we want other events other than CONNECTED_TO. Hopefully we can do nsresult -> nsString without much difficulty.

@@ +576,5 @@
> +
> +NS_IMETHODIMP
> +Dashboard::Notify(nsITimer *timer)
> +{
> +    printf("FIRE: the timer was triggered\n");

Remove the printf.
Comment 6 Robert Bindar 2013-07-23 09:59:17 PDT
Created attachment 779835 [details] [diff] [review]
ping diagnostic tool
Comment 7 Valentin Gosu [:valentin] 2013-07-25 09:45:22 PDT
Comment on attachment 779835 [details] [diff] [review]
ping diagnostic tool

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

Getting closer now. I would also have a smaller status message for the patch. The bug number and title will do.

::: netwerk/base/src/Dashboard.cpp
@@ +596,5 @@
> +
> +const char *
> +Dashboard::GetErrorString(nsresult rv)
> +{
> +    ErrorEntry errors[] = {

I would make errors a static variable in Dashboard.cpp
I don't know if allocating it here is very optimal.

::: netwerk/base/src/Dashboard.h
@@ +52,5 @@
>      nsresult GetSockets();
>      nsresult GetHttpConnections();
>      nsresult GetWebSocketConnections();
>      nsresult GetDNSCacheEntries();
> +    nsresult GetConnectionStatus(nsString aStatus);

Maybe we in the future we could pass more info than a string.
Make a structure and pass that to the method.

@@ +141,5 @@
>      struct HttpData mHttp;
>      struct WebSocketData mWs;
>      struct DnsData mDns;
> +    struct ConnectionData mConn;
> +    ErrorEntry mSocketTransportStatuses[7] = {

Also move this to Dashboard.cpp - we only use it in that one function.
I also don't think ErrorEntry is needed here.
Comment 8 Robert Bindar 2013-07-25 12:48:49 PDT
Created attachment 781166 [details] [diff] [review]
ping diagnostic tool
Comment 9 Valentin Gosu [:valentin] 2013-07-28 06:14:22 PDT
Comment on attachment 781166 [details] [diff] [review]
ping diagnostic tool

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

r+ with a small change.

::: netwerk/base/src/Dashboard.h
@@ +120,5 @@
>      };
>  
> +    struct ConnectionData
> +    {
> +        ConnStatus status;

You don't need this. Just use a local ConnStatus variable.
Comment 10 Robert Bindar 2013-07-28 07:36:36 PDT
Created attachment 782260 [details] [diff] [review]
ping diagnostic tool

r=valentin.gosu
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-29 09:16:04 PDT
Needs a rebased patch.
Comment 12 Robert Bindar 2013-07-30 00:12:08 PDT
Created attachment 782984 [details] [diff] [review]
ping diagnostic tool

rebasing done
r=valentin.gosu
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-07-30 07:10:17 PDT
https://hg.mozilla.org/integration/fx-team/rev/e9530cf2baed
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-07-30 07:56:23 PDT
Backed out for Windows Werror bustage.
https://hg.mozilla.org/integration/fx-team/rev/051d88f15bc7

https://tbpl.mozilla.org/php/getParsedLog.php?id=25911194&tree=Fx-Team
Comment 15 Valentin Gosu [:valentin] 2013-07-30 08:01:25 PDT
Comment on attachment 782984 [details] [diff] [review]
ping diagnostic tool

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

::: netwerk/base/src/Dashboard.cpp
@@ +621,5 @@
> +    const char *error;
> +} ErrorEntry;
> +
> +ErrorEntry errors[] = {
> +    #define ERROR(key, val) {key, #key}

Place a #undef ERROR before defining this macro.
You may use this for defining both errors and socketTransportStatuses.
Do a try-server run before requesting checkin-needed.
Comment 16 Robert Bindar 2013-07-31 01:11:17 PDT
Created attachment 783636 [details] [diff] [review]
ping_diagnostic_tool

r=valentin.gosu
https://tbpl.mozilla.org/?tree=Try&rev=61812e74a7fc
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-07-31 06:18:00 PDT
https://hg.mozilla.org/integration/fx-team/rev/a3d8476211b3
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-07-31 13:38:21 PDT
https://hg.mozilla.org/mozilla-central/rev/a3d8476211b3

Note You need to log in before you can comment on or make changes to this bug.