Closed
Bug 1415536
Opened 7 years ago
Closed 6 years ago
Extend NotifyNetworkActivity
Categories
(Core :: Networking, enhancement, P5)
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 | ||
Updated•7 years ago
|
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.
Assignee | ||
Comment 2•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
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 7•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 9•6 years ago
|
||
mozreview-review |
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":)"
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review-reply |
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 14•6 years ago
|
||
mozreview-review |
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); ```
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review-reply |
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8931259 -
Attachment is obsolete: true
Comment 19•6 years ago
|
||
mozreview-review |
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;
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•6 years ago
|
||
> 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 22•6 years ago
|
||
mozreview-review |
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 23•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 25•6 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 26•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 33•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 43•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 45•6 years ago
|
||
mozreview-review-reply |
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 46•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 48•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 50•6 years ago
|
||
mozreview-review |
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]
Assignee | ||
Comment 51•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 53•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 55•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 60•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3596f38753934c6dda11cf8da9ff2fe5308e474e
Comment hidden (mozreview-request) |
Assignee | ||
Comment 62•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 64•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 67•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 71•6 years ago
|
||
Pushed by tziade@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e8f2d18830de Extend NotifyNetworkActivity to get sent/received bytes r=baku,valentin
Comment hidden (mozreview-request) |
Comment 73•6 years ago
|
||
Backed out changeset e8f2d18830de (bug 1415536) for failing xpcshell netwerk/test/unit/test_network_activity.js at least on Android r=backout on a CLOSED TREE https://hg.mozilla.org/integration/autoland/rev/fe49764e750b4fe5e133a4075bea613ad5008b6e https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e8f2d18830de12d7cb6e83a3aadbfd7571c5a4f6&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Flags: needinfo?(tarek)
Comment hidden (mozreview-request) |
Comment 75•6 years ago
|
||
Valgrind has detected a leak for the patch which got backed out later: https://treeherder.mozilla.org/logviewer.html#?job_id=149790518&repo=autoland
Assignee | ||
Comment 76•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 83•6 years ago
|
||
Pushed by tziade@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1b587f4d7e81 Extend NotifyNetworkActivity to get sent/received bytes r=baku,valentin
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(michal.novotny)
Assignee | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment 84•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1b587f4d7e81
You need to log in
before you can comment on or make changes to this bug.
Description
•