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

VERIFIED FIXED

Status

Firefox OS
General
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: salva, Assigned: albert)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: c=)

Attachments

(3 attachments, 4 obsolete attachments)

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

5 years ago
Created attachment 758975 [details] [diff] [review]
Add an observer to update stats when interface goes down
(Assignee)

Updated

5 years ago
Attachment #758975 - Flags: review?(gene.lian)
(Assignee)

Updated

5 years ago
Blocks: 879790
Since Bug 879770 just landed, I don't know why do you remove that again? Wouldn't it cause regression?
(Assignee)

Comment 3

5 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

5 years ago
Created attachment 759313 [details] [diff] [review]
Add an observer to update stats when interface goes down v2

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)
(Assignee)

Comment 6

5 years ago
Created attachment 759802 [details] [diff] [review]
Add an observer to update stats when interface goes down v3

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

5 years ago
Created attachment 759803 [details] [diff] [review]
Tests
Attachment #759803 - Flags: review?(gene.lian)
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 12

5 years ago
The data is updated later, no stats are lost.
Flags: needinfo?(acperez)

Updated

5 years ago
blocking-b2g: tef? → leo?
This is not a blocker for leo either then given comments 11 and 12.
blocking-b2g: leo? → -
Whiteboard: c=
(Assignee)

Comment 14

5 years ago
Created attachment 760916 [details] [diff] [review]
Tests

Changes from comment 10
Attachment #759803 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Created attachment 760923 [details] [diff] [review]
Tests

Fixed test_networkStatsAvailable_failure()
Attachment #760916 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/32ea84579cfb
https://hg.mozilla.org/projects/birch/rev/e50f7dd54daf
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/32ea84579cfb
https://hg.mozilla.org/mozilla-central/rev/e50f7dd54daf
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
status-b2g-v2.1: --- → fixed

Updated

3 years ago
status-b2g-v2.1: fixed → verified

Comment 18

3 years ago
Created attachment 8530110 [details]
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

Updated

3 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.