If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 30, Firefox OS v1.3T

Status

Firefox OS
Gaia::Cost Control
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mai, Assigned: albert)

Tracking

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

Firefox Tracking Flags

(blocking-b2g:1.3T+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.3 affected, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

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

Updated

4 years ago
Assignee: nobody → acperez
Blocks: 858017
(Assignee)

Updated

4 years ago
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(Assignee)

Comment 1

4 years ago
Created attachment 8367854 [details] [diff] [review]
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.
Attachment #8367854 - Flags: review?(gene.lian)
(Assignee)

Updated

4 years ago
Blocks: 965319
(Assignee)

Comment 2

4 years ago
Created attachment 8368120 [details] [diff] [review]
Patch

Patch updated from test results
Attachment #8367854 - Attachment is obsolete: true
Attachment #8367854 - Flags: review?(gene.lian)
Attachment #8368120 - Flags: review?(gene.lian)
(Assignee)

Comment 3

4 years ago
Created attachment 8368121 [details] [diff] [review]
Update tests
Attachment #8368121 - Flags: review?(gene.lian)
(Assignee)

Comment 4

4 years ago
Try server: https://tbpl.mozilla.org/?tree=Try&rev=4c08ea8dc3b3
nominating to v1.3? since it is an issue related to NetworkStats API already uplifted to v1.3. Thanks!
blocking-b2g: --- → 1.3?
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.
Flags: needinfo?(fabrice)
Is that a regression or a new feature?
Flags: needinfo?(fabrice)
(Assignee)

Comment 8

4 years ago
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.
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.
Flags: needinfo?(mri)
(Reporter)

Comment 10

4 years ago
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
Flags: needinfo?(mri)
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?
Attachment #8368120 - Flags: review?(gene.lian) → review+
Attachment #8368121 - Flags: review?(gene.lian) → review+
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
blocking-b2g: 1.3? → 1.3+
(Assignee)

Updated

4 years ago
Whiteboard: ETA: 2/6
(Assignee)

Comment 13

4 years ago
Created attachment 8370612 [details] [diff] [review]
Patch

Fixes from comment 11
Attachment #8368120 - Attachment is obsolete: true
Attachment #8370612 - Flags: feedback?(gene.lian)
(Assignee)

Comment 14

4 years ago
Created attachment 8370614 [details] [diff] [review]
Parch

Sorry I forgot the qrefresh in the previous patch. This is the correct.
Attachment #8370612 - Attachment is obsolete: true
Attachment #8370612 - Flags: feedback?(gene.lian)
Attachment #8370614 - Flags: feedback?(gene.lian)
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...".
Attachment #8370614 - Flags: feedback?(gene.lian) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 8370660 [details] [diff] [review]
Patch

Changes from commemt 15
Attachment #8370614 - Attachment is obsolete: true
Attachment #8370660 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/3dd8d8379d2a
https://hg.mozilla.org/integration/b2g-inbound/rev/c2852f280c71
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3dd8d8379d2a
https://hg.mozilla.org/mozilla-central/rev/c2852f280c71
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S1 (14feb)

Updated

4 years ago
status-b2g-v1.3: --- → affected
Hi John,

Could you please help us on uplifting this patch to v1.3 branch?. Many Thanks!!
Flags: needinfo?(jhford)
It's a Gecko patch, I'll get it later today when I'm doing other 1.3 uplifts.
Flags: needinfo?(jhford)
This needs some rebasing for v1.3.
Flags: needinfo?(acperez)
Keywords: branch-patch-needed
Whiteboard: ETA: 2/6
(Assignee)

Comment 22

4 years ago
Created attachment 8373957 [details] [diff] [review]
Patch for 1.3
Attachment #8373957 - Flags: review+
Flags: needinfo?(acperez)
Hi Ryan,

Patch for 1.3 ready, could you please help us with the uplift to v1.3 branch?. Many Thanks!
Flags: needinfo?(ryanvm)
(Assignee)

Comment 24

4 years ago
Created attachment 8373959 [details] [diff] [review]
Patch for 1.3

Sorry, last patch was wrong.
Attachment #8373957 - Attachment is obsolete: true
Attachment #8373959 - Flags: review+
(Assignee)

Comment 25

4 years ago
Ryan, tests patch is ok for both master and 1.3
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/290cb088fe58
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/859cf34d6a18
status-b2g-v1.3: affected → fixed
status-b2g-v1.4: --- → fixed
status-firefox28: --- → wontfix
status-firefox29: --- → wontfix
status-firefox30: --- → fixed
Flags: needinfo?(ryanvm)
Keywords: branch-patch-needed
Renoming for more triage discussion. If this gets a minus, we'll back out.
blocking-b2g: 1.3+ → 1.3?
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.

Updated

4 years ago
Depends on: 971696
(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
Keywords: checkin-needed
Patch is needed for Tarako version
blocking-b2g: 1.3? → 1.3T?
Backed out per comment 29.
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/89143e01361b
status-b2g-v1.3: fixed → wontfix
Keywords: checkin-needed
status-b2g-v1.3: wontfix → affected
traige: 1.3T+ to get this into tarako for usage app memory saving
blocking-b2g: 1.3T? → 1.3T+
status-b2g-v1.3T: --- → affected
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
Flags: needinfo?(ying.xu)
(Assignee)

Comment 34

4 years ago
Created attachment 8389502 [details] [diff] [review]
Patch for 1.3t
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/c10f087e5944
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/120ff6e27c87
status-b2g-v1.3T: affected → fixed

Updated

4 years ago
Flags: needinfo?(ying.xu)
You need to log in before you can comment on or make changes to this bug.