Closed
Bug 879793
Opened 11 years ago
Closed 11 years ago
NetworkStatistics API does not update network statistics if the interface is switched off.
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-b2g:-, b2g-v2.1 verified)
People
(Reporter: salva, Assigned: albert)
References
Details
(Whiteboard: c=)
Attachments
(3 files, 4 obsolete files)
12.14 KB,
patch
|
airpingu
:
review+
|
Details | Diff | Splinter Review |
6.48 KB,
patch
|
Details | Diff | Splinter Review | |
4.99 MB,
video/mp4
|
Details |
STR: 1- Enable data on 3G 2- Browse the internet 3- Check Usage application and see the data usage 4- Browse the internet (see some videos to consume some MB more) 5- Disable Usage 6- Check Usage application Expected: The data usage is updated Actual: The data usage remains as in the step 3
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #758975 -
Flags: review?(gene.lian)
Comment 2•11 years ago
|
||
Since Bug 879770 just landed, I don't know why do you remove that again? Wouldn't it cause regression?
Assignee | ||
Comment 3•11 years ago
|
||
As happened with Bug 879770, here the problem is related with the network interfaces management in the NetworkManager. Although consumption data can be checked by the network daemon when an interface is down (which reads the file /proc/net/dev), the NetworkManager removes NetworkInterfaces when they are unregistered. It is a problem because the NetworkStatsService requests the usage stats through the networkManager passing it the connection type (mobile or wifi), so the manager has to find the network interface name for the active connection type and, since the interface is down it cannot resolve the interface name until the connection is switched on. To fix it, the patch avoid to resolve the network interface name using the connection type. To do that, the NetworkStatsService will store the active connection NetworkInterface object coming in network-interface-registered and network-interface-unregistered events and passing the interface name instead of the connection type. This fix will fix Bug 879770 as well.
Assignee | ||
Comment 4•11 years ago
|
||
Rebase
Attachment #758975 -
Attachment is obsolete: true
Attachment #758975 -
Flags: review?(gene.lian)
Attachment #759313 -
Flags: review?(gene.lian)
Comment 5•11 years ago
|
||
Comment on attachment 759313 [details] [diff] [review] Add an observer to update stats when interface goes down v2 Review of attachment 759313 [details] [diff] [review]: ----------------------------------------------------------------- Please add the following two lines at the case "xpcom-shutdown": Services.obs.removeObserver(this, TOPIC_INTERFACE_REGISTERED); Services.obs.removeObserver(this, TOPIC_INTERFACE_UNREGISTERED); Btw, I don't think you have fixed the test cases at [1]. Right? [1] dom/network/tests/unit_stats/test_networkstats_service.js: ::: dom/network/src/NetworkStatsService.jsm @@ +50,5 @@ > > this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > > this._connectionTypes = Object.create(null); > + this._connectionTypes[NETWORK_TYPE_WIFI] = { name: "wifi", network: Object.create(null) }; Please fold this line: this._connectionTypes[NETWORK_TYPE_WIFI] = { name: "wifi", network: Object.create(null) }; @@ +51,5 @@ > this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > > this._connectionTypes = Object.create(null); > + this._connectionTypes[NETWORK_TYPE_WIFI] = { name: "wifi", network: Object.create(null) }; > + this._connectionTypes[NETWORK_TYPE_MOBILE] = { name: "mobile", network: Object.create(null) }; Ditto. Please fold this line. ::: dom/system/gonk/NetworkManager.js @@ +456,5 @@ > params.report = true; > params.isAsync = true; > > this.controlMessage(params, function(result) { > let success = result.resultCode >= NETD_COMMAND_OKAY && result.resultCode < NETD_COMMAND_ERROR; Since you're here, could you please fold this: let success = result.resultCode >= NETD_COMMAND_OKAY && result.resultCode < NETD_COMMAND_ERROR; @@ +457,5 @@ > params.isAsync = true; > > this.controlMessage(params, function(result) { > let success = result.resultCode >= NETD_COMMAND_OKAY && result.resultCode < NETD_COMMAND_ERROR; > + callback.networkStatsAvailable(success, result.rxBytes, result.txBytes, result.date); and callback.networkStatsAvailable(success, result.rxBytes, result.txBytes, result.date); ::: dom/system/gonk/nsINetworkManager.idl @@ +103,3 @@ > interface nsINetworkStatsCallback : nsISupports > { > void networkStatsAvailable(in boolean success, I used to be confused about the IDL function name |networkStatsAvailable| here and the |NetworkStatsService.networkStatsAvailable(...)|. Not bothering. It's fine.
Attachment #759313 -
Flags: review?(gene.lian)
Assignee | ||
Comment 6•11 years ago
|
||
Modified following gene comments and fix one problem found when modifying tests.
Attachment #759313 -
Attachment is obsolete: true
Attachment #759802 -
Flags: review?(gene.lian)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #759803 -
Flags: review?(gene.lian)
Comment 9•11 years ago
|
||
Comment on attachment 759802 [details] [diff] [review] Add an observer to update stats when interface goes down v3 Review of attachment 759802 [details] [diff] [review]: ----------------------------------------------------------------- Nice patch!
Attachment #759802 -
Flags: review?(gene.lian) → review+
Comment 10•11 years ago
|
||
Comment on attachment 759803 [details] [diff] [review] Tests Review of attachment 759803 [details] [diff] [review]: ----------------------------------------------------------------- There is still one line wrong in this patch, where you didn't fix: .networkStatsAvailable(...) in the following test: add_test(function test_networkStatsAvailable_failure() {...}); Also, please fold the following two lines to avoid exceeding 80 chars, which is a general coding convention. Also, please use |;| at the tails of JS statements instead of |,|. NetworkStatsService._connectionTypes[Ci.nsINetworkInterface.NETWORK_TYPE_WIFI] .network.name = 'wlan0'; NetworkStatsService._connectionTypes[Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE] .network.name = 'rmnet0'; Because I'll take PTO next Monday, I directly give you review+ in case that this would become tef+ so that you can land this quickly. Please do fix the above comments and run tests again before landing. Thanks!
Attachment #759803 -
Flags: review?(gene.lian) → review+
Comment 11•11 years ago
|
||
I don't think this should be tef+ if the data is updated later on, Albert, Salva, does the stats finally update or that data is never taking into account.
Flags: needinfo?(dcoloma) → needinfo?(acperez)
Assignee | ||
Comment 12•11 years ago
|
||
The data is updated later, no stats are lost.
Flags: needinfo?(acperez)
Updated•11 years ago
|
blocking-b2g: tef? → leo?
Comment 13•11 years ago
|
||
This is not a blocker for leo either then given comments 11 and 12.
blocking-b2g: leo? → -
Whiteboard: c=
Assignee | ||
Comment 14•11 years ago
|
||
Changes from comment 10
Attachment #759803 -
Attachment is obsolete: true
Assignee | ||
Comment 15•11 years ago
|
||
Fixed test_networkStatsAvailable_failure()
Attachment #760916 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 16•11 years ago
|
||
https://hg.mozilla.org/projects/birch/rev/32ea84579cfb https://hg.mozilla.org/projects/birch/rev/e50f7dd54daf
Keywords: checkin-needed
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/32ea84579cfb https://hg.mozilla.org/mozilla-central/rev/e50f7dd54daf
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Updated•10 years ago
|
status-b2g-v2.1:
--- → fixed
Comment 18•10 years ago
|
||
This issue has been verified successfully on Flame v2.1 See attachment: verify_video.MP4 Reproducing rate: 0/5 Flame 2.1 versions: Gaia-Rev 5372b675e018b6aac97d95ff5db8d4bd16addb9b Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/f34377ae402b Build-ID 20141127001201 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20141127.035005 FW-Date Thu Nov 27 03:50:16 EST 2014 Bootloader L1TC00011880
You need to log in
before you can comment on or make changes to this bug.
Description
•