Closed Bug 1415536 Opened 7 years ago Closed 6 years ago

Extend NotifyNetworkActivity

Categories

(Core :: Networking, enhancement, P5)

57 Branch
enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: tarek, Assigned: tarek)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 1 obsolete file)

The NetworkActivityMonitor class triggers NotifyNetworkActivity events everytime something is happening on a file descriptor of a socket. that works for all socket types afaik.

This is done by wrapping send/recv/sendto/etc in nsNetMon.

This class is not used anymore, and was there for FirefoxOS

I would like to extend that class in order to have something similar to nsIHttpActivityDistributor by keeping track of the amount of bytes that were sent and received for each socket, and a way to know from which tab the action originated from.

I am not 100% clear yet on how to provide a generic context similar to httpChannels, but for the rest of the code it should be fairly straightforward. A mapping in NotifyNetworkActivity for each fd number, counting the bytes in and out, and an event triggered at a regular interval (which can be tweaked by configuration) - the implementation can be done in NetworkActivityMonitor::DataInOut

My use case is to track all network activity *per tab* in order to build something similar to the chrome task manager and provide 4 columns related to the network:" Rx/Tx/Total bytes received/Total Byte sent.
Assignee: nobody → tarek
As it happens, nsSocketTransportService2 has a list of active and idle sockets. (To be used on the socket thread only)
http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/netwerk/base/nsSocketTransportService2.h#178-186

You can probably use the socketHandler to get to the hostname (via nsSocketTransport2) originAttributes, etc. Not sure how hard it would be to map it to the tab as well.
Priority: -- → P5
Whiteboard: [necko-triaged]
Thanks, I see that array in nsSocketTransportService2 - there are however no public method to get the context from the service.

One thing I could do is change the AttachSocket api here https://searchfox.org/mozilla-central/source/netwerk/base/nsSocketTransportService2.cpp#215 so it returns the SocketContext.

but maybe a less intrusive and backward compatible way would be to introduce a new API in that service that would let me return the Socket context, given an fd if it's in the active or the idle list.

something like:

SocketContext* nsSocketTransportService::GetSocketContext(PRFileDesc *fd);


So in NetworkActivityMonitor::AttachIOLayer(PRFileDesc *fd)  I could grab the handler using that API and store a reference to it.

There's also one thing I don't know how to do yet, is find a unique identifier for the socket from PRFileDesc.
In Python we have access to the system FD fileno but I have not seen it in the C++ layer. Do you know if there's a way to access it?  The goal will be to use it as a serializable identifier when dumping the info about sockets, instead of the PRFileDesc instance.

