Closed Bug 879793 Opened 7 years ago Closed 7 years ago

NetworkStatistics API does not update network statistics if the interface is switched off.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:-, b2g-v2.1 verified)

VERIFIED FIXED
blocking-b2g -
Tracking Status
b2g-v2.1 --- verified

People

(Reporter: salva, Assigned: albert)

References

Details

(Whiteboard: c=)

Attachments

(3 files, 4 obsolete files)

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
Attachment #758975 - Flags: review?(gene.lian)
Blocks: 879790
Since Bug 879770 just landed, I don't know why do you remove that again? Wouldn't it cause regression?
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.
Rebase
Attachment #758975 - Attachment is obsolete: true
Attachment #758975 - Flags: review?(gene.lian)
Attachment #759313 - Flags: review?(gene.lian)
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)
Modified following gene comments and fix one problem found when modifying tests.
Attachment #759313 - Attachment is obsolete: true
Attachment #759802 - Flags: review?(gene.lian)
Attached patch Tests (obsolete) — Splinter Review
Attachment #759803 - Flags: review?(gene.lian)
Blocks: 858005
Daniel, would you like to block on this?
Flags: needinfo?(dcoloma)
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 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+
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)
The data is updated later, no stats are lost.
Flags: needinfo?(acperez)
blocking-b2g: tef? → leo?
This is not a blocker for leo either then given comments 11 and 12.
blocking-b2g: leo? → -
Whiteboard: c=
Attached patch Tests (obsolete) — Splinter Review
Changes from comment 10
Attachment #759803 - Attachment is obsolete: true
Attached patch TestsSplinter Review
Fixed test_networkStatsAvailable_failure()
Attachment #760916 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/32ea84579cfb
https://hg.mozilla.org/mozilla-central/rev/e50f7dd54daf
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attached video video of issue verify
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.