964270 - [NetworkStats API] Alarms methods do not work when data traffic is disabled

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Cost Control, defect)

Description

[Marina Rodríguez [:mai]]

When starts the mobile without enable data traffic, the alarms methods (getAllAlarms, addAlarm,...) of NetworkStats API do not works.

Comment 1

[Albert [:albert]]

Attachment: Patch

Added status to network object in connections array to avoid duplicated code when validating networks and enable alarm functions for networks that hasn't established a connection yet.

Comment 2

[Albert [:albert]]

Attachment: Patch

Patch updated from test results

Comment 3

[Albert [:albert]]

Attachment: Update tests

Comment 4

[Albert [:albert]]

Try server: https://tbpl.mozilla.org/?tree=Try&rev=4c08ea8dc3b3

Comment 5

[Noemí Freire (:noemi)]

nominating to v1.3? since it is an issue related to NetworkStats API already uplifted to v1.3. Thanks!

Comment 6

[Gene Lian (I already quit Mozilla)]

I need some time to review this next week because new mechanism has been added. I'll be stick to this it becomes urgent.

Anyway, we need Fabrice's approval before uplifting. I don't know if we're still allowed to uplift at this moment.

Comment 7

[[:fabrice] Fabrice Desré]

Is that a regression or a new feature?

Comment 8

[Albert [:albert]]

It is not a new feature but a bug related to the New Network Stats API already landed in v1.3 branch. Currently when trying to call alarms methods: getAllAlarms, addAlarm,... and data or wifi is off we obtain InvalidInterface error which is wrong since the interface is available. This patch allows to call the alarms method when the interface is detected.

Comment 9

[Preeti Raghunath(:Preeti)]

Marina

Please help understand the user impact if bug fix is not taken.

Also, please help with STRs as we are not sure if the issue is a regression or not.

Comment 10

[Marina Rodríguez [:mai]]

Hi, 
this bug produces that all alarm methods are unavailable when mobile traffic is disabled. This is transparent for the CC application, because the application doesn't need the mobile data is enabled to run.

The user impacts when mobile data is disabled are:
* The user cannot deactivate the alarms.
* The user cannot modify the alarms limit.
* The user cannot add a new alarm.
* When the user o the system clears the network traffic, the application cannot restore the alarm, whence the alarm never fires when the mobile traffic is restaured.

Regards

Comment 11

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8368120 [details] [diff] [review]
Patch

Review of attachment 8368120 [details] [diff] [review]:
-----------------------------------------------------------------

This is a great patch, except for some missing comments that I hope to see. Directly give review+ because this bug seems urgent.

::: dom/network/src/NetworkStatsService.jsm
@@ +30,5 @@
>  const NET_TYPE_MOBILE = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
>  
> +const NETWORK_STATUS_READY   = 0;
> +const NETWORK_STATUS_STANDBY = 1;
> +const NETWORK_STATUS_AWAY    = 2;

Please add explicit comments for the purpose of these 3 types, to make the followers easily to maintain the codes.

@@ +291,5 @@
>    getNetworkId: function getNetworkId(aIccId, aNetworkType) {
>      return aIccId + '' + aNetworkType;
>    },
>  
> +  validateNetwork: function validateNetwork(aNetwork, aCallback) {

Add a simple comment above this function:

// aCallback is signatured as |function(netId)|.

@@ +300,5 @@
> +      return;
> +    }
> +
> +    // Check if network is valid (RIL entry) but has not established a connection yet.
> +    // If So add to networks list with empty interfaceName.

s/So/so, /

