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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Salvador de la Puente González [:salva]]

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

Comment 1

[Albert [:albert]]

Attachment: Add an observer to update stats when interface goes down

Comment 2

[Gene Lian (I already quit Mozilla)]

Since Bug 879770 just landed, I don't know why do you remove that again? Wouldn't it cause regression?

Comment 3

[Albert [:albert]]

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.

Comment 4

[Albert [:albert]]

Attachment: Add an observer to update stats when interface goes down v2

Rebase

Comment 5

[Gene Lian (I already quit Mozilla)]

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.

Comment 6

[Albert [:albert]]

Attachment: Add an observer to update stats when interface goes down v3

Modified following gene comments and fix one problem found when modifying tests.

Comment 7

[Albert [:albert]]

Attachment: Tests

Comment 8

[Andrew Overholt [:overholt]]

Daniel, would you like to block on this?

Comment 9

[Gene Lian (I already quit Mozilla)]

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!

Comment 10

[Gene Lian (I already quit Mozilla)]

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!

Comment 11

[Daniel Coloma:dcoloma]

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.

Comment 12

[Albert [:albert]]

The data is updated later, no stats are lost.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

This is not a blocker for leo either then given comments 11 and 12.

Comment 14

[Albert [:albert]]

Attachment: Tests

Changes from comment 10

Comment 15

[Albert [:albert]]

Attachment: Tests

Fixed test_networkStatsAvailable_failure()

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/32ea84579cfb
https://hg.mozilla.org/projects/birch/rev/e50f7dd54daf

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32ea84579cfb
https://hg.mozilla.org/mozilla-central/rev/e50f7dd54daf

Comment 18

[Sue]

Attachment: 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