Thanks!
Flags: needinfo?(valentin.gosu)
(In reply to Tarek Ziadé (:tarek) from comment #2)
> Thanks, I see that array in nsSocketTransportService2 - there are however no
> public method to get the context from the service.
> 
> One thing I could do is change the AttachSocket api here
> https://searchfox.org/mozilla-central/source/netwerk/base/
> nsSocketTransportService2.cpp#215 so it returns the SocketContext.
> [...]
> So in NetworkActivityMonitor::AttachIOLayer(PRFileDesc *fd)  I could grab
> the handler using that API and store a reference to it.

I'll defer to Michal for this question.

> There's also one thing I don't know how to do yet, is find a unique
> identifier for the socket from PRFileDesc.
> In Python we have access to the system FD fileno but I have not seen it in
> the C++ layer. Do you know if there's a way to access it?  The goal will be
> to use it as a serializable identifier when dumping the info about sockets,
> instead of the PRFileDesc instance.

On Linux that would be fd->secret->md.osfd
https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/nsprpub/pr/src/pthreads/ptio.c#3417
Not sure how portable it is to windows handles.
Flags: needinfo?(valentin.gosu) → needinfo?(michal.novotny)
(In reply to Tarek Ziadé (:tarek) from comment #2)
> but maybe a less intrusive and backward compatible way would be to introduce
> a new API in that service that would let me return the Socket context, given
> an fd if it's in the active or the idle list.

This IMO won't work because you would need to iterate over the lists to find the SocketContext by the fd.

> something like:
> 
> SocketContext* nsSocketTransportService::GetSocketContext(PRFileDesc *fd);
> 
> So in NetworkActivityMonitor::AttachIOLayer(PRFileDesc *fd)  I could grab
> the handler using that API and store a reference to it.

SocketContext* is a pointer to one of the lists, so it shouldn't be used outside nsSocketTransportService2. You should return nsASocketHandler* instead.
Flags: needinfo?(michal.novotny)
This is a first attempt at a patch. It misses tests in JS but I wanted to have some feedback to see if this is the right direction. 

The network activity is sent every 5 seconds as a JSON dump of all active sockets, and that can be used in JS with an observer.

On the JS side I can collect and build something like https://ziade.org/aboutEnergy.xhtml

Thanks for your feedback
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(michal.novotny)
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207014


C/C++ static analysis found 6 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/base/NetworkActivityMonitor.cpp:207
(Diff revision 1)
>      if (!obs)
>        return NS_ERROR_FAILURE;
>  
> -    obs->NotifyObservers(nullptr,
> -                         mDirection == NetworkActivityMonitor::kUpload
> -                           ? NS_NETWORK_ACTIVITY_BLIP_UPLOAD_TOPIC
> +    SocketActivity* activity;
> +    nsString* data = new nsString;
> +    data->AppendPrintf("{\"sockets\":");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

    data->AppendPrintf("{\"sockets\":");
                       ^
                       R"({"sockets":)"

::: netwerk/base/NetworkActivityMonitor.cpp:212
(Diff revision 1)
> -                           ? NS_NETWORK_ACTIVITY_BLIP_UPLOAD_TOPIC
> -                           : NS_NETWORK_ACTIVITY_BLIP_DOWNLOAD_TOPIC,
> -                         nullptr);
> +    data->AppendPrintf("{\"sockets\":");
> +
> +    for (unsigned long i = 0; i < activities.Length(); i++) {
> +        activity = activities[i];
> +        data->AppendPrintf("{");
> +        data->AppendPrintf("\"Fd\":%d,", activity->fd);

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

        data->AppendPrintf("\"Fd\":%d,", activity->fd);
                           ^
                           R"("Fd":%d,)"

::: netwerk/base/NetworkActivityMonitor.cpp:213
(Diff revision 1)
> -                           : NS_NETWORK_ACTIVITY_BLIP_DOWNLOAD_TOPIC,
> -                         nullptr);
> +
> +    for (unsigned long i = 0; i < activities.Length(); i++) {
> +        activity = activities[i];
> +        data->AppendPrintf("{");
> +        data->AppendPrintf("\"Fd\":%d,", activity->fd);
> +        data->AppendPrintf("\"Rx\":%d,", activity->Rx);

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

        data->AppendPrintf("\"Rx\":%d,", activity->Rx);
                           ^
                           R"("Rx":%d,)"

::: netwerk/base/NetworkActivityMonitor.cpp:214
(Diff revision 1)
> -                         nullptr);
> +    for (unsigned long i = 0; i < activities.Length(); i++) {
> +        activity = activities[i];
> +        data->AppendPrintf("{");
> +        data->AppendPrintf("\"Fd\":%d,", activity->fd);
> +        data->AppendPrintf("\"Rx\":%d,", activity->Rx);
> +        data->AppendPrintf("\"Tx\":%d", activity->Tx);

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

        data->AppendPrintf("\"Tx\":%d", activity->Tx);
                           ^
                           R"("Tx":%d)"

::: netwerk/base/NetworkActivityMonitor.cpp:215
(Diff revision 1)
> +        activity = activities[i];
> +        data->AppendPrintf("{");
> +        data->AppendPrintf("\"Fd\":%d,", activity->fd);
> +        data->AppendPrintf("\"Rx\":%d,", activity->Rx);
> +        data->AppendPrintf("\"Tx\":%d", activity->Tx);
> +        data->AppendPrintf("\"Host\":");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

        data->AppendPrintf("\"Host\":");
                           ^
                           R"("Host":)"

::: netwerk/base/NetworkActivityMonitor.cpp:217
(Diff revision 1)
> +        data->AppendPrintf("\"Fd\":%d,", activity->fd);
> +        data->AppendPrintf("\"Rx\":%d,", activity->Rx);
> +        data->AppendPrintf("\"Tx\":%d", activity->Tx);
> +        data->AppendPrintf("\"Host\":");
> +        data->Append(activity->host);
> +        data->AppendPrintf("\"Port\":%d", activity->port);

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

        data->AppendPrintf("\"Port\":%d", activity->port);
                           ^
                           R"("Port":%d)"
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207098


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/base/NetworkActivityMonitor.cpp:207
(Diff revision 2)
>      if (!obs)
>        return NS_ERROR_FAILURE;
>  
> -    obs->NotifyObservers(nullptr,
> -                         mDirection == NetworkActivityMonitor::kUpload
> -                           ? NS_NETWORK_ACTIVITY_BLIP_UPLOAD_TOPIC
> +    SocketActivity* activity;
> +    nsString* data = new nsString;
> +    data->AppendPrintf("{\"sockets\":");

Warning: Escaped string literal can be written as a raw string literal [clang-tidy: modernize-raw-string-literal]

    data->AppendPrintf("{\"sockets\":");
                       ^
                       R"({"sockets":)"
Blocks: 1419681
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207360

::: netwerk/base/NetworkActivityMonitor.cpp:220
(Diff revision 4)
> +        data->AppendPrintf(R"("Host":)");
> +        data->Append(activity->host);
> +        data->AppendPrintf(",");
> +        data->AppendPrintf(R"("Port":%d)", activity->port);
> +        data->AppendPrintf("}");
> +    }

Generating string JSON and parsing it in the consumer seems wasteful at best.
Take a look at netwerk/base/Dashboard.cpp
In Dashboard::GetSockets for example, it uses a webidl to create a JSON object, and it passes it directly to the JS, without having to alocate a huge string for it, and without having to parse in JS.

::: netwerk/base/NetworkActivityMonitor.cpp:346
(Diff revision 4)
> +    gInstance->mActivity.port.Put(osfd, port);
> +    char _host[64];
> +    PR_NetAddrToString(addr, _host, sizeof(_host));
> +    nsString* host = new nsString;
> +    host->AppendPrintf("%s", _host);
> +    gInstance->mActivity.host.Put(osfd, *host);

This seems like a leak to me.
Why not nsString host(_host); ... host.Put(osfd, host)
Attachment #8930612 - Flags: review?(valentin.gosu)
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207360

> Generating string JSON and parsing it in the consumer seems wasteful at best.
> Take a look at netwerk/base/Dashboard.cpp
> In Dashboard::GetSockets for example, it uses a webidl to create a JSON object, and it passes it directly to the JS, without having to alocate a huge string for it, and without having to parse in JS.

Ok sounds good - the change is almost ready. There's one thing I am not sure how to do.

In the current code, I am passing the JSON as a string with :

```
obs->NotifyObservers(nullptr, NS_NETWORK_ACTIVITY, data->get());
```

Now that I have a JS::RootedValue instead, I am not sure how to pass it
to the observer since the event data (3rd argument) is expected to 
be a string.

It looks like the dashboard is using a callback, not the observer service.
I don't know how to trigger the event with the json object.

> This seems like a leak to me.
> Why not nsString host(_host); ... host.Put(osfd, host)

I am not sure how to do this. This code:

```
char _host[64];
PR_NetAddrToString(addr, _host, sizeof(_host));
nsString host(_host);
gInstance->mActivity.host.Put(osfd, host);
``` 

fails because there's no constructor in nsString that accepts a char[]
My initial idea was that it was a waste to have two strings and
I was trying to pass a nsString directly to NetAddToString by 
pointing some internal char, but I failed.
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207426

::: netwerk/base/NetworkActivityMonitor.cpp:220
(Diff revision 4)
> +        data->AppendPrintf(R"("Host":)");
> +        data->Append(activity->host);
> +        data->AppendPrintf(",");
> +        data->AppendPrintf(R"("Port":%d)", activity->port);
> +        data->AppendPrintf("}");
> +    }

You can probably wrap it in something that extends nsISupports and pass it as the first argument to NotifyObservers.

::: netwerk/base/NetworkActivityMonitor.cpp:346
(Diff revision 4)
> +    gInstance->mActivity.port.Put(osfd, port);
> +    char _host[64];
> +    PR_NetAddrToString(addr, _host, sizeof(_host));
> +    nsString* host = new nsString;
> +    host->AppendPrintf("%s", _host);
> +    gInstance->mActivity.host.Put(osfd, *host);

Then this should work:
```
nsString host;
host.AppendPrintf("%s", _host);
gInstance->mActivity.host.Put(osfd, *host);
```
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207426

> You can probably wrap it in something that extends nsISupports and pass it as the first argument to NotifyObservers.

This is my "attempt" after a few hours trying things:


```
class NetworkJsonData final : public nsISupports
                            , public nsWrapperCache
{
public:
  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(NetworkJsonData)
  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
  NetworkJsonData(JS::RootedValue* aData) {
      mData = aData;
  };

  JS::RootedValue* mData;

private:
  virtual ~NetworkJsonData() {};
};


NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(NetworkJsonData)
NS_IMPL_CYCLE_COLLECTING_ADDREF(NetworkJsonData)
NS_IMPL_CYCLE_COLLECTING_RELEASE(NetworkJsonData)
NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(NetworkJsonData).
    NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
    NS_INTERFACE_MAP_ENTRY(nsISupports)
NS_INTERFACE_MAP_END

JSObject*
NetworkJsonData::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
{
  return nullptr;
}
```


Is this really what it takes just to wrap the JSON value in nsISupports ?
It looks very complicated, do you know if there's a simpler way to do it?

Thanks for all the feedback so far :)
(In reply to Tarek Ziadé (:tarek) from comment #15)
> Comment on attachment 8930612 [details] 
> Is this really what it takes just to wrap the JSON value in nsISupports ?
> It looks very complicated, do you know if there's a simpler way to do it?
> 
> Thanks for all the feedback so far :)

That seems about right. Not totally sure if I'm missing something. Maybe someone from the DOM team could also provide some feedback.
I suspect WrapObject should return something else than nullptr, right?
Flags: needinfo?(valentin.gosu)
Attachment #8931259 - Attachment is obsolete: true
Comment on attachment 8931259 [details]
use webidl

https://reviewboard.mozilla.org/r/202402/#review207732


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/base/NetworkActivityMonitor.cpp:171
(Diff revision 1)
> +                            , public nsWrapperCache
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(NetworkJsonData)
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: netwerk/base/NetworkActivityMonitor.cpp:172
(Diff revision 1)
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(NetworkJsonData)
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +  NetworkJsonData(JS::RootedValue* aData) {

Error: Bad implicit conversion constructor for 'networkjsondata' [clang-tidy: mozilla-implicit-constructor]

::: netwerk/base/NetworkActivityMonitor.cpp:179
(Diff revision 1)
> +  };
> +
> +  JS::RootedValue* mData;
> +
> +private:
> +  virtual ~NetworkJsonData() {};

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  virtual ~NetworkJsonData() {};
          ^
                             = default;
> I suspect WrapObject should return something else than nullptr, right?

I have no idea, I don't even know why I had to use nsWrapperCache in the first place. 
I'll ping someone from the DOM goog suggsetion thanks!
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207740


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/base/NetworkActivityMonitor.cpp:171
(Diff revision 5)
> +                            , public nsWrapperCache
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(NetworkJsonData)
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

::: netwerk/base/NetworkActivityMonitor.cpp:172
(Diff revision 5)
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(NetworkJsonData)
> +  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +  NetworkJsonData(JS::RootedValue* aData) {

Error: Bad implicit conversion constructor for 'networkjsondata' [clang-tidy: mozilla-implicit-constructor]

::: netwerk/base/NetworkActivityMonitor.cpp:179
(Diff revision 5)
> +  };
> +
> +  JS::RootedValue* mData;
> +
> +private:
> +  virtual ~NetworkJsonData() {};

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  virtual ~NetworkJsonData() {};
          ^
                             = default;
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207746

Can you write some unit tests?
I'm worried about leaks and correctness, and it would be great to have the tests check for this.
Also, have a DOM peer also review this patch. https://wiki.mozilla.org/Modules/All#Document_Object_Model

::: dom/webidl/moz.build:714
(Diff revision 6)
>      'MutationEvent.webidl',
>      'MutationObserver.webidl',
>      'NamedNodeMap.webidl',
>      'NativeOSFileInternals.webidl',
>      'NetDashboard.webidl',
> +    'NetworkActivity.webidl',

You should add this file to the patch too.

::: netwerk/base/NetworkActivityMonitor.cpp:244
(Diff revision 6)
>  
> -    obs->NotifyObservers(nullptr,
> -                         mDirection == NetworkActivityMonitor::kUpload
> -                           ? NS_NETWORK_ACTIVITY_BLIP_UPLOAD_TOPIC
> -                           : NS_NETWORK_ACTIVITY_BLIP_DOWNLOAD_TOPIC,
> -                         nullptr);
> +    AutoSafeJSContext cx;
> +    mozilla::dom::SocketsMonitor dict;
> +    dict.mSockets.Construct();
> +    Sequence<mozilla::dom::SocketMonitor> &sockets = dict.mSockets.Value();
> +    SocketActivity* activity;

Declare this pointer where it's used

::: netwerk/base/NetworkActivityMonitor.cpp:259
(Diff revision 6)
> +    }
> +    JS::RootedValue val(cx);
> +    if (!ToJSValue(cx, dict, &val))
> +        return NS_ERROR_FAILURE;
> +
> +    NetworkJsonData* data = new NetworkJsonData(&val);

