Closed Bug 944051 Opened 6 years ago Closed 6 years ago

[Cost Control] Cost Control is not detecting usage if enabling 3G data while the application is already open

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+)

RESOLVED FIXED
1.3 Sprint 6 - 12/6
blocking-b2g 1.3+

People

(Reporter: salva, Assigned: albert)

Details

Attachments

(1 file, 3 obsolete files)

STR

1. Flash Gaia and pass through FTU without enabling data nor wifi
2. Open Cost Control application, pass the FTU and see how there is no data for mobile chart
3. Send Cost Control to the background (not close!) by pressing home
4. Enable 3G and browse Internet
5. Go to the task manager and bring Cost Control to the foreground

Expected:
The chart is repainted and the mobile consumption is displayed.

Actual:
No mobile consumption is shown.

This only happens the very first time we enable 3G data usage after Cost Control.

This is a back-end related bug as it has not any form of communicate the available networks have change since the activation of the SIM interface.
Assignee: nobody → acperez
triage: 1.3+ broken feature
blocking-b2g: 1.3? → 1.3+
Attached patch Partch (obsolete) — Splinter Review
Added a request to ril to return all networks when calling 'getAvailableNetworks' , so usage application can get networks that haven't configured a data connection.
Attachment #8340367 - Flags: review?(jshih)
Attachment #8340367 - Flags: review?(gene.lian)
Comment on attachment 8340367 [details] [diff] [review]
Partch

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +515,5 @@
>            aTxn.result.push({ id: cursor.key[0],
>                               type: cursor.key[1] });
> +
> +          let netId = cursor.key[0] + '' + cursor.key[1];
> +          if (defaultNetworks[netId]) {

little confused here, so you are deleting the network in the list if there is found a matching one in db?

::: dom/network/src/NetworkStatsService.jsm
@@ +340,5 @@
> +      // Check if network is correct but have not stablished a connection yet.
> +      let rilNetworks = this.getRilNetworks();
> +      if (rilNetworks[netId]) {
> +        error = null;
> +        result = true;

you should also change the line below to

mm.sendAsyncMessage("NetworkStats:Clear:Return",
{ id: msg.id, error: error, result: result });

right?
Attachment #8340367 - Flags: review?(jshih)
Attached patch Patch (obsolete) — Splinter Review
Changes from comment 3
Attachment #8340367 - Attachment is obsolete: true
Attachment #8340367 - Flags: review?(gene.lian)
Attachment #8340472 - Flags: review?(jshih)
Attachment #8340472 - Flags: review?(gene.lian)
(In reply to John Shih from comment #3)
> Comment on attachment 8340367 [details] [diff] [review]
> Partch
> 
> Review of attachment 8340367 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/NetworkStatsDB.jsm
> @@ +515,5 @@
> >            aTxn.result.push({ id: cursor.key[0],
> >                               type: cursor.key[1] });
> > +
> > +          let netId = cursor.key[0] + '' + cursor.key[1];
> > +          if (defaultNetworks[netId]) {
> 
> little confused here, so you are deleting the network in the list if there
> is found a matching one in db?

Suppose first system start and you don't switch on mobile data. When usage application calls 'getAvailableNetworks', then NetworkStatsService gets ril networkinterfaces to obtain iccids, generates an array of nsIDOMMozNetworkStatsInterfaces and pass it to NetworkStatsDB. What I'm doing here is filling the result with networks found in the database and remove those ones from ril networkinterfaces, so finally I have in defaultNetworks the 3g network that still has not been activated but is valid. Then, after obtain all networks from database I push the contents of defaultNetworks to the result.

If I don't delete, some networks will be duplicated.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Attachment #8340472 - Flags: review?(jshih) → review+
Comment on attachment 8340472 [details] [diff] [review]
Patch

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +501,5 @@
>                     date: new Date(aData[aData.length - 1].date.getTime() + SAMPLE_RATE) });
>      }
>    },
>  
> +  getAvailableNetworks: function getAvailableNetworks(defaultNetworks, aResultCb) {

This doesn't sound reasonable to me. From the point of view of DB, NetworkStatsDB.getAvailableNetworks(...) is aimed to return the networks that used to be saved in the DB. Adding a |defaultNetworks| as parameter would break the intention of this function, which is out of the scope of DB operations.

Can we add the rilNetworks at the point where this function returns?

::: dom/network/src/NetworkStatsService.jsm
@@ +194,5 @@
>     * nsINetworkStatsService
>     */
> +  getRilNetworks: function() {
> +    let networks = {};
> +    let interfaces = gRil.numRadioInterfaces;

s/interfaces/numRadioInterfaces/

@@ +195,5 @@
>     */
> +  getRilNetworks: function() {
> +    let networks = {};
> +    let interfaces = gRil.numRadioInterfaces;
> +    for (let i = 0; i < interfaces; i++) {

s/interfaces/numRadioInterfaces/

@@ +242,5 @@
>    },
>  
>    getAvailableNetworks: function getAvailableNetworks(mm, msg) {
> +    let rilNetworks = this.getRilNetworks();
> +    this._db.getAvailableNetworks(rilNetworks, function onGetNetworks(aError, aResult) {

As mentioned above, I'd prefer adding the |rilNetworks| in the |aResult| here. Please let me know if you disagree. Thanks!

@@ +305,5 @@
>          }, network, start, end, appId, manifestURL);
>          return;
>        }
>  
>        if (!aError) {

I'd rewrite the logic (please also note some comments are polished):

// Check if the network is available in the DB. If yes, we also
// retrieve the stats for it from the DB.
this._db.isNetworkAvailable(network, function(aError, aResult) {
  let toFind = false;
  if (aResult) {
    toFind = true;
  } else if (!aError) {
    // Network is not found in the database without any errors.
    // Check if network is valid but has not established a connection yet.
    let rilNetworks = self.getRilNetworks();
    if (rilNetworks[netId]) {
      toFind = true;
    }
  }

  if (toFind) {
    // If network is not active, there is no need to update stats before finding.
    self._db.find(function onStatsFound(aError, aResult) {
      mm.sendAsyncMessage("NetworkStats:Get:Return",
                          { id: msg.id, error: aError, result: aResult });
    }, network, start, end, appId, manifestURL);
    return;
  }

  if (!aError)
    aError = "Invalid connectionType";
  }

  mm.sendAsyncMessage("NetworkStats:Get:Return",
                      { id: msg.id, error: aError, result: null });
}

@@ +310,5 @@
> +        // Network not found in database.
> +        // Check if network is correct but have not stablished a connection yet.
> +        let rilNetworks = self.getRilNetworks();
> +        if (rilNetworks[netId]) {
> +          self._db.find(function onStatsFound(aError, aResult) {

This confuses a bit. If network is not found in DB for sure, how can the .find(...) further return valid records?

@@ +335,5 @@
>      if (!this._networks[netId]) {
> +      let error = "Invalid networkType";
> +      let result = null;
> +
> +      // Network not found in database.

You don't need the first comment.

@@ +336,5 @@
> +      let error = "Invalid networkType";
> +      let result = null;
> +
> +      // Network not found in database.
> +      // Check if network is correct but have not stablished a connection yet.

s/correct/valid/
s/have/has/
s/stablished/established/
Attachment #8340472 - Flags: review?(gene.lian)
Attached patch Patch (obsolete) — Splinter Review
Changes from comment 6
Attachment #8340472 - Attachment is obsolete: true
Attachment #8341607 - Flags: review?(gene.lian)
I had a question in the previous comment. Could you please clarify that?

> +        // Network not found in database.
> +        // Check if network is correct but have not stablished a connection yet.
> +        let rilNetworks = self.getRilNetworks();
> +        if (rilNetworks[netId]) {
> +          self._db.find(function onStatsFound(aError, aResult) {

This confuses a bit. If network is not found in DB for sure, how can the .find(...) further return valid records?
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #8)
> I had a question in the previous comment. Could you please clarify that?
> 
> > +        // Network not found in database.
> > +        // Check if network is correct but have not stablished a connection yet.
> > +        let rilNetworks = self.getRilNetworks();
> > +        if (rilNetworks[netId]) {
> > +          self._db.find(function onStatsFound(aError, aResult) {
> 
> This confuses a bit. If network is not found in DB for sure, how can the
> .find(...) further return valid records?

'find' DB function not find data for that network, but is going to fill the result object with the fields needed to create a nsIDOMMozNetworkStats when passing the result to the manager.
Comment on attachment 8341607 [details] [diff] [review]
Patch

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

Please fix the following nits that I used to point out. Thanks!

::: dom/network/src/NetworkStatsService.jsm
@@ +247,5 @@
>      this._db.getAvailableNetworks(function onGetNetworks(aError, aResult) {
> +
> +      for (let netId in rilNetworks) {
> +        let found = false;
> +        for (var i = 0; i < aResult.length; i++) {

s/var/let/

@@ +351,5 @@
>      if (!this._networks[netId]) {
> +      let error = "Invalid networkType";
> +      let result = null;
> +
> +      // Network not found in database.

You don't need this comment.

@@ +352,5 @@
> +      let error = "Invalid networkType";
> +      let result = null;
> +
> +      // Network not found in database.
> +      // Check if network is correct but have not stablished a connection yet.

s/correct/valid
s/have/has
s/stablished/established
(In reply to Albert [:albert] from comment #9)
> 'find' DB function not find data for that network, but is going to fill the
> result object with the fields needed to create a nsIDOMMozNetworkStats when
> passing the result to the manager.

Still don't understand. If db.isNetworkAvailable(...) has ensured we cannot find any stats records by that network in the DB, why do we still want to call db.find(...) again? Can we directly call sendAsyncMessage(...) to return?
Comment on attachment 8341607 [details] [diff] [review]
Patch

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

::: dom/network/src/NetworkStatsService.jsm
@@ +247,3 @@
>      this._db.getAvailableNetworks(function onGetNetworks(aError, aResult) {
> +
> +      for (let netId in rilNetworks) {

Also, please add a comment before this for loop:

// Also return the networks that are valid but have not
// established connections yet.
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #11)
> (In reply to Albert [:albert] from comment #9)
> > 'find' DB function not find data for that network, but is going to fill the
> > result object with the fields needed to create a nsIDOMMozNetworkStats when
> > passing the result to the manager.
> 
> Still don't understand. If db.isNetworkAvailable(...) has ensured we cannot
> find any stats records by that network in the DB, why do we still want to
> call db.find(...) again? Can we directly call sendAsyncMessage(...) to
> return?

Before calling sendAsyncMessage we should format the result as an object containing the following fields: manifestURL, network, start, end, data. As NetworkStatsDB does this I thought that is better to call find and avoid to duplicate code, but if you prefer to create the object in NetworkStatsService I can do it.
(In reply to Albert [:albert] from comment #13)
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #11)
> > (In reply to Albert [:albert] from comment #9)
> > > 'find' DB function not find data for that network, but is going to fill the
> > > result object with the fields needed to create a nsIDOMMozNetworkStats when
> > > passing the result to the manager.
> > 
> > Still don't understand. If db.isNetworkAvailable(...) has ensured we cannot
> > find any stats records by that network in the DB, why do we still want to
> > call db.find(...) again? Can we directly call sendAsyncMessage(...) to
> > return?
> 
> Before calling sendAsyncMessage we should format the result as an object
> containing the following fields: manifestURL, network, start, end, data. As
> NetworkStatsDB does this I thought that is better to call find and avoid to
> duplicate code, but if you prefer to create the object in
> NetworkStatsService I can do it.

lines from https://mxr.mozilla.org/mozilla-central/source/dom/network/src/NetworkStatsDB.jsm#470 to 476 are necessary to give to result the format needed.
OK, I see. Well, then please add a comment for that. I'll point it out in the next review. :)
Attached patch PatchSplinter Review
Changes from last comments.
Attachment #8341607 - Attachment is obsolete: true
Attachment #8341607 - Flags: review?(gene.lian)
Attachment #8341766 - Flags: review?(gene.lian)
Comment on attachment 8341607 [details] [diff] [review]
Patch

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

::: dom/network/src/NetworkStatsService.jsm
@@ +316,5 @@
>        if (aResult) {
> +        toFind = true;
> +      } else if (!aError) {
> +        // Network is not found in the database without any errors.
> +        // Check if network is valid but has not established a connection yet.

Add one more comment here:

// If yes, we still want to rely on the _db.find(...) to create
// a valid nsIDOMMozNetworkStats object to return, even if we're
// sure no stats records will be found in the DB.

Also, please remember to fix comment #10 and comment #12 before landing. ;)
Attachment #8341607 - Attachment is obsolete: false
Comment on attachment 8341766 [details] [diff] [review]
Patch

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

::: dom/network/src/NetworkStatsService.jsm
@@ +323,5 @@
> +        let rilNetworks = self.getRilNetworks();
> +        if (rilNetworks[netId]) {
> +          // find will not get data for network from the database but will format the
> +          // result object in order to make NetworkStatsManager be able to construct a
> +          // nsIDOMMozNetworkStats object.

The comment is nice!
Attachment #8341766 - Flags: review?(gene.lian) → review+
Comment on attachment 8341607 [details] [diff] [review]
Patch

Oops, I was posting comments for the obsolete patch.
Attachment #8341607 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7da6412ef035
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 Sprint 6 - 12/6
You need to log in before you can comment on or make changes to this bug.