@@ +313,5 @@
> +    }
> +
> +    // Check if network is available in the DB.
> +    this._db.isNetworkAvailable(aNetwork, function(aError, aResult) {
> +      let result = null;

Drop this line.

@@ +316,5 @@
> +    this._db.isNetworkAvailable(aNetwork, function(aError, aResult) {
> +      let result = null;
> +
> +      if (aResult) {
> +        result = netId;

Drop this line.

@@ +321,5 @@
> +
> +        this._networks[netId] = Object.create(null);
> +        this._networks[netId].network = aNetwork;
> +        this._networks[netId].status = NETWORK_STATUS_AWAY;
> +        this._currentAlarms[netId] = Object.create(null);

aCallback(netId);
return;

@@ +324,5 @@
> +        this._networks[netId].status = NETWORK_STATUS_AWAY;
> +        this._currentAlarms[netId] = Object.create(null);
> +      }
> +
> +      aCallback(result);

aCallback(null);

Just because of too many |result|s.

@@ +501,5 @@
>        lastElement = { netId: netId,
>                        queueIndex: this.updateQueueIndex(netId)};
>  
>        if (lastElement.queueIndex == -1) {
>          elements.push({netId: lastElement.netId, callbacks: []});

nit: since you're here please add spaces around "{" and "}".

@@ +505,5 @@
>          elements.push({netId: lastElement.netId, callbacks: []});
>        }
>      }
>  
> +    if (!lastElement) {

Please add a comment for why you want to do early return here. I don't get this well.

@@ +960,5 @@
>  
>    _setAlarm: function _setAlarm(aAlarm, aCallback) {
>      let currentAlarm = this._currentAlarms[aAlarm.networkId];
> +    if ((Object.getOwnPropertyNames(currentAlarm).length !== 0 &&
> +        aAlarm.absoluteThreshold > currentAlarm.alarm.absoluteThreshold) ||

Please align "aAlarm" to "Object", instead of the "(".

@@ +1012,5 @@
>            aCallback(error, result);
>            return;
>          }
>  
> +        if (!result) {

Could you please add some comments for this to clarify me?

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ for new API bug. it could be worse to backout the new API so the better path forward is to fix bugs related to the new API

Comment 13

[Albert [:albert]]

Attachment: Patch

Fixes from comment 11

Comment 14

[Albert [:albert]]

Attachment: Parch

Sorry I forgot the qrefresh in the previous patch. This is the correct.

Comment 15

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8370614 [details] [diff] [review]
Parch

Review of attachment 8370614 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/network/src/NetworkStatsService.jsm
@@ +28,5 @@
>  const TOPIC_INTERFACE_UNREGISTERED = "network-interface-unregistered";
>  const NET_TYPE_WIFI = Ci.nsINetworkInterface.NETWORK_TYPE_WIFI;
>  const NET_TYPE_MOBILE = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
>  
> +// Networks have different status that NetworkStats API needs to aware.

s/aware/be aware of/

aware is adj. ;)

@@ +29,5 @@
>  const NET_TYPE_WIFI = Ci.nsINetworkInterface.NETWORK_TYPE_WIFI;
>  const NET_TYPE_MOBILE = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
>  
> +// Networks have different status that NetworkStats API needs to aware.
> +// Network present and ready, so NetworkManager provide whole info.

s/present/is present/
s/provide/provides/
s/whole/the whole/

@@ +31,5 @@
>  
> +// Networks have different status that NetworkStats API needs to aware.
> +// Network present and ready, so NetworkManager provide whole info.
> +const NETWORK_STATUS_READY   = 0;
> +// Network present but hasn't established a connection yet (e.g. SIM that has not

s/present/is present/

@@ +34,5 @@
> +const NETWORK_STATUS_READY   = 0;
> +// Network present but hasn't established a connection yet (e.g. SIM that has not
> +// enabled 3G since boot).
> +const NETWORK_STATUS_STANDBY = 1;
> +// Network not present, but stored in database from previous connections.

s/not/is not/
s/from/by the/

@@ +299,5 @@
>  
> +  /* Function to ensure that one network is valid. The network is valid if its status is
> +   * NETWORK_STATUS_READY, NETWORK_STATUS_STANDBY or NETWORK_STATUS_AWAY.
> +   *
> +   * The result is null if the network is not valid or the netId if it is.

* The result is null if the network or its netId is not valid.

@@ +971,5 @@
>    _setAlarm: function _setAlarm(aAlarm, aCallback) {
>      let currentAlarm = this._currentAlarms[aAlarm.networkId];
> +    if ((Object.getOwnPropertyNames(currentAlarm).length !== 0 &&
> +         aAlarm.absoluteThreshold > currentAlarm.alarm.absoluteThreshold) ||
> +         this._networks[aAlarm.networkId].status != NETWORK_STATUS_READY) {

Please align "this._networks..." to the "(Object...".

Comment 16

[Albert [:albert]]

Attachment: Patch

Changes from commemt 15

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3dd8d8379d2a
https://hg.mozilla.org/integration/b2g-inbound/rev/c2852f280c71

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3dd8d8379d2a
https://hg.mozilla.org/mozilla-central/rev/c2852f280c71

Comment 19

[Noemí Freire (:noemi)]

Hi John,

Could you please help us on uplifting this patch to v1.3 branch?. Many Thanks!!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

It's a Gecko patch, I'll get it later today when I'm doing other 1.3 uplifts.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This needs some rebasing for v1.3.

Comment 22

[Albert [:albert]]

Attachment: Patch for 1.3

Comment 23

[Noemí Freire (:noemi)]

Hi Ryan,

Patch for 1.3 ready, could you please help us with the uplift to v1.3 branch?. Many Thanks!

Comment 24

[Albert [:albert]]

Attachment: Patch for 1.3

Sorry, last patch was wrong.

Comment 25

[Albert [:albert]]

Ryan, tests patch is ok for both master and 1.3

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/290cb088fe58
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/859cf34d6a18

Comment 27

[Jason Smith [:jsmith]]

Renoming for more triage discussion. If this gets a minus, we'll back out.

Comment 28

[Noemí Freire (:noemi)]

Hi,

The patch rebased for v1.3 corresponding to this bug was the one causing Bug 971696 in that branch so it is the one that needs to be backouted from v1.3 branch.

Comment 29

[Jason Smith [:jsmith]]

(In reply to Noemí Freire (:noemi) from comment #28)
> Hi,
> 
> The patch rebased for v1.3 corresponding to this bug was the one causing Bug
> 971696 in that branch so it is the one that needs to be backouted from v1.3
> branch.

Okay. Flagging checkin-needed then to backout:

https://bugzilla.mozilla.org/show_bug.cgi?id=964270#c26

Comment 30

[David Scravaglieri [:scravag]]

Patch is needed for Tarako version

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Backed out per comment 29.
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/89143e01361b

Comment 32

[Joe Cheng [:jcheng] (please needinfo)]

traige: 1.3T+ to get this into tarako for usage app memory saving

Comment 33

[Joe Cheng [:jcheng] (please needinfo)]

Hi Ying Xu, heard that you will be doing uplifts to 1.3T branch. After you completed the uplift, can you please set status-b2g-v1.3T to fixed? please let us know if you have problems with it. thanks

Comment 34

[Albert [:albert]]

Attachment: Patch for 1.3t

Comment 35

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/c10f087e5944
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/120ff6e27c87