Use a RefPtr<NetworkJsonData> instead of a C style pointer to prevent leak.

::: netwerk/base/NetworkActivityMonitor.cpp:265
(Diff revision 6)
> +
> +    obs->NotifyObservers(data, NS_NETWORK_ACTIVITY, nullptr);
>      return NS_OK;
>    }
>  private:
> -  NetworkActivityMonitor::Direction mDirection;
> +  nsTArray<SocketActivity*> activities;

All of the SocketActivity objects would leak, since we never delete them.
Make this an array of <SocketActivity> (not pointers) or handle it somehow.

::: netwerk/base/NetworkActivityMonitor.cpp:346
(Diff revision 6)
>  }
>  
>  nsresult
>  NetworkActivityMonitor::AttachIOLayer(PRFileDesc *fd)
>  {
> -  if (!gInstance)
> +  if (!gInstance) 

nit: trailing whitespace
Also, use {} braces
Attachment #8930612 - Flags: review?(valentin.gosu)
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207746

> You should add this file to the patch too.

oops yeah, thanks that was a miss
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review207746

Yes I was planning to do that next - wanted to make sure the patch was doing the right things
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review208158

::: commit-message-5c48b:5
(Diff revision 11)
> +Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes r?valentin r?michal r?baku
> +
> +MozReview-Commit-ID: AcHk8IqeHeM
> +***
> +use webidl

you are not using webidl :)

