783205 - about:net-internals Networking dashboard

Status: RESOLVED FIXED

(Core :: Networking, defect)

Comment 1

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

Attachment: Dashboard v0.1

This patch was created by merging Jiten Thakkar's repository [1] with the latest mozilla-inbound, and by fixing any errors.
It may be eventually broken down into little pieces for an easier review.
Feel free to provide some feedback.

[1] https://bitbucket.org/Jiten/mozilla-central

Comment 2

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

Attachment: Dashboard v0.2

Patch now contains new files.

Comment 3

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

Attachment: Dashboard v0.3

Modified types to stdint following the landing of Bug 579517.
Requesting review/feedback for the mega-patch

Comment 4

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

Attachment: Dashboard v0.4

Fixed 2 warnings in the code.
While this patch gets reviewed, I'll start working on an event system for the dashboard.

Comment 5

[Patrick McManus [:mcmanus]]

https://wiki.mozilla.org/Necko/Dashboard

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 655389 [details] [diff] [review]
Dashboard v0.4

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

Hey Valentin - I'll probably use the word "you" in the review just out of force of habit.. I know this is inherited code :) I'm sorry I'm a day later here than I said I would be.

I'm glad to see this submitted - we're moving towards something good!

you need to run the type conversion script ehsan posted on dev-platfrom on this patch.. we don't use PR*int* anymore.

tl;dr - I think we want to split this into 3 patches the same way the wiki is organized.. {status, diagnostic, tracing}... the status stuff seems furthest along and can work in an efficient pull mode and this patch is weighed down with some of the tracing bits which clearly need more thought. hopefully we can do with 1 service (the necko dashboard service) instead of the several introduced here.

I did read some sections in fine detail and left some comments, but there is enough big picture work here that it would certainly all need a full fine read when resubmitted.

::: netwerk/base/src/Makefile.in
@@ +15,5 @@
>  LIBRARY_NAME	= neckobase_s
>  LIBXUL_LIBRARY  = 1
>  
> +EXPORTS = nsURLHelper.h \
> +        nsSocketTransportService2.h \

spacing is screwed up

nsURLHelper.h has been added to the export list twice.