::: netwerk/base/NetworkActivityMonitor.h:14
(Diff revision 11)
>  
>  #include <stdint.h>
>  #include "nscore.h"
>  #include "prio.h"
>  #include "prinrval.h"
> +#include "nsDataHashtable.h"

alphbetic order in the includes.

::: netwerk/base/NetworkActivityMonitor.h:29
(Diff revision 11)
> +typedef nsDataHashtable<nsUint32HashKey, uint32_t> SocketBytes;
> +typedef nsDataHashtable<nsUint32HashKey, uint32_t> SocketPort;
> +typedef nsDataHashtable<nsUint32HashKey, nsString> SocketHost;
> +
> +
> +struct SocketActivity {

this can be moved to the cpp file.

::: netwerk/base/NetworkActivityMonitor.h:55
(Diff revision 11)
> +  NS_DECL_NSINETWORKACTIVITYDATA
> +  NetworkData(const nsString& aHost, int32_t aPort, int32_t aFd,
> +              int32_t aRx, int32_t aTx)
> +      : mHost(aHost), mPort(aPort), mFd(aFd), mRx(aRx), mTx(aTx) {}
> +private:
> +  ~NetworkData() {}

= default;

::: netwerk/base/NetworkActivityMonitor.h:76
(Diff revision 11)
>    };
>  
>    NetworkActivityMonitor();
>    ~NetworkActivityMonitor();
>  
> -  static nsresult Init(int32_t blipInterval);
> +  static nsresult Init(int32_t interval);

aInterval

::: netwerk/base/NetworkActivityMonitor.h:80
(Diff revision 11)
>  
> -  static nsresult Init(int32_t blipInterval);
> +  static nsresult Init(int32_t interval);
>    static nsresult Shutdown();
>  
>    static nsresult AttachIOLayer(PRFileDesc *fd);
> -  static nsresult DataInOut(Direction direction);
> +  static nsresult DataInOut(Direction direction, PRFileDesc *fd, uint32_t amount);

aSomething

::: netwerk/base/NetworkActivityMonitor.h:81
(Diff revision 11)
> -  static nsresult Init(int32_t blipInterval);
> +  static nsresult Init(int32_t interval);
>    static nsresult Shutdown();
>  
>    static nsresult AttachIOLayer(PRFileDesc *fd);
> -  static nsresult DataInOut(Direction direction);
> -
> +  static nsresult DataInOut(Direction direction, PRFileDesc *fd, uint32_t amount);
> +  static nsresult RegisterFd(PRFileDesc *fd, const PRNetAddr *addr);

everywhere in this file

::: netwerk/base/NetworkActivityMonitor.cpp:17
(Diff revision 11)
>  #include "nsThreadUtils.h"
>  #include "mozilla/Services.h"
>  #include "prerror.h"
> +#include <vector>
> +#include "mozilla/Logging.h"
> +#include "nsPrintfCString.h"

sort this header... somehow.

::: netwerk/base/NetworkActivityMonitor.cpp:31
(Diff revision 11)
>    PRStatus ret;
>    PRErrorCode code;
>    ret = fd->lower->methods->connect(fd->lower, addr, timeout);
>    if (ret == PR_SUCCESS || (code = PR_GetError()) == PR_WOULD_BLOCK_ERROR ||
> -      code == PR_IN_PROGRESS_ERROR)
> -    NetworkActivityMonitor::DataInOut(NetworkActivityMonitor::kUpload);
> +      code == PR_IN_PROGRESS_ERROR) {
> +      NetworkActivityMonitor::DataInOut(NetworkActivityMonitor::kUpload, fd, 0);

I leave this to necko peers.

::: netwerk/base/NetworkActivityMonitor.cpp:164
(Diff revision 11)
>  
>  
> +/* NetworkData class */
> +NS_IMPL_ISUPPORTS(NetworkData, nsINetworkActivityData);
> +
> +NS_IMETHODIMP NetworkData::GetHost(nsAString & aHost) {

NS_IMETHODIMP
NetworkData::GetFoobar(nsAString& aValue)
{
  aValue = mValue;
  return NS_OK;
}

use this style, please.

::: netwerk/base/NetworkActivityMonitor.cpp:169
(Diff revision 11)
> +NS_IMETHODIMP NetworkData::GetHost(nsAString & aHost) {
> +  aHost = mHost;
> +  return NS_OK;
> +};
> +
> +NS_IMETHODIMP NetworkData::SetHost(const nsAString & aHost) {

no setters required if you mark the attribute readonly.

::: netwerk/base/NetworkActivityMonitor.cpp:174
(Diff revision 11)
> +NS_IMETHODIMP NetworkData::SetHost(const nsAString & aHost) {
> +  mHost = aHost;
> +  return NS_OK;
> +};
> +
> +NS_IMETHODIMP NetworkData::GetPort(int32_t *aPort) {

int32_t* aPort

::: netwerk/base/NetworkActivityMonitor.cpp:220
(Diff revision 11)
> +
>  class NotifyNetworkActivity : public mozilla::Runnable {
>  public:
> -  explicit NotifyNetworkActivity(NetworkActivityMonitor::Direction aDirection)
> +  explicit NotifyNetworkActivity(NetworkActivity* netActivity)
>      : mozilla::Runnable("NotifyNetworkActivity")
> -    , mDirection(aDirection)
> +

no extra line here

::: netwerk/base/NetworkActivityMonitor.cpp:252
(Diff revision 11)
> +
>    NS_IMETHOD Run() override
>    {
>      MOZ_ASSERT(NS_IsMainThread());
> -
> +   /*
> +    if (activities.Length() == 0) {

why this comment?

::: netwerk/base/NetworkActivityMonitor.cpp:260
(Diff revision 11)
> +    */
>      nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
>      if (!obs)
>        return NS_ERROR_FAILURE;
>  
> -    obs->NotifyObservers(nullptr,
> +    nsCOMPtr<nsIMutableArray> array = do_CreateInstance(NS_ARRAY_CONTRACTID);

this could fail.
if (NS_WARN_IF(!array)) {
  return ...

::: netwerk/base/NetworkActivityMonitor.cpp:263
(Diff revision 11)
>        return NS_ERROR_FAILURE;
>  
> -    obs->NotifyObservers(nullptr,
> -                         mDirection == NetworkActivityMonitor::kUpload
> -                           ? NS_NETWORK_ACTIVITY_BLIP_UPLOAD_TOPIC
> -                           : NS_NETWORK_ACTIVITY_BLIP_DOWNLOAD_TOPIC,
> +    nsCOMPtr<nsIMutableArray> array = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +
> +    for (unsigned long i = 0; i < activities.Length(); i++) {
> +        nsCOMPtr<nsINetworkActivityData> data = new NetworkData(activities[i].host,

indentation.

::: netwerk/base/NetworkActivityMonitor.cpp:269
(Diff revision 11)
> -                         nullptr);
> +                activities[i].port,
> +                activities[i].fd,
> +                activities[i].rx,
> +                activities[i].tx);
> +
> +        array->AppendElement(data);

This can fail. In case, NS_ERROR_OUT_OF_MEMORY

::: netwerk/base/NetworkActivityMonitor.cpp:272
(Diff revision 11)
> +                activities[i].tx);
> +
> +        array->AppendElement(data);
> +    }
> +
> +    obs->NotifyObservers(array, NS_NETWORK_ACTIVITY, nullptr);

do you want to dispatch events also if the array is empty?

::: netwerk/base/NetworkActivityMonitor.cpp:276
(Diff revision 11)
> +
> +    obs->NotifyObservers(array, NS_NETWORK_ACTIVITY, nullptr);
>      return NS_OK;
>    }
>  private:
> -  NetworkActivityMonitor::Direction mDirection;
> +  nsTArray<SocketActivity> activities;

mActivities;

::: netwerk/base/NetworkActivityMonitor.cpp:300
(Diff revision 11)
>    MOZ_COUNT_DTOR(NetworkActivityMonitor);
>    gInstance = nullptr;
>  }
>  
>  nsresult
> -NetworkActivityMonitor::Init(int32_t blipInterval)
> +NetworkActivityMonitor::Init(int32_t interval)

aInterval

::: netwerk/base/NetworkActivityMonitor.cpp:329
(Diff revision 11)
>    delete gInstance;
>    return NS_OK;
>  }
>  
>  nsresult
> -NetworkActivityMonitor::Init_Internal(int32_t blipInterval)
> +NetworkActivityMonitor::Init_Internal(int32_t interval)

aInterval

::: netwerk/base/nsINetworkActivityData.idl:7
(Diff revision 11)
> + * 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"
> +
> +

add a comment.. what is this about?

::: netwerk/base/nsINetworkActivityData.idl:11
(Diff revision 11)
> +
> +
> +[scriptable, builtinclass, uuid(30d5f743-939e-46c6-808a-7ea07c77028e)]
> +interface nsINetworkActivityData : nsISupports
> +{
> +  attribute DOMString host;

mark all these attributes readonly and remove the setter methods.

::: netwerk/test/unit/test_network_activity.js:2
(Diff revision 11)
> +// test for networkactivity
> +// 

extra space.
Attachment #8930612 - Flags: review?(amarchesini) → review+
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review208158

> This can fail. In case, NS_ERROR_OUT_OF_MEMORY

for this one I can't find any example where that error is catched and dealt with. AppendElement() calls seem to be done wihtout looking for that error. SHould I just catch NS_ERROR_OUT_OF_MEMORY and return NS_ERROR_FAILURE, or just let it happen? thx!
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review209224

Sorry to keep this going, but we have test crashes, and the thread-safety concern that I hadn't noticed before.

::: netwerk/base/NetworkActivityMonitor.h:34
(Diff revision 21)
> +struct NetworkActivity {
> +    SocketPort port;
> +    SocketHost host;
> +    SocketBytes rx;
> +    SocketBytes tx;
> +};

I triggered a debug test on the try run:
https://treeherder.mozilla.org/logviewer.html#?job_id=148342639&repo=try&lineNumber=2360

It probably has a problem with this line:
aActivity->tx.Put(fd, 0); 
in the NotifyNetworkActivity constructor.

I assume the struct isn't properly initialized?
I think it should have a constructor.

Also, we have a thread-safety issue here.
Hashtables are not inherently threadsafe.
Right now, we access them both from the timer (which is initied on the main thread, so it runs on the main thread) and the socket thread.
So we either need a mutex when accessing on multiple threads, or we need to make the timer run on the socket thread. Even then, we might still need a mutex,  in case shutdown comes while we are running the callback on the socket thread.

::: netwerk/base/NetworkActivityMonitor.cpp:260
(Diff revision 21)
> +    for (unsigned long i = 0; i < mActivities.Length(); i++) {
> +      nsCOMPtr<nsINetworkActivityData> data = new NetworkData(mActivities[i].host,
> +        mActivities[i].port,
> +        mActivities[i].fd,
> +        mActivities[i].rx,
> +        mActivities[i].tx);

nit: the arguments should be aligned with the ( from new NetworkData. It's best if you bring the new to the next line.

::: netwerk/base/NetworkActivityMonitor.cpp:382
(Diff revision 21)
> -  // activity even right now will trigger notification.
> -  mLastNotificationTime[kUpload] = PR_IntervalNow() - mBlipInterval;
> -  mLastNotificationTime[kDownload] = mLastNotificationTime[kUpload];
> -
> -  return NS_OK;
> +  mInterval = aInterval;
> +  mTimer = NS_NewTimer();
> +  if (!mTimer) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  return mTimer->InitWithCallback(cb, mInterval, nsITimer::TYPE_REPEATING_SLACK);

Why don't we just make NetworkActivityMonitor implement nsITimerCallback and init the timer with `this` as the first parameter?

::: netwerk/test/unit/test_XHR_redirects.js:88
(Diff revision 21)
>    // format: redirectType, methodToSend, redirectedMethod, finalStatus
>    //   redirectType sets the URI the initial request goes to
>    //   methodToSend is the HTTP method to send
>    //   redirectedMethod is the method to use for the redirect, if any
>    //   finalStatus is 200 when the redirect takes place, redirectType otherwise
> -  
> +

No whitespace only changes please. Revert this file.
Attachment #8930612 - Flags: review?(valentin.gosu)
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review209224

No problem! I want the best patch. Thanks for all your feedback so far.

> I triggered a debug test on the try run:
> https://treeherder.mozilla.org/logviewer.html#?job_id=148342639&repo=try&lineNumber=2360
> 
> It probably has a problem with this line:
> aActivity->tx.Put(fd, 0); 
> in the NotifyNetworkActivity constructor.
> 
> I assume the struct isn't properly initialized?
> I think it should have a constructor.
> 
> Also, we have a thread-safety issue here.
> Hashtables are not inherently threadsafe.
> Right now, we access them both from the timer (which is initied on the main thread, so it runs on the main thread) and the socket thread.
> So we either need a mutex when accessing on multiple threads, or we need to make the timer run on the socket thread. Even then, we might still need a mutex,  in case shutdown comes while we are running the callback on the socket thread.

I have added a mutex, and that means adding more methods to the class to use it from the static methods. I find this singleton design a bit clunky (where there's a single global instance dealing with static calls) but I guess we can arrange the design later.

I am going to run try again this morning. Let me know how you run it btw, so I can try it the same way.

> nit: the arguments should be aligned with the ( from new NetworkData. It's best if you bring the new to the next line.

done
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review209306


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/base/NetworkActivityMonitor.cpp:437
(Diff revision 22)
>      }
> +    return gInstance->RegisterFd_Internal(osfd, NS_ConvertUTF8toUTF16(host), port);
> +}
> +
> +nsresult
> +NetworkActivityMonitor::RegisterFd_Internal(PROsfd aOsfd, nsString host, uint16_t port)

Warning: The parameter 'host' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: netwerk/base/NetworkActivityMonitor.cpp:453
(Diff revision 22)
> +  MOZ_ASSERT(OnSocketThread(), "not on socket thread");
> +  if (gInstance) {
> +    PROsfd osfd = PR_FileDesc2NativeHandle(aFd);
> +    return gInstance->DataInOut_Internal(osfd, aDirection, aAmount);
> +  }
> +  else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review209326


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/base/NetworkActivityMonitor.cpp:435
(Diff revision 23)
>      }
> +    return gInstance->RegisterFd_Internal(osfd, NS_ConvertUTF8toUTF16(host), port);
> +}
> +
> +nsresult
> +NetworkActivityMonitor::RegisterFd_Internal(PROsfd aOsfd, nsString host, uint16_t port)

Warning: The parameter 'host' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: netwerk/base/NetworkActivityMonitor.cpp:451
(Diff revision 23)
> +  MOZ_ASSERT(OnSocketThread(), "not on socket thread");
> +  if (gInstance) {
> +    PROsfd osfd = PR_FileDesc2NativeHandle(aFd);
> +    return gInstance->DataInOut_Internal(osfd, aDirection, aAmount);
> +  }
> +  else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review209338


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/base/NetworkActivityMonitor.cpp:435
(Diff revision 24)
>      }
> +    return gInstance->RegisterFd_Internal(osfd, NS_ConvertUTF8toUTF16(host), port);
> -  }
> +}
>  
> +nsresult
> +NetworkActivityMonitor::RegisterFd_Internal(PROsfd const aOsfd, nsString host, uint16_t port)

Warning: The parameter 'host' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review209224

> I have added a mutex, and that means adding more methods to the class to use it from the static methods. I find this singleton design a bit clunky (where there's a single global instance dealing with static calls) but I guess we can arrange the design later.
> 
> I am going to run try again this morning. Let me know how you run it btw, so I can try it the same way.

I am also wondering how come the timer is triggered in the mochi tests since there's only one xpcshell test that changes the pref
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review209356


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/base/NetworkActivityMonitor.cpp:431
(Diff revision 25)
>      }
> +    return gInstance->RegisterFd_Internal(osfd, NS_ConvertUTF8toUTF16(host), port);
> -  }
> +}
>  
> +nsresult
> +NetworkActivityMonitor::RegisterFd_Internal(PROsfd const aOsfd, nsString host, uint16_t port)