generally adding export is a bad sign (but not impossible that it needs to be done). I think if the various services are consolidated we can get rid of this and maybe extending existing IDLs (nsISocketTransportService) as necessary.

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +357,5 @@
>  
>      // only send this notification if we have indeed read some data.
>      // see bug 196827 for an example of why this is important.
>      if (n > 0)
> +    {

if (n > 0) {

@@ +358,5 @@
>      // only send this notification if we have indeed read some data.
>      // see bug 196827 for an example of why this is important.
>      if (n > 0)
> +    {
> +        mTransport->SendStatus(nsISocketTransport::STATUS_RECEIVING_FROM);

don't change this from NS_NET_STATUS_RECEIVING_FROM - honza likes them that way :)

@@ +361,5 @@
> +    {
> +        mTransport->SendStatus(nsISocketTransport::STATUS_RECEIVING_FROM);
> +        //For 'Networking Dashboard'
> +        nsRefPtr<nsIRunnable> event = new SocketLogRunnable(n, false);
> +        NS_DispatchToMainThread(event);

so every time data is recvd this code creates a structure and posts it to the main thread so that it can be added to a counter on the main thread. That's way too expensive.

I would suggest keeping the counter local to the socket and when the dashboard actually wants the count to have the service iterate all the sockets and package up 1 report that gets sent over to the log service.. More along the lines of nsSocketTransportService::getActiveConnections().. it can probably all be done on the socket thread.

@@ +585,5 @@
>  
>      // only send this notification if we have indeed written some data.
>      // see bug 196827 for an example of why this is important.
>      if (n > 0)
> +    {

brace again

@@ +586,5 @@
>      // only send this notification if we have indeed written some data.
>      // see bug 196827 for an example of why this is important.
>      if (n > 0)
> +    {
> +        mTransport->SendStatus(nsISocketTransport::STATUS_SENDING_TO);

constant change again

@@ +589,5 @@
> +    {
> +        mTransport->SendStatus(nsISocketTransport::STATUS_SENDING_TO);
> +        //For 'Networking Dashboard'
> +        nsRefPtr<nsIRunnable> event = new SocketLogRunnable(n, true);
> +        NS_DispatchToMainThread(event);

same comment as above.. we can't be posting runnable events for every data segment sent or received

::: netwerk/build/nsNetCID.h
@@ +608,5 @@
>      0x4a09,                                          \
>      {0x96, 0x1f, 0x65, 0x53, 0xcd, 0x60, 0xb1, 0xa2} \
>  }
>  
> +#define NS_HTTPCONNECTIONLOG_CLASSNAME \

rather than having connection log services for every protocol (websocket, http, etc..) let's just have 1 and call it the dashboard service.

@@ +757,5 @@
>    {0x82, 0xbb, 0x08, 0x5c, 0xce, 0x06, 0xc0, 0xbb}   \
>  }
>  
> +#define NS_WEBSOCKETCONNECTIONLOG_CLASSNAME \
> +    "nsWebSocketConnectionLog"

per above

@@ +834,5 @@
>      {0x85, 0x94, 0x5c, 0x4f, 0xf3, 0x00, 0x88, 0x8e} \
>  }
>  
> +#define NS_DNSCACHEENTRIES_CLASSNAME \
> +    "nsIDNSCacheEntries"

I think we can just make a change to nsIDNSService here - no need for a new idl

@@ +915,5 @@
>      NS_NETWORK_SOCKET_CONTRACTID_PREFIX "starttls"
>  
> +    
> +#define NS_SOCKETLOG_CLASSNAME \
> +    "nsSocketLog"

as per above (1 dashboard service)

::: netwerk/dns/nsDNSCacheEntries.cpp
@@ +6,5 @@
> +#include "nsDNSCacheEntries.h"
> +
> +NS_IMPL_ISUPPORTS1(nsDNSCacheEntries, nsIDNSCacheEntries)
> +
> +#define CreateArrayObject(object)           \

Its pretty important to keep the jsval/json junk out of most necko code. It should all be held within the dashboard module.. the cache entry interface should return an array of structs or strings.. and the json bits can be done in the dashboard code.. I guess this file would actually be rolled into the unified dashboard code.

::: netwerk/dns/nsDNSService2.cpp
@@ +810,5 @@
>  
>      return af;
>  }
> +
> +nsresult

s/nsresult/NSIMETHOD

::: netwerk/dns/nsDNSService2.h
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef nsDNSService2_h__

the missing ifndef/define is a good catch.. can you file this as a separate bug and mark it as blocking the dashboard? We can get that merged quickly. Its in general good to pull out fixes of old bugs from your feature as you do the feature work.. one reason is that the feature is always somewhat risky and at risk of backout but we don't want to take the easy fixes out with it too if that happens.

@@ +17,5 @@
>  #include "mozilla/Attributes.h"
>  
>  class nsDNSService MOZ_FINAL : public nsPIDNSService
>                               , public nsIObserver
> +                             , public nsISupports

why this change? (nsIObserver already inherits from nsISupports)

::: netwerk/dns/nsHostResolver.cpp
@@ +37,5 @@
>  #include "mozilla/FunctionTimer.h"
>  #include "mozilla/TimeStamp.h"
>  #include "mozilla/Telemetry.h"
>  
> +#include "jsapi.h"

again - get the json code into its own well defined file

@@ +1052,5 @@
> +{
> +  nsHostDBEnt* ent = static_cast<nsHostDBEnt *> (entry);
> +  DNSCacheEntries* args = static_cast<DNSCacheEntries *> (arg);
> +  nsHostRecord* rec = ent->rec;
> +  if(rec->addr_info)     //Without addr_info, there is no meaning of adding this entry

if (rec->addr_info) { // comment
(space and brace)

I'm not going to read this function too carefully because you're going to change the API to one that is not json oriented anyhow

@@ +1071,5 @@
> +          MutexAutoLock lock(rec->addr_info_lock);
> +          if(rec->addr_info)
> +              hostname = PR_GetCanonNameFromAddrInfo(rec->addr_info);
> +      }
> +    }else 

line ends in whitespace.. I personally don't care much about that stuff but it drives some people nuts. There is a bunch of it in this patch

google for jst-review.pl, its a lint-like script you can run on the patches that finds all that nitty stuff.

@@ +1084,5 @@
> +      family = (char *) "ipv6";
> +    else
> +      family = (char *) "ipv4";
> +    
> +    if ( PR_GetHostByName(hostname, buf, sizeof(buf), &he) == PR_FAILURE) {

why on earth is this code calling gethostbyname()? That doesn't make any sense to me in a data collection routine - plus it will block the thread!

The address should be available in he->addr and can be made into a string with PR_NetAddrToString()

@@ +1085,5 @@
> +    else
> +      family = (char *) "ipv4";
> +    
> +    if ( PR_GetHostByName(hostname, buf, sizeof(buf), &he) == PR_FAILURE) {
> +        LOG(("PR_GetIPNodeByName failed."));

PR_Get* LOG and action don't match

@@ +1111,5 @@
> +          hostAddr.AppendElement(buf);
> +     }
> +
> +    PRUint32 now =  NowInMinutes();
> +    expiration = (rec->expiration - now) * 60;

this can be negative if it has already expired.. and its being assigned to an unsigned

::: netwerk/dns/nsHostResolver.h
@@ +273,5 @@
>      bool          mShutdown;
>      PRIntervalTime mLongIdleTimeout;
>      PRIntervalTime mShortIdleTimeout;
> +public:
> +    void getDNSCacheEntries(void *);

dont deal in json at this level and use stronger types for the prototypes

::: netwerk/dns/nsIDNSCacheEntries.idl
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"

I presume this file can be rolled into the single dashboard service?

::: netwerk/dns/nsIDNSService.idl
@@ +74,5 @@
>      nsIDNSRecord resolve(in AUTF8String   aHostName,
>                           in unsigned long aFlags);
>  
>      /**
> +    * Returns cache entries in DNS cache.

i think this is ok to have on dnsservice.idl, but it should not return jsobjects and not take void pointers

::: netwerk/protocol/http/Makefile.in
@@ +6,5 @@
>  DEPTH     = @DEPTH@
>  topsrcdir = @top_srcdir@
>  srcdir    = @srcdir@
>  VPATH     = @srcdir@
> +FAIL_ON_WARNINGS = 1

this is an accident from another patch, right?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +863,5 @@
>      }
>  
> +    //To add the header to 'Networking Dashboard' log
> +    nsCOMPtr<nsIRunnable> event = new HttpHeaderLogRunnable(mRequestHead, mConnectionInfo->Host());
> +    NS_DispatchToMainThread(event);

if we just need to catch new requests being made then http-on-modify-request observers are probably the right way to do it.. Assuming this is for the event tracing portion of the project (as opposed to status info and diagnostics) I would suggest breaking all that stuff out of this patch in favor of landing the basics first.

::: netwerk/protocol/http/nsHttpConnectionLog.cpp
@@ +7,5 @@
> +#include "nsHttp.h"
> +#include "nsHttpConnectionLog.h"
> +#include "nsHttpHandler.h"
> +
> +NS_IMPL_ISUPPORTS1(nsHttpConnectionLog, nsIHttpConnectionLog)

think about threading

@@ +40,5 @@
> +                                          ,mStartLoggingHeaders(false)
> +{LOG(("Creating nsHttpConnectionLog @%x",this));}
> +
> +nsHttpConnectionLog::~nsHttpConnectionLog()
> +{LOG(("Destroying nsHttpConnectionLog @%x",this));}

spacing, and %p

::: netwerk/protocol/http/nsHttpConnectionLog.h
@@ +13,5 @@
> +#include "nsCOMPtr.h"
> +#include "nsHttpResponseHead.h"
> +#include "nsHttpRequestHead.h"
> +#include "nsIHttpHeaderVisitor.h"
> +

namespace

@@ +35,5 @@
> +    JSContext* cx;
> +    PRUint32 counter;
> +  };
> +  
> +  struct HeaderLog {

if it is anything other than members, I prefer to call them classes (this one has an operator overload).

@@ +46,5 @@
> +       return mHost.Equals(a.mHost);
> +    }
> +  };
> +  
> +  bool addHeader(nsCAutoString, nsCString, bool);

autostring is not an appropriate prototype

@@ +47,5 @@
> +    }
> +  };
> +  
> +  bool addHeader(nsCAutoString, nsCString, bool);
> +//   void getAllResponseHeaders(nsString& aResponseHeaders);

remove dead code completely

@@ +68,5 @@
> +
> +class HttpHeaderLogRunnable : public nsRunnable
> +{
> +public:
> +  HttpHeaderLogRunnable(nsHttpRequestHead& aHeader, const char * aHost):mSent(true)

as noted elsewhere hopefully we can get rid of the runnables

@@ +72,5 @@
> +  HttpHeaderLogRunnable(nsHttpRequestHead& aHeader, const char * aHost):mSent(true)
> +  {
> +//     Log(("Created event HttpHeaderLogRunnable %s",this));
> +     /**
> +      *TODO : Remove/Hide sensitive data like coockies.

I personally don't think that needs to be done.. but either do it or remove the comment, don't just leave a todo

@@ +93,5 @@
> +    nsresult rv;
> +    nsCOMPtr<nsHttpConnectionLog> HttpConnectionLogService =
> +                                  do_GetService("@mozilla.org/network/http-connectionlog;1", &rv);
> +    if(NS_FAILED(rv)){}
> +//       LOG(("Failed to initiate http-connectiolog service."));

you have a if(failed) {} //comment else {} ..

replace with something saner

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +18,5 @@
>  
>  #include "nsISSLSocketControl.h"
>  #include "prnetdb.h"
>  #include "mozilla/Telemetry.h"
> +#include "jsapi.h"

not in this file

@@ +3184,5 @@
> +}
> +
> +bool
> +nsHttpConnectionMgr::getConnectionData(void* aArg)
> +{

I think getConnectionData() is the right way to go.. but again - this is the wrong place to be making the json and use stronger types in the prototype

also assert that this is called on the network thread

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +608,5 @@
> +
> +    static PLDHashOperator ReadConnectionEntry(const nsACString &key,
> +                                             nsAutoPtr<nsConnectionEntry> &ent,
> +                                             void *aArg);
> +

indentation

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1181,5 @@
>  #endif
>          // Save http version, mResponseHead isn't available anymore after
>          // TakeResponseHead() is called
>          mHttpVersion = mResponseHead->Version();
> +        //To add the header to 'Networking Dashboard' log

spacing on the comment "// To"

and more importantly the same comment as from httpheaderlogrunnable in nshttpchannel: we probably don't need this at all and let's break out the tracing stuff from the earlier parts of the patch and just focus on status and diagnostics

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +20,5 @@
>  #include "nsITransport.h"
>  #include "nsIEventTarget.h"
>  #include "TimingStruct.h"
>  
> +#include "nsHttpConnectionLog.h"

this isn't needed in the h file for any reason.

::: netwerk/protocol/http/nsIHttpConnectionLog.idl
@@ +6,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(E1F09EC0-6F49-11E1-94D7-2F554824019B)]
> +interface nsIHttpConnectionLog : nsISupports

hopefully this is just 1 method in a dashboard service

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +942,5 @@
>    mCurrentOutSent(0),
>    mCompressor(nullptr),
>    mDynamicOutputSize(0),
> +  mDynamicOutput(nullptr),
> +  mConnectionLogService(0)

s/0/nullptr

@@ +1008,5 @@
>      nsILoadGroup *forgettableGroup;
>      mLoadGroup.forget(&forgettableGroup);
>      NS_ProxyRelease(mainThread, forgettableGroup, false);
>    }
> +  mConnectionLogService = 0;

s/0/nullptr

@@ +1316,5 @@
>  
>          NS_DispatchToMainThread(new CallOnMessageAvailable(this, utf8Data, -1));
> +        nsresult rv;
> +        if (!mConnectionLogService) {
> +          mConnectionLogService = do_GetService("@mozilla.org/network/websocket-connectionlog;1",&rv);

again, I hope we can consolidate to just a dashboard service

and make sure the threading is ok.. there are service->new* calls here from both the main and socket threads.. is everything locked appropriately in the service to handle that? It doesn't look like it to me.

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +239,5 @@
>    uint32_t                        mHdrOutToSend;
>    uint8_t                        *mHdrOut;
>    uint8_t                         mOutHeader[kCopyBreak + 16];
>    nsWSCompression                *mCompressor;
> +

needless change

::: netwerk/protocol/websocket/nsIWebSocketConnectionLog.idl
@@ +6,5 @@
> + 
> +#include "nsISupports.idl"
> + 
> +[scriptable, uuid(55eabfc7-aae8-419a-a7de-49f9dac7aec9)]
> +interface nsIWebSocketConnectionLog: nsISupports

hopefully this is just 1 method in a dashboard service

::: netwerk/protocol/websocket/nsWebSocketConnectionLog.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// #include "WebSocketLog.h"

remove dead code

@@ +24,5 @@
> +
> +namespace mozilla{
> +namespace net{
> +
> +NS_IMPL_ISUPPORTS1(nsWebSocketConnectionLog, nsIWebSocketConnectionLog)

I think this needs to be THREADSAFE because you use it on 2 threads, and then make sure the datastructures are locked or a post mechanism of some kind is used.

@@ +28,5 @@
> +NS_IMPL_ISUPPORTS1(nsWebSocketConnectionLog, nsIWebSocketConnectionLog)
> +
> +nsWebSocketConnectionLog::nsWebSocketConnectionLog():mStartLogging(false)
> +{
> +//   LOG(("Creating nsWebSocketConnectionLog @%x",this));

the log is good - don't comment it out. use %p for pointers, not %x

@@ +33,5 @@
> +}
> +
> +nsWebSocketConnectionLog::~nsWebSocketConnectionLog()
> +{
> +//   LOG(("Destroying nsWebSocketConnectionLog @%x",this));

the log is good - don't comment it out. use %p for pointers, not %x

@@ +55,5 @@
> +nsWebSocketConnectionLog::addHost(nsCString &aHost, bool aEncrypted)
> +{
> +  if(mStartLogging)
> +  {
> +    LogData data = {aHost, 0, 0, 0, 0, aEncrypted};

probably best to have a helper function for "gimme the index of this host".. it is repeated many times

@@ +68,5 @@
> +
> +bool
> +nsWebSocketConnectionLog::removeHost(nsCString &aHost)
> +{
> +  if(mStartLogging)

spacing

@@ +72,5 @@
> +  if(mStartLogging)
> +  {
> +      const LogData& temp = {aHost, 0, 0, 0, 0, 0};
> +      PRUint32 index = mData.IndexOf(temp);
> +      if(index==mData.NoIndex)

spacing

@@ +83,5 @@
> +
> +bool
> +nsWebSocketConnectionLog::newMsgSent(nsCString &aHost, PRUint32 aLength)
> +{
> +  if(mStartLogging)

spacing

@@ +86,5 @@
> +{
> +  if(mStartLogging)
> +  {
> +    const LogData& temp = {aHost, 0, 0, 0, 0, 0};
> +    PRUint32 index = mData.IndexOf(temp);

spacing

@@ +99,5 @@
> +
> +bool
> +nsWebSocketConnectionLog::newMsgReceived(nsCString &aHost, PRUint32 aLength)
> +{
> +  if(mStartLogging)

spacing

@@ +103,5 @@
> +  if(mStartLogging)
> +  {
> +    const LogData& temp = {aHost, 0, 0, 0, 0, 0};
> +    PRUint32 index = mData.IndexOf(temp);
> +    if(index==mData.NoIndex)

spacing

@@ +106,5 @@
> +    PRUint32 index = mData.IndexOf(temp);
> +    if(index==mData.NoIndex)
> +      return false;
> +    mData[index].mMsgReceived++;
> +    mData[index].mSizeReceived+=aLength;

spacing

@@ +126,5 @@
> +  CreateArrayObject(mSizeSentObj);
> +  CreateArrayObject(mSizeReceivedObj);
> +  CreateArrayObject(mEncryptedObj);
> +
> +  for(PRUint32 i=0; i<mData.Length(); i++)

trying to normalize on: 
for (uint32 i = 0; i < mData.Length(); ++i)

I probably didn't comment on it everywhere, but you should search the patch for it

@@ +136,5 @@
> +    SetElement(mSizeSentObj, INT_TO_JSVAL, mData[i].mSizeSent);
> +    SetElement(mSizeReceivedObj, INT_TO_JSVAL, mData[i].mSizeReceived);
> +    SetElement(mEncryptedObj, BOOLEAN_TO_JSVAL, mData[i].mEncrypted);
> +  }
> +  JSObject* finalObject = JS_NewObject(aCx, NULL,NULL,NULL);

spacing and nullptr not NULL

::: netwerk/protocol/websocket/nsWebSocketConnectionLog.h
@@ +15,5 @@
> +
> +namespace mozilla{
> +namespace net{
> +
> +class nsWebSocketConnectionLog: public nsIWebSocketConnectionLog

just FOO not nsFOO if you're in the mozilla::net namespace (which all new code should be)

@@ +34,5 @@
> +private:
> +
> +  struct LogData
> +  {
> +    nsCString mHost;

interesting that this is indexed by host.. don't we want to track it per websocket connection? (which might be different applications)

::: netwerk/socket/Makefile.in
@@ +35,4 @@
>    $(NULL)
>  
> + EXPORTS = \
> +  nsSocketLog.h \

generally adding export is a bad sign (but not impossible that it needs to be done). I think if the various services are consolidated we can get rid of this.

::: netwerk/socket/nsISocketLog.idl
@@ +6,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(4f2665fe-c9d3-4a3b-8108-4eeb2d9280a9)]
> +interface nsISocketLog : nsISupports

hopefully this is just 1 method in a dashboard service

::: netwerk/socket/nsSocketLog.cpp
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsSocketLog.h"
> +
> +NS_IMPL_ISUPPORTS1(nsSocketLog, nsISocketLog)

again.. probly threadsafe and then consider the implications of that.

@@ +21,5 @@
> +
> +#define SetProperty(finalObject, object, property)               \
> +val = OBJECT_TO_JSVAL(object);                                   \
> +if(!JS_SetProperty(aCx, finalObject, #property, &val)){}         \
> +//   LOG(("Couldn't set property %s in socket log.",#property)); 

logging is good.. don't comment it out. (applies to whole file)

@@ +35,5 @@
> +                          ,mTcp(nullptr)
> +                          ,mTotalSent(0)
> +                          ,mTotalReceived(0)
> +{
> +//   LOGDEBUG(("Creating nsSocketLog @%x",this));

%p not %x

@@ +67,5 @@
> +      data.mActive++;
> +    else
> +      data.mIdle++;
> +    PRInt32 index = mLog.IndexOf(data);
> +    if(index==mLog.NoIndex)

spacing

@@ +117,5 @@
> +      SocketContext* idle = static_cast<SocketContext*>
> +                            (gSocketTransportService->getIdleSocketConnections());
> +      PRInt32 i = gSocketTransportService->getActiveSocketConnectionSize();
> +//       LOGDEBUG(("size of active socket connection %d ",i+1));
> +      for(PRInt32 j=0;j<i;j++)

probly uint

@@ +123,5 @@
> +          addConnection(active[j].mFD, true);
> +      }
> +      i = gSocketTransportService->getIdleSocketConnectionSize();
> +//       LOGDEBUG(("size of idle socket connection %d ",i+1));
> +      for(PRInt32 j=0;j<i;j++)

probly uint

::: netwerk/socket/nsSocketLog.h
@@ +15,5 @@
> +#include "jsapi.h"
> +#include "prnetdb.h"
> +#include "nsThreadUtils.h"
> +#include "nsServiceManagerUtils.h"
> +

should bit in mozilla::net and named SocketLog not nsSocketLog.. (and it should be merged into the 1 dashboard service)

@@ +69,5 @@
> +class SocketLogRunnable : public nsRunnable
> +{
> +public:
> +  SocketLogRunnable(PRInt32 aSize, bool aSent):mSize(aSize)
> +                                              ,mSent(aSent)

formatting of parameter list

@@ +76,5 @@
> +  NS_IMETHOD Run()
> +  {
> +    nsresult rv;
> +    nsCOMPtr<nsSocketLog> log = do_GetService("@mozilla.org/network/socketlog;1", &rv);
> +    if(NS_SUCCEEDED(rv))

typical necko idiom:

if (NS_FAILED(rv))
  return (rv)

foo;
return NS_OK

[ instead of putting the base case indented ]

Comment 7

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

First of all, thanks for the review. It was very thorough for such a short notice.

I've started working on it, but I have a few questions:
Where would be the best place for the Dashboard.idl/Dashboard.cpp ?
Would this qualify to be implemented in netwerk/protocols/about/ ?

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to Valentin Gosu from comment #7)
> First of all, thanks for the review. It was very thorough for such a short
> notice.
> 
> I've started working on it, but I have a few questions:
> Where would be the best place for the Dashboard.idl/Dashboard.cpp ?

> Would this qualify to be implemented in netwerk/protocols/about/ ?

protocols/about is the about: implementation, so not there.

I'd just put it in base/{public,src}

Comment 9

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

Attachment: Dashboard v1.0

Comment 10

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

(In reply to Patrick McManus [:mcmanus] from comment #6)
> Comment on attachment 655389 [details] [diff] [review]
> Dashboard v0.4
> 
> Review of attachment 655389 [details] [diff] [review]:

> 
> I think getConnectionData() is the right way to go.. but again - this is the
> wrong place to be making the json and use stronger types in the prototype
> 
> also assert that this is called on the network thread
> 

This method is actually not called from the network thread. Should it?
Is there a possible race condition when enumerating the connections?

Comment 11

[Patrick McManus [:mcmanus]]

(In reply to Valentin Gosu from comment #10)
> (In reply to Patrick McManus [:mcmanus] from comment #6)
> > Comment on attachment 655389 [details] [diff] [review]
> > Dashboard v0.4
> > 
> > Review of attachment 655389 [details] [diff] [review]:
> 
> > 
> > I think getConnectionData() is the right way to go.. but again - this is the
> > wrong place to be making the json and use stronger types in the prototype
> > 
> > also assert that this is called on the network thread
> > 
> 
> This method is actually not called from the network thread. Should it?
> Is there a possible race condition when enumerating the connections?

yeah it needs to be as it iterates mCT.. that hash isn't locked, its simply only accessed from the network thread. That's true of all of mConnectionMgr except for the routines that post messages back and forth. So you need to look at any routine that touches connection manager internals and audit it for this to have a shot at working.

Comment 12

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

Is making mCT a locked hash an option, or are there performance issues?
Or is there any way of invoking the function on the socket thread?

Comment 13

[Patrick McManus [:mcmanus]]

(In reply to Valentin Gosu from comment #12)
> Is making mCT a locked hash an option, or are there performance issues?

its designed to unlocked and single threaded.

it is entertaining to think about doing real necko multithreaded processing, which would probably entail the overhead of locking, but the payoff has to be more than the diagnostic page.

> Or is there any way of invoking the function on the socket thread?

PostEvent()

Comment 14

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

Attachment: Dashboard v1.1

Solved threading issue by invoking the method synchronously on the socket thread.

Comment 15

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

Attachment: Dashboard v1.2

Solved windows build errors.
https://tbpl.mozilla.org/?tree=Try&rev=dbcafb3f985a

Comment 16

[Honza Bambas (:mayhemer)]

Valentin, did you consider using an async API to get the stats?  Also, please add comments/description to your new IDL.

Comment 17

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

I think the plan is to get the patch into mozilla-central ASAP, and figure out the rest of the features for the dashboard.
I can then focus on making it async and optimizing resource access/etc.

Comment 18

[Patrick McManus [:mcmanus]]

Comment on attachment 661138 [details] [diff] [review]
Dashboard v1.2

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

we are definietly making some progress - but there are few big picture things that need to be adjusted.

The first is that this needs to be an synchrnonous API... i.e. instead of getDNSData() you need requestDNSData() and a onDNSDataAvailable() callback... you can see why most clearly in the connection manager where you've solved the js-on-main, cmgr-hashtable-only-on-socketthread problem by dispatching a synchronous event from the main thread to the socket thread.

that does 1 of 2 things (both bad):

a] blocks the main thread until the socket thread gets around to executing the event, causing main thread jank (public enemy #1).. 

or

b] runs a nested event loop on the main thread (allowing stuff to continue there) which means everything on the stack when you execute this has to be re-entrant, and main thread code is notorioulsy not re-entrant. IME there is essentially no way to audit that this is safe on the main thread.

the second major issue is that this is a mess of exports and idls and void pointers in public interfaces and we need to be more careful with the public interfaces than internal code.

I suggest a total de-comification of the dashboard. So don't implement nsIDashboard or the dashboard xpcom service. Instead, make it owned by an existing singleton like nsSocketTransportService2 and have a globally available pointer to it.. less fuss, muss, and dependencies.. you will need to export some stuff but make an effort to keep it to the minimum - the existing setup uses way to many magic casts from void to nsDashboard.h data types from pointers that were passed through public idls without any documentation.. it just isn't tennable. This is a place where xpcom gets in the way (and in general I think it is overused.. if this isn't a component we want other people implementing then it doesn't need a public interface).

I'm happy to follow up with you on either of these things.

Comment 19

[Patrick McManus [:mcmanus]]

(In reply to Patrick McManus [:mcmanus] from comment #18)
> Comment on attachment 661138 [details] [diff] [review]
> Dashboard v1.2
> 
> Review of attachment 661138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> we are definietly making some progress - but there are few big picture
> things that need to be adjusted.
> 
> The first is that this needs to be an synchrnonous API... i.e. instead of
> 

s/sync/async of course

Comment 20

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

Attachment: Dashboard v2.0

https://tbpl.mozilla.org/?tree=Try&rev=7420af271e40

Important Changes:
The dashboard now dispatches the actual work to the socket thread. Upon completion it sets the *Available flags to true, which signal the addon can retrieve the information.

I've added the nsHttpInfo.* files, in order to eliminate unnecessary exports from the /protocol/http folder

This patch works with the following calls from JS:
https://github.com/valenting/jid1-SsyJKBEwVM7h4Q-jetpack/blob/master/resources/networking/lib/main.js

Question:
Is it ok if the DNS entries are also retrieved on the Socket Thread? Or do we use a different thread for the DNS?

Comment 21

[Patrick McManus [:mcmanus]]

Comment on attachment 665571 [details] [diff] [review]
Dashboard v2.0

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

you can't, imo, have a polling API. polling is always either wasteful (too fast) or latent (too slow). It works well as a batching mechanism when a high level of events are expected, but for 1::1 request response it is generally bad idea. 

bholley was really helpful to me when I was figuring out how to do similar things for pac processing.. maybe he can help. or bz. or jst.

worst case I suppose you could do a json->string conversion in c++ and pass the string as part of the idl and then have the js convert that back to json (somehow, I don't know the details off the top of my head) but that's so ugly we should just forget I mentioned it.

Comment 22

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

Attachment: Dashboard v2.2

Working dashboard with callbacks.

Try server status:
https://tbpl.mozilla.org/?tree=Try&rev=da4a288916d2

JS calls from the add-on:
https://github.com/valenting/jid1-SsyJKBEwVM7h4Q-jetpack/blob/master/resources/networking/lib/main.js

Comment 23

[Patrick McManus [:mcmanus]]

Comment on attachment 666180 [details] [diff] [review]
Dashboard v2.2

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

valentin - this is a million times better! Very exciting. Comments below are basically about interfaces.. all the functional stuff is up to par now imo.

::: netwerk/base/public/nsIDashboard.idl
@@ +10,5 @@
> + */
> +[scriptable, function, uuid(19d7f24f-a95a-4fd9-87e2-d96e9e4b1f6d)]
> +interface DashCallback : nsISupports
> +{
> +	void submit(in jsval data);

those should be spaces if they are tabs

s/submit/onDashboardDataAvailable

@@ +21,5 @@
> +interface nsIDashboard : nsISupports
> +{
> +	/* Arrays: host, port, tcp, active, idle, socksent, sockreceived
> +	 * Values: sent, received  */
> +	void getSocketsReq(in DashCallback cb);

requestSockets

@@ +25,5 @@
> +	void getSocketsReq(in DashCallback cb);
> +
> +	/* Arrays: host, port, spdy, ssl
> +	 * Array of arrays: active, idle */
> +	void getHttpConnectionsReq(in DashCallback cb);

requestHttpConnections

@@ +28,5 @@
> +	 * Array of arrays: active, idle */
> +	void getHttpConnectionsReq(in DashCallback cb);
> +
> +	/* Arrays: hostport, encrypted, msgsent, msgreceived, sentsize, receivedsize */
> +	void getWebSocketConnectionsReq(in DashCallback cb);

requestWebsocketConnections

@@ +31,5 @@
> +	/* Arrays: hostport, encrypted, msgsent, msgreceived, sentsize, receivedsize */
> +	void getWebSocketConnectionsReq(in DashCallback cb);
> +
> +	/* Arrays: hostname, family, hostaddr, expiration */
> +	void getDNSCacheEntriesReq(in DashCallback cb);

requestDNSInfo

@@ +34,5 @@
> +	/* Arrays: hostname, family, hostaddr, expiration */
> +	void getDNSCacheEntriesReq(in DashCallback cb);
> +
> +	/* When true, the service will log websocket events */
> +	attribute boolean startLogging;

probably enableLogging

::: netwerk/base/public/nsIDashboardListener.idl
@@ +4,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[builtinclass, uuid(24fdfcbe-54cb-4997-8392-3c476126ea3b)]
> +interface nsIDashboardListener : nsISupports

probably include this interface in the nsIDashboard.idl file

Listener probably isn't the right name

@@ +8,5 @@
> +interface nsIDashboardListener : nsISupports
> +{
> +	/* Websockets : These methods are called to
> +	 * register a websocket event with the dashboard
> +	 */

need documentation on what all the parameters mean.. id is probly renamed to serial

::: netwerk/base/src/nsDashboard.h
@@ +34,5 @@
> +    void getHttp();
> +    void getDnsEntries();
> +public:
> +
> +    struct SocketInfo

its not important, but its nice if you arrange the struct to get nice packing on 64 bits

{host, sent, received, active, idle, port, tcp}

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +1007,5 @@
> +    data->AppendElement(info);
> +}
> +
> +bool
> +nsSocketTransportService::GetSocketConnections(void * dataPtr)

why is this void *?

::: netwerk/base/src/nsSocketTransportService2.h
@@ +72,5 @@
>      bool CanAttachSocket() {
>          return mActiveCount + mIdleCount < gMaxCount;
>      }
>  
> +    bool GetSocketConnections(void *);

comment that this is about dashboard

::: netwerk/dns/Makefile.in
@@ +43,5 @@
> +  nsDNSService2.h \
> +  nsHostResolver.h \
> +  $(NULL)
> +
> +

you don't need this export anymore, right?

extra whitespace

::: netwerk/dns/nsHostResolver.cpp
@@ +1044,5 @@
> +        const char * hostname;
> +        PRNetAddr addr;
> +
> +        if (rec->flags & nsHostResolver::RES_CANON_NAME) {
> +            {

extra level of braces

::: netwerk/dns/nsHostResolver.h
@@ +275,5 @@
>      bool          mShutdown;
>      PRIntervalTime mLongIdleTimeout;
>      PRIntervalTime mShortIdleTimeout;
> +public:
> +    void GetDNSCacheEntries(void *);

more void public interfaces :(

::: netwerk/dns/nsIDNSService.idl
@@ +77,5 @@
>      /**
> +    * Returns cache entries in DNS cache.
> +    *
> +    **/
> +    void getDNSCacheEntries(in voidPtr args);

this doesn't make a lot of sense as an interface.. an in void ptr as the only argument. How does the callger get the dns cache entries?

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3114,5 @@
> +    nsDashboard::HttpRetParams info;
> +    info.host = ent->mConnInfo->Host();
> +    info.port = ent->mConnInfo->Port();
> +    for (uint32_t i = 0; i < ent->mActiveConns.Length(); i++) {
> +        nsDashboard::HttpConnInfo nfo;

your more agile mind may be able to sight read the differnce between info and nfo - but have mercy on the rest of us please :)

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +220,5 @@
>  
>      
>      bool     SupportsPipelining(nsHttpConnectionInfo *);
>  
> +    bool getConnectionData(void * aArg);

this is the entry point.. can you avoid the void *? Prototypes serve as documentation

::: netwerk/protocol/http/nsHttpInfo.h
@@ +3,5 @@
> + * file, You can obtain one at http:mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsHttpInfo__
> +#define nsHttpInfo__
> +

namespace mozilla::net

name the file HttpInfo.[cpp/h]

@@ +8,5 @@
> +class nsHttpInfo
> +{
> +public:
> +	/* Calls getConnectionData method in nsHttpConnectionMgr.
> +	 * Argument is of type nsDashboard::dnsData */

so why is it actually prototyped as void *?

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1324,5 @@
>  
>          NS_DispatchToMainThread(new CallOnMessageAvailable(this, utf8Data, -1));
> +        nsresult rv;
> +        if (mConnectionLogService) {
> +          nsCString host;

autocstring

@@ +1412,5 @@
>        if (mListener) {
>          nsCString binaryData((const char *)payload, payloadLength);
>          NS_DispatchToMainThread(new CallOnMessageAvailable(this, binaryData,
>                                                             payloadLength));
> +        //To add the header to 'Networking Dashboard' log

//[SPACE]To

@@ +1413,5 @@
>          nsCString binaryData((const char *)payload, payloadLength);
>          NS_DispatchToMainThread(new CallOnMessageAvailable(this, binaryData,
>                                                             payloadLength));
> +        //To add the header to 'Networking Dashboard' log
> +	      nsresult rv;

indentation

@@ +1837,5 @@
>    }
>  
> +  nsresult rv;
> +  if (mConnectionLogService) {
> +    nsCString host;

autocstring

@@ +2632,5 @@
>    if (NS_FAILED(rv))
>      return rv;
>  
> +  if (mConnectionLogService) {
> +    nsCString host;

autocstring

@@ +2737,5 @@
>    }
>  
> +  nsresult rv;
> +  if (mConnectionLogService) {
> +    nsCString host;

autocstring

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +243,5 @@
>    uint32_t                        mDynamicOutputSize;
>    uint8_t                        *mDynamicOutput;
> +
> +  nsCOMPtr<nsIDashboardListener> mConnectionLogService;
> +  uint32_t  mUID;

maybe call it mDashboardSerial ? Its not really a uid

Comment 24

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

(In reply to Patrick McManus [:mcmanus] from comment #23)
> Comment on attachment 666180 [details] [diff] [review]
> Dashboard v2.2 
> ::: netwerk/base/public/nsIDashboardListener.idl
> @@ +4,5 @@
> > +
> > +#include "nsISupports.idl"
> > +
> > +[builtinclass, uuid(24fdfcbe-54cb-4997-8392-3c476126ea3b)]
> > +interface nsIDashboardListener : nsISupports
> 
> probably include this interface in the nsIDashboard.idl file
> 
> Listener probably isn't the right name
> 

DashboardEventNotifier.idl ?


> 
> ::: netwerk/dns/nsHostResolver.h
> @@ +275,5 @@
> >      bool          mShutdown;
> >      PRIntervalTime mLongIdleTimeout;
> >      PRIntervalTime mShortIdleTimeout;
> > +public:
> > +    void GetDNSCacheEntries(void *);
> 
> more void public interfaces :(
> 
> ::: netwerk/dns/nsIDNSService.idl
> @@ +77,5 @@
> >      /**
> > +    * Returns cache entries in DNS cache.
> > +    *
> > +    **/
> > +    void getDNSCacheEntries(in voidPtr args);
> 
> this doesn't make a lot of sense as an interface.. an in void ptr as the
> only argument. How does the callger get the dns cache entries?
> 

Because I didn't want to #include "nsDashboard.h" in the idl. That seemed messy.
And the void * was passed from method to method until nsHostResolver::GetDNSCacheEntries

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 666180 [details] [diff] [review]
Dashboard v2.2

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

::: netwerk/base/public/nsIDashboard.idl
@@ +8,5 @@
> +/* A JavaScript callback function that takes a JSON as its parameter.
> + * The returned JSON contains arrays with data
> + */
> +[scriptable, function, uuid(19d7f24f-a95a-4fd9-87e2-d96e9e4b1f6d)]
> +interface DashCallback : nsISupports

The file name and also the interface names has all to start with nsINetworkDashboard prefix.  Using just "dash" in the name implies reference to the DASH protocol and "Dashboard" it self is too general.

Comment 26

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

Attachment: Dashboard v3.0

Fixed most of the issues.
The remaining void pointers have been properly commented.
Renamed nsDashboard to Dashboard and exported the header to mozilla/net
Renamed DashCallback to NetDashboardCallback
Various other cosmetic code modifications.

Comment 27

[Patrick McManus [:mcmanus]]

Comment on attachment 668582 [details] [diff] [review]
Dashboard v3.0

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

almost. pretty much just some void * issues left. Still too many - unless you're passing into a function that genuinely takes arbitrary types they are a real red flag.

also, you'll need a super review for this because it establishes new public interfaces.

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +1040,5 @@
> +
> +    data->AppendElement(info);
> +}
> +
> +bool

return void?

::: netwerk/base/src/nsSocketTransportService2.h
@@ +75,5 @@
>  
> +    // Called by the networking dashboard
> +    // Arg type is nsTArray<Dashboard::SocketInfo> *
> +    // Fills the passed array with socket information
> +    bool GetSocketConnections(void *);

why the void *?

::: netwerk/dns/nsDNSService2.cpp
@@ +854,5 @@
>      return af;
>  }
> +
> +NS_IMETHODIMP
> +nsDNSService::GetDNSCacheEntries(void * args)

more void pointers to functions that really don't take arbitrary pointers

::: netwerk/dns/nsHostResolver.cpp
@@ +1034,5 @@
>  }
> +
> +PLDHashOperator
> +CacheEntryEnumerator(PLDHashTable *table,
> +                     PLDHashEntryHdr *entry, uint32_t number, void *arg)

now this is a function that really takes a void* :)

::: netwerk/dns/nsHostResolver.h
@@ +280,5 @@
> +    /*
> +     * Called by the networking browser via the DnsService2
> +     * Param type is nsTArray<mozilla::net::Dashboard::DNSCacheEntries> *
> +     */
> +    void GetDNSCacheEntries(void *);

the comment is nice, why the void * ?

::: netwerk/dns/nsIDNSService.idl
@@ +79,5 @@
> +    * Called by the networking dashboard
> +    * @param args
> +    *        an nsTArray<mozilla::net::Dashboard::DNSCacheEntries> *
> +    **/
> +    void getDNSCacheEntries(in voidPtr args);

more void problems. strong typing is your friend.

native idl ptrs can help

nsihttpchannelinternal.idl might provide some examples.

Comment 28

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

I thing native idl ptr is the solution for this.
However, including "Dashboard.h" in the nsIDNSService.idl causes quite a bit of issues, because nsIDNSService.h is included in Dashboard.h
Do you think it's ok if I moved all of the structures from Dashboard.h to another header, (nsDashboardTypes.h ?) and include that everywhere it's needed? It seems a lot cleaner than using forward declaration in a bunch of places.

Comment 29

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

Attachment: Dashboard v3.2

Fixed said issues with void *
Created mozilla/net/DashboardTypes.h to hold all the structures passed to various methods.

@Patrick: Please review these changes before I request a superreview

Comment 30

[Patrick McManus [:mcmanus]]

Comment on attachment 669862 [details] [diff] [review]
Dashboard v3.2

http://www.mozilla.org/hacking/reviewers.html (:biesi probly)

Comment 31

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

Attachment: Dashboard v3.3

Removed [noscript] methods from nsIDashboard.idl -> moved them to Dashboard.h

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 672083 [details] [diff] [review]
Dashboard v3.3

+++ b/netwerk/base/public/nsIDashboard.idl
+ * The async API returns JSONs, which hold arrays with the required info

you should document that for each type of data, only one request can be pending at a time. and your code should enforce that. I think right now the code would get pretty confused if you call one of the request* functions twice in a row.

+++ b/netwerk/base/src/Dashboard.cpp
+    SET_ELEMENT(mSock.sentJ, INT_TO_JSVAL, (uint32_t) mSock.totalSent, 0);

This is not good. 32-bit integers are not enough. Since this is JS, you may have to use a double here (DOUBLE_TO_JSVAL)

+    JS_RemoveObjectRoot(cx, &mSock.hostJ);
Can you use some sort of RAII class for handing the rooting? The manual add/remove object root calls seem error-prone.

Similar for Begin/EndRequest - can you use JSAutoRequest?

I'm not too happy that you use NS_ERROR_FAILURE for everything but I guess that doesn't really matter here.

Actually, come to think of it, your error handling is not correct. When your Get* methods return an error, the error will disappear and the callbacks will never be called. That doesn't seem right - shouldn't the callbacks be called, perhaps with an empty object or null pointer or something?

+        SET_ELEMENT(mWs.sizeSentJ, INT_TO_JSVAL, (uint32_t) mWs.data[i].mSizeSent, i);

this also needs to be 64-bit/double

+        mDns.serv = do_GetService("@mozilla.org/network/dns-service;1", &rv);
+        if (NS_FAILED(rv))
+            return NS_ERROR_FAILURE;

return rv;

+        LogData data = {nsCString(aHost), uid, 0, 0, 0, 0, aEncrypted};

Just give LogData a constructor, to avoid this long list of all members. (shouldn't uid be called serial?)

+++ b/netwerk/base/src/Dashboard.h
+    virtual ~Dashboard();

should be private and nonvirtual

+        JSObject * hostJ;

I think Js instead of J would be easier to read -- I read the J suffix as Java.

+++ b/netwerk/base/src/DashboardTypes.h
+    uint32_t  active;
+    uint32_t  idle;

What are those members? Please add a comment.

+#endif // nsDashboardTypes__
\ No newline at end of file

please fix


Gotta go to a meeting now... will continue when I'm back

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 672083 [details] [diff] [review]
Dashboard v3.3

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

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +1013,5 @@
>  
>      return PR_SUCCESS;
>  }
> +
> +using namespace mozilla::net;

That should be at the top of the file

@@ +1016,5 @@
> +
> +using namespace mozilla::net;
> +
> +void
> +nsSocketTransportService::AnalyzeConnection(nsTArray<SocketInfo> * data,

General note, there should not be a star in front of the * in declarations. https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Operators

@@ +1019,5 @@
> +void
> +nsSocketTransportService::AnalyzeConnection(nsTArray<SocketInfo> * data,
> +                struct SocketContext * context, bool aActive)
> +{
> +    PRNetAddr peer_addr;

Please move all these variables down to where they are first used. See also https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

@@ +1032,5 @@
> +        port = peer_addr.inet.port;
> +    else
> +        port = peer_addr.ipv6.port;
> +    port = PR_ntohs(port);
> +    nsSocketTransport * sockTrans = static_cast<nsSocketTransport *>(context->mHandler);

This cast is not safe. Not all handlers are socket transports:
http://mxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#335
http://mxr.mozilla.org/mozilla-central/source/media/mtransport/transportlayerprsock.cpp#51
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsServerSocket.cpp#138

Just add your ByteCountSent/Received methods to nsASocketHandler and avoid the cast.

@@ +1036,5 @@
> +    nsSocketTransport * sockTrans = static_cast<nsSocketTransport *>(context->mHandler);
> +    uint64_t sent = sockTrans->ByteCountSent();
> +    uint64_t received = sockTrans->ByteCountReceived();
> +    uint32_t active = aActive ? 1 : 0;
> +    SocketInfo info = { nsCString(host, 64), sent, received, active, 1 - active, port, tcp };

That nsCString(host, 64) is incorrect. You just want nsCString(host), otherwise your string will contain all the garbage after the \0.

::: netwerk/dns/nsDNSService2.cpp
@@ +856,5 @@
> +
> +NS_IMETHODIMP
> +nsDNSService::GetDNSCacheEntries(nsTArray<mozilla::net::DNSCacheEntries> * args)
> +{
> +    mResolver->GetDNSCacheEntries(args);

Probably want:
    NS_ENSURE_TRUE(mResolver, NS_ERROR_NOT_INITIALIZED);

::: netwerk/dns/nsHostResolver.cpp
@@ +1031,5 @@
>      return rv;
>  }
> +
> +PLDHashOperator
> +CacheEntryEnumerator(PLDHashTable * table, PLDHashEntryHdr * entry, 

this line shouldn't have trailing whitespace

@@ +1035,5 @@
> +CacheEntryEnumerator(PLDHashTable * table, PLDHashEntryHdr * entry, 
> +                    uint32_t number, void * arg)
> +{
> +    nsHostDBEnt * ent = static_cast<nsHostDBEnt *> (entry);
> +    nsTArray<mozilla::net::DNSCacheEntries> * args = static_cast<nsTArray<mozilla::net::DNSCacheEntries> *> (arg);

This line is way too long. 80 characters only please.

@@ +1037,5 @@
> +{
> +    nsHostDBEnt * ent = static_cast<nsHostDBEnt *> (entry);
> +    nsTArray<mozilla::net::DNSCacheEntries> * args = static_cast<nsTArray<mozilla::net::DNSCacheEntries> *> (arg);
> +    nsHostRecord * rec = ent->rec;
> +    if (rec->addr_info) { //Without addr_info, there is no meaning of adding this entry

one space after //

@@ +1046,5 @@
> +        if (rec->flags & nsHostResolver::RES_CANON_NAME) {
> +            MutexAutoLock lock(rec->addr_info_lock);
> +            if (rec->addr_info)
> +                hostname = PR_GetCanonNameFromAddrInfo(rec->addr_info);
> +        } else if (rec->host)

please add {} braces here too. Why do you prefer CANON_NAME over rec->host?

@@ +1060,5 @@
> +        if (info.expiration <= 0)
> +            return PL_DHASH_NEXT;
> +
> +        info.family = rec->af;
> +        info.hostname = nsCString(hostname);

You don't need the nsCString() here

@@ +1063,5 @@
> +        info.family = rec->af;
> +        info.hostname = nsCString(hostname);
> +
> +        {
> +            void * ptr = nullptr;

move this down to the assignment

::: netwerk/dns/nsHostResolver.h
@@ +279,5 @@
> +
> +public:
> +    /*
> +     * Called by the networking browser via the DnsService2
> +     * Param type is nsTArray<mozilla::net::Dashboard::DNSCacheEntries> *

This line is not necessary, since the type is clearly spelled out in the signature :-)

::: netwerk/dns/nsIDNSService.idl
@@ +22,4 @@
>  /**
>   * nsIDNSService
>   */
>  [scriptable, uuid(F6E05CC3-8A13-463D-877F-D59B20B59724)]

Need a new IID

@@ +86,5 @@
>      /**
> +     * The method takes a pointer to an nsTArray
> +     * and fills it with cache entry data
> +     * Called by the networking dashboard
> +     **/

should just be */

@@ +87,5 @@
> +     * The method takes a pointer to an nsTArray
> +     * and fills it with cache entry data
> +     * Called by the networking dashboard
> +     **/
> +    void getDNSCacheEntries(in EntriesArray args);

I'd mark this [noscript] for clarity - it's clearly not scriptable.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3106,5 @@
>  }
>  
> +PLDHashOperator
> +nsHttpConnectionMgr::ReadConnectionEntry(const nsACString &key,
> +                nsAutoPtr<nsConnectionEntry> &ent,

Your indentation is kind of strange BTW, here and in the other files. Usually the second line is either indented two levels of indentation (here, 8 spaces), or lined up with the parameters in the first line.

Comment 34

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

> 
> Actually, come to think of it, your error handling is not correct. When your
> Get* methods return an error, the error will disappear and the callbacks
> will never be called. That doesn't seem right - shouldn't the callbacks be
> called, perhaps with an empty object or null pointer or something?
> 

This would mean having the callback function explicitly handle errors.
If the browser is out of memory - which would be the most common error - 
would it be able to recover from it if it was reported to javascript?

> @@ +1046,5 @@
> > +        if (rec->flags & nsHostResolver::RES_CANON_NAME) {
> > +            MutexAutoLock lock(rec->addr_info_lock);
> > +            if (rec->addr_info)
> > +                hostname = PR_GetCanonNameFromAddrInfo(rec->addr_info);
> > +        } else if (rec->host)
> 
> please add {} braces here too. Why do you prefer CANON_NAME over rec->host?
> 

I don't prefer it. Is it not a correct or efficient way of getting the hostname?

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to Valentin Gosu from comment #34)
> > 
> > Actually, come to think of it, your error handling is not correct. When your
> > Get* methods return an error, the error will disappear and the callbacks
> > will never be called. That doesn't seem right - shouldn't the callbacks be
> > called, perhaps with an empty object or null pointer or something?
> > 
> 
> This would mean having the callback function explicitly handle errors.
> If the browser is out of memory - which would be the most common error - 
> would it be able to recover from it if it was reported to javascript?

Maybe you're right... I'm just really hesitant to have callback functions that are not guaranteed to be called. It's not what streamlistener is designed around, for example.

> > @@ +1046,5 @@
> > > +        if (rec->flags & nsHostResolver::RES_CANON_NAME) {
> > > +            MutexAutoLock lock(rec->addr_info_lock);
> > > +            if (rec->addr_info)
> > > +                hostname = PR_GetCanonNameFromAddrInfo(rec->addr_info);
> > > +        } else if (rec->host)
> > 
> > please add {} braces here too. Why do you prefer CANON_NAME over rec->host?
> > 
> 
> I don't prefer it. Is it not a correct or efficient way of getting the
> hostname?

The code does prefer the canon name over rec->host, which is what I meant :) It's not correct, no. It is the canonical name of the host, which is not necessarily useful, even if the flag was passed. Just use rec->host.

Comment 36

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

Attachment: Dashboard v3.4

Enforcing just one request pending.
Using JSAutoRequest & AutoObjectRooter
Constructor now private (and virtual)
Operators now follow recommended coding style (or the one in the original file)

Comment 37

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

Ping!

Comment 38

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

Ping!

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 678458 [details] [diff] [review]
Dashboard v3.4

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

sr=biesi with these changes, but please also get review from someone who knows the JS API

::: media/mtransport/nr_socket_prsock.h
@@ +74,5 @@
>    // Implement nsASocket
>    virtual void OnSocketReady(PRFileDesc *fd, int16_t outflags);
>    virtual void OnSocketDetached(PRFileDesc *fd);
>    virtual void IsLocal(bool *aIsLocal);
> +  virtual uint64_t ByteCountSent() { return 0; }

May want to implement this at some point...

::: media/mtransport/transportlayerprsock.h
@@ +81,5 @@
>          // TODO(jesup): better check? Does it matter? (likely no)
>          *aIsLocal = false;
>        }
>  
> +      virtual uint64_t ByteCountSent() { return 0; }

and this

::: netwerk/base/src/Dashboard.cpp
@@ +38,5 @@
> +Dashboard::RequestSockets(NetDashboardCallback* cb)
> +{
> +    mSock.data.Clear();
> +    mSock.thread = NS_GetCurrentThread();
> +    if (mSock.cb)

this check really needs to be at the beginning of the function, otherwise you'll mess up the data for the existing callback. same goes for the other request* functions.

@@ +155,5 @@
> +        JSString* hostString = JS_NewStringCopyZ(cx, mHttp.data[i].host.get());
> +        SET_ELEMENT(mHttp.hostJs, STRING_TO_JSVAL, hostString, i);
> +        SET_ELEMENT(mHttp.portJs, INT_TO_JSVAL, mHttp.data[i].port, i);
> +
> +        JSObject* rtt_Active = JS_NewArrayObject(cx, 0, nullptr);

you don't have to root this?

@@ +156,5 @@
> +        SET_ELEMENT(mHttp.hostJs, STRING_TO_JSVAL, hostString, i);
> +        SET_ELEMENT(mHttp.portJs, INT_TO_JSVAL, mHttp.data[i].port, i);
> +
> +        JSObject* rtt_Active = JS_NewArrayObject(cx, 0, nullptr);
> +        JSObject* timeToLive_Active = JS_NewArrayObject(cx, 0, nullptr);

and this

@@ +166,5 @@
> +        SET_PROPERTY(active, rtt_Active, rtt);
> +        SET_PROPERTY(active, timeToLive_Active, ttl);
> +        SET_ELEMENT(mHttp.activeJs, OBJECT_TO_JSVAL, active, i);
> +
> +        JSObject* rtt_Idle = JS_NewArrayObject(cx, 0, nullptr);

and this

@@ +167,5 @@
> +        SET_PROPERTY(active, timeToLive_Active, ttl);
> +        SET_ELEMENT(mHttp.activeJs, OBJECT_TO_JSVAL, active, i);
> +
> +        JSObject* rtt_Idle = JS_NewArrayObject(cx, 0, nullptr);
> +        JSObject* timeToLive_Idle = JS_NewArrayObject(cx, 0, nullptr);

and this

@@ +219,5 @@
> +        LogData data(nsCString(aHost), aSerial, aEncrypted);
> +        if (mWs.data.Contains(data))
> +            return NS_OK;
> +        if (!mWs.data.AppendElement(data))
> +            return NS_ERROR_FAILURE;

NS_ERROR_OUT_OF_MEMORY

@@ +301,5 @@
> +    CREATE_ARRAY_OBJECT(mWs.sizeSentJs, sizesent);
> +    CREATE_ARRAY_OBJECT(mWs.sizeRecvJs, sizerecv);
> +    CREATE_ARRAY_OBJECT(mWs.encryptJs, encrypt);
> +
> +    mWs.lock.Lock();

Use MutexAutoLock instead of manual Lock/Unlock calls
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/Mutex.h#130

@@ +364,5 @@
> +    jsval val;
> +    JSContext* cx = nsContentUtils::GetSafeJSContext();
> +    JSAutoRequest request(cx);
> +
> +    JSObject* finalObject = JS_NewObject(cx, nullptr, nullptr, nullptr);

Don't all your finalObjects need to be rooted?

::: netwerk/base/src/Dashboard.h
@@ +42,5 @@
> +    nsresult GetWebSocketConnections();
> +    nsresult GetDNSCacheEntries();
> +
> +private:
> +    struct socketData

struct names should start with an uppercase letter, e.g. SocketData here.

::: netwerk/base/src/nsServerSocket.h
@@ +23,5 @@
>    virtual void OnSocketReady(PRFileDesc *fd, int16_t outFlags);
>    virtual void OnSocketDetached(PRFileDesc *fd);
>    virtual void IsLocal(bool *aIsLocal);
>  
> +  virtual uint64_t ByteCountSent() { return 0; }

should eventually implement this

::: netwerk/dns/nsHostResolver.cpp
@@ +1046,5 @@
> +            hostname = rec->host;
> +        else // No need to add this entry if no host name is there
> +            return PL_DHASH_NEXT;
> +
> +        uint32_t now =  NowInMinutes();

remove one of the duplicate spaces here

::: netwerk/protocol/http/HttpInfo.cpp
@@ +9,5 @@
> +void
> +mozilla::net::HttpInfo::
> +GetHttpConnectionData(nsTArray<HttpRetParams>* args)
> +{
> +    nsHttpHandler* handler = gHttpHandler;

Why add this local variable?

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +244,5 @@
>    uint32_t                        mDynamicOutputSize;
>    uint8_t                        *mDynamicOutput;
> +
> +  nsCOMPtr<nsIDashboardEventNotifier> mConnectionLogService;
> +  uint32_t  mSerial;

remove one of the two spaces

Comment 40

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

Comment on attachment 678458 [details] [diff] [review]
Dashboard v3.4

I haven't seen any issues with the rooting so far.
r? from jorendorff on this issue

Comment 41

[Jason Orendorff [:jorendorff]]

Comment on attachment 678458 [details] [diff] [review]
Dashboard v3.4

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

::: netwerk/base/src/Dashboard.cpp
@@ +18,5 @@
> +JS::AutoObjectRooter root_##str(cx, object);        \
> +
> +#define SET_ELEMENT(object, func, param, index)     \
> +val = func(param);                                  \
> +JS_SetElement(cx, object, index, &val);             \

Check the return value.

    if (!JS_SetElement(...))
        return NS_ERROR_OUT_OF_MEMORY;

Actually though it's a little better to use JS_DefineElement here:

    if (!JS_DefineElement(cx, object, index, func(param), NULL, NULL, JSPROP_ENUMERATE))

I think JS_SetElement walks the prototype chain (for obscure reasons -- it's kind of crazy, admittedly).

@@ +22,5 @@
> +JS_SetElement(cx, object, index, &val);             \
> +
> +#define SET_PROPERTY(finalObject, object, property) \
> +val = OBJECT_TO_JSVAL(object);                      \
> +JS_SetProperty(cx, finalObject, #property, &val);   \

Same here:
- check the return value
- use JS_DefineProperty

@@ +148,5 @@
> +    CREATE_ARRAY_OBJECT(mHttp.portJs, port);
> +    CREATE_ARRAY_OBJECT(mHttp.activeJs, active);
> +    CREATE_ARRAY_OBJECT(mHttp.idleJs, idle);
> +    CREATE_ARRAY_OBJECT(mHttp.spdyJs, spdy);
> +    CREATE_ARRAY_OBJECT(mHttp.sslJs, ssl);

It would be better to make all these local variables. Then you won't have to root them. And it seems they are not used anywhere else. (?)

Generally, any pointers from a C++ object that lives on the heap to JS objects must be rooted. I don't think you want to root anything here, because I think local variables will do the trick.

Same goes for all the other structs.

@@ +155,5 @@
> +        JSString* hostString = JS_NewStringCopyZ(cx, mHttp.data[i].host.get());
> +        SET_ELEMENT(mHttp.hostJs, STRING_TO_JSVAL, hostString, i);
> +        SET_ELEMENT(mHttp.portJs, INT_TO_JSVAL, mHttp.data[i].port, i);
> +
> +        JSObject* rtt_Active = JS_NewArrayObject(cx, 0, nullptr);

The code here looks like:
>        JSObject* rtt_Active = JS_NewArrayObject(cx, 0, nullptr);
and that's fine. You don't have to do anything special to root local variables, currently.

Comment 42

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

(In reply to Jason Orendorff [:jorendorff] from comment #41)
> Comment on attachment 678458 [details] [diff] [review]
> Dashboard v3.4
> 
> Review of attachment 678458 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +148,5 @@
> > +    CREATE_ARRAY_OBJECT(mHttp.portJs, port);
> > +    CREATE_ARRAY_OBJECT(mHttp.activeJs, active);
> > +    CREATE_ARRAY_OBJECT(mHttp.idleJs, idle);
> > +    CREATE_ARRAY_OBJECT(mHttp.spdyJs, spdy);
> > +    CREATE_ARRAY_OBJECT(mHttp.sslJs, ssl);
> 
> It would be better to make all these local variables. Then you won't have to
> root them. And it seems they are not used anywhere else. (?)
> 
> Generally, any pointers from a C++ object that lives on the heap to JS
> objects must be rooted. I don't think you want to root anything here,
> because I think local variables will do the trick.
> 

I've tried this in the beginning, but it crashed big time.
I'll give it another shot, and see how it goes.

Comment 43

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

Attachment: Dashboard v3.5

@jorendorff
You were right about rooting local objects.
Previously I was using the context in a wrong way, and that's why it crashed.
Thanks

Comment 44

[Jason Orendorff [:jorendorff]]

Comment on attachment 688018 [details] [diff] [review]
Dashboard v3.5

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

Looks good, r=me.

::: netwerk/base/src/Dashboard.cpp
@@ +18,5 @@
> +
> +#define SET_ELEMENT(object, func, param, index)       \
> +if (!JS_DefineElement(cx, object, index, func(param), \
> +        nullptr, nullptr, JSPROP_ENUMERATE))          \
> +    return NS_ERROR_OUT_OF_MEMORY;                    \

One more suggestion: instead of 'func' and 'param' arguments, use a single 'val' argument.

Instead of:
    SET_ELEMENT(hostJs, STRING_TO_JSVAL, jsstring, i);
it'll say:
    SET_ELEMENT(hostJs, STRING_TO_JSVAL(jsstring), i);

I would probably put the arguments in a different order, too:
SET_ELEMENT(object, index, val) to match JS `object[index] = val` and
SET_PROPERTY(finalObject, property, object) to match `finalObject.proprety = object`.

But that's just nit-picking. This patch is ready to go with or without these changes.

Comment 45

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

I'll just have it checked in, then I'll file another bug for these last few things.

Try run is green:
https://tbpl.mozilla.org/?tree=Try&rev=2f2f6d786351

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45907821616d

Comment 47

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

Hi Ryan,
Is there any additional input needed from my part in order to land the patch?
The Talos Regression info says: a11y Row Major MozAfterPaint increase 1.8% on Linux x64 Mozilla-Inbound-Non-PGO
This patch should no impact performance in any way.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Looking at the charts, it appears to be within the noise of the test. I don't see any reason to back this out, so it'll land on m-c with the next merge from inbound.

Comment 49

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

Thanks for the info.

Comment 50

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/45907821616d