Warning: The parameter 'host' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review209360


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: netwerk/base/NetworkActivityMonitor.cpp:427
(Diff revision 26)
>      }
> +    return gInstance->RegisterFd_Internal(osfd, NS_ConvertUTF8toUTF16(host), port);
> -  }
> +}
>  
> +nsresult
> +NetworkActivityMonitor::RegisterFd_Internal(PROsfd const aOsfd, nsString host, uint16_t port)

Warning: The parameter 'host' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review209224

> I am also wondering how come the timer is triggered in the mochi tests since there's only one xpcshell test that changes the pref

ok I think everything is now fixed. The pref was not unset, that's why others tests were impacted. Running one last big try with the pref ON to make sure it does work and not busting, once try is happy I will put back the pref to 0 in all.js.
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review210628

::: netwerk/base/NetworkActivityMonitor.h:69
(Diff revision 32)
>    };
>  
>    NetworkActivityMonitor();
> -  ~NetworkActivityMonitor();
>  
> -  static nsresult Init(int32_t blipInterval);
> +  NS_DECL_ISUPPORTS

Use NS_DECL_THREADSAFE_ISUPPORTS

::: netwerk/base/NetworkActivityMonitor.h:86
(Diff revision 32)
> +  nsresult DataInOut_Internal(PROsfd aOsfd, Direction aDirection, uint32_t aAmount);
>  private:
> -  nsresult Init_Internal(int32_t blipInterval);
> -  void PostNotification(Direction direction);
> -
> -  static NetworkActivityMonitor * gInstance;
> +  virtual ~NetworkActivityMonitor();
> +  nsresult Init_Internal(int32_t aInterval);
> +  nsresult Shutdown_Internal();
> +  static NetworkActivityMonitor*  gInstance;

Don't declare it here. Instead we'll have only a StaticRefPtr<NetworkActivityMonitor> in the cpp file.

::: netwerk/base/NetworkActivityMonitor.cpp:283
(Diff revision 32)
>  private:
> -  NetworkActivityMonitor::Direction mDirection;
> +  nsTArray<SocketActivity> mActivities;
>  };
>  
> +
>  NetworkActivityMonitor * NetworkActivityMonitor::gInstance = nullptr;

This should be a StaticRefPtr

::: netwerk/base/NetworkActivityMonitor.cpp:337
(Diff revision 32)
> +  }
>  
>    NetworkActivityMonitor * mon = new NetworkActivityMonitor();
> -  rv = mon->Init_Internal(blipInterval);
> +  rv = mon->Init_Internal(aInterval);
>    if (NS_FAILED(rv)) {
>      delete mon;

Use a RefPtr to hold mon, and don't call delete.

::: netwerk/base/NetworkActivityMonitor.cpp:352
(Diff revision 32)
>  {
> -  if (!gInstance)
> +  if (!gInstance) {
>      return NS_ERROR_NOT_INITIALIZED;
> -
> +  }
> +  gInstance->Shutdown_Internal();
>    delete gInstance;

It's not a good idea to manually delete something that implements nsISupports.
If we make gInstance a StaticRefPtr you can just null it out here.

::: netwerk/base/NetworkActivityMonitor.cpp:437
(Diff revision 32)
> +    if (PR_NetAddrToString(aAddr, _host, sizeof(_host) - 1) == PR_SUCCESS) {
> +      host.Assign(_host);
> +    } else {
> +      host.AppendPrintf("N/A");
> +    }
> +    return gInstance->RegisterFd_Internal(osfd, NS_ConvertUTF8toUTF16(host), port);

On all methods that are going to be called on the socket thread, we need to guard agains gInstance going away on the main thread.
So do
RefPtr<NetworkActivityMonitor> mon(gInstance);
if (mon) {
  mon->Register...
}

::: netwerk/base/NetworkActivityMonitor.cpp:491
(Diff revision 32)
> +      rv = gInstance->DataInOut_Internal(osfd, aDirection, aAmount);
> +    }
> +  }
> +  else {
> +    rv = NS_OK;
> +  }

nit: make rv = NS_OK when declaring it, and don't have an else branch.
Attachment #8930612 - Flags: review?(valentin.gosu)
Comment on attachment 8930612 [details]
Bug 1415536 - Extend NotifyNetworkActivity to get sent/received bytes

https://reviewboard.mozilla.org/r/201748/#review210806

Apart from a few minor issues, I think it all looks good.
r+ from me, with these fixed. Make sure to do a try push first.

::: netwerk/base/NetworkActivityMonitor.h:81
(Diff revision 34)
> -  static nsresult DataInOut(Direction direction);
> -
> +  static nsresult DataInOut(Direction aDirection, PRFileDesc *aFd, uint32_t aAmount);
> +  static nsresult RegisterFd(PRFileDesc *aFd, const PRNetAddr *aAddr);
> +  static nsresult UnregisterFd(PRFileDesc *aFd);
> +  nsresult RegisterFd_Internal(PROsfd aOsfd, const nsString& host, uint16_t port);
> +  nsresult UnregisterFd_Internal(PROsfd aOsfd);
> +  nsresult DataInOut_Internal(PROsfd aOsfd, Direction aDirection, uint32_t aAmount);

Internal methods should be private, no?

::: netwerk/base/NetworkActivityMonitor.cpp:293
(Diff revision 34)
>  
>  NetworkActivityMonitor::NetworkActivityMonitor()
> -  : mBlipInterval(PR_INTERVAL_NO_TIMEOUT)
> +  : mInterval(PR_INTERVAL_NO_TIMEOUT)
> +  , mLock("NetworkActivityMonitor::mLock")
>  {
> -  MOZ_COUNT_CTOR(NetworkActivityMonitor);
> +  NS_ASSERTION(gInstance, "multiple NetworkActivityMonitor instances!");

This condition is backwards.
Make it MOZ_ASSERT(!gInstance)

As you can see in the try push, it fails.

::: netwerk/base/NetworkActivityMonitor.cpp:329
(Diff revision 34)
>      return NS_ERROR_ALREADY_INITIALIZED;
> -
> -  NetworkActivityMonitor * mon = new NetworkActivityMonitor();
> -  rv = mon->Init_Internal(blipInterval);
> +  }
> +  gInstance = new NetworkActivityMonitor();
> +  rv = gInstance->Init_Internal(aInterval);
>    if (NS_FAILED(rv)) {
> -    delete mon;
> +    gInstance = nullptr;

A slightly better way would be
RefPtr<...> mon = new ...
if (NS_SUCCEDEED(rv)) gInstance = mon;
Attachment #8930612 - Flags: review?(valentin.gosu) → review+
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8f2d18830de
Extend NotifyNetworkActivity to get sent/received bytes r=baku,valentin
Yeah thanks Sebastian. I got a cleaner run here (minus the unrelated windows bust) 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8072fd9bb1cd92a9cf8578675519cd1f4948b95a

Looking at that leak now
Flags: needinfo?(tarek)
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b587f4d7e81
Extend NotifyNetworkActivity to get sent/received bytes r=baku,valentin
Flags: needinfo?(michal.novotny)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: