Closed Bug 827198 Opened 11 years ago Closed 11 years ago

[Cost Control] Wifi usage is not logged.

Categories

(Firefox OS Graveyard :: General, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed)

VERIFIED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: wachen, Assigned: albert)

References

Details

(Keywords: regression, Whiteboard: [triage:1/16])

Attachments

(1 file, 3 obsolete files)

2013-1-6 b2g18(v1.0) build from pvt server

STR:
  0. Make sure your wifi is open
  1. get the notification bar and press on the usage bar

expected result: 
  See usage of wifi

actual result:
  the usage is shown for wifi is 0KB
blocking-basecamp: --- → ?
(In reply to Walter Chen from comment #0)
> 2013-1-6 b2g18(v1.0) build from pvt server
> 
> STR:
>   0. Make sure your wifi is open
>   1. get the notification bar and press on the usage bar
> 
> expected result: 
>   See usage of wifi
> 
> actual result:
>   the usage is shown for wifi is 0KB

Do you mean in the notification area thing?  It seems like the Cost Control app correctly splits out wifi vs. mobile data.
Flags: needinfo?(wachen)
It does splits out wifi vs mobile data.
But, please try to turn on the wifi and try to see if the usage accumulates.

Mine always showing 0B here.
Flags: needinfo?(wachen)
Component: Builds → Gaia::Cost Control
QA Contact: carlos.martinez
https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/
LADP user/passwd

2013-1-7 b2g18(v1.0) nightly build
(In reply to Walter Chen from comment #2)
> It does splits out wifi vs mobile data.
> But, please try to turn on the wifi and try to see if the usage accumulates.
> 
> Mine always showing 0B here.

The cost control app doesn't work for me:

http://www.pastebin.mozilla.org/2043397 (I'll file a bug)

but for jst, wifi usage does increase with usage.
Assignee: nobody → salva
blocking-basecamp: ? → +
Keywords: regression
Priority: -- → P2
Target Milestone: --- → B2G C4 (2jan on)
Stealing ;)
Assignee: salva → francisco.jordano
Asking for a quick testing from QA.

I've tried to reproduce this but I'm unabled. Also tried to get an old version of the application (22 days old) and update it to the current one.

Works perfectly after Bug 825581 landed yesterday. I can see how both the 3g and wifi counters increase as I browse.

Can someone reconfirm this?

Thanks
Keywords: qawanted
I´m testing with today´s nightly build Gecko-e39daf3 Gaia-e24d066 and is not working. I don´t know if this patch has landed after...
I´ve retested with today´s nightly (Gecko-29a48d1 Gaia-7c348f4) and is not working neither.
Stealing ;)
Assignee: francisco.jordano → salva
Retested again with today´s nightly (Gecko-644132c Gaia-f8e15c1) with the same result, no wifi consumption is logged.
Carlos, Could you attach a screenshot?
I think that the widget being displayed in notifications only display 3G data usage,
so that if you only use Wifi, it will stay on 0.0B.
It seems to be related with how BE stores data usage and dates.
Renominating.
What is the product/component for Gecko back-end?
Assignee: salva → acperez
blocking-basecamp: + → ?
Component: Gaia::Cost Control → General
QA Contact: carlos.martinez
WFM.  Please verify?
Now I roughly know about how to reproduce this.
Still reproducible for 2013-1-11 build from pvt public b2g18 nightly

**For the following test, please flash the phone before you start to test.

Please confirm that whoever see the Wifi usage works for them was trying to reproduce "Homescreen" > "Cost Control" app or did open cost control app from home screen after you flashed the phone. (Lucas Adamski, Francisco Jordano, Andrew Overholt)

Also, if anyone can confirm that Wifi usage doesn't work for them if they pull down the notification bar and click on 3G usage to enter cost control app and see the Wifi usage that way, I would be appreciate. (Alexandre Poirot, Carlos Martínez Toral)

Conclusion for the above: Whoever opened cost control app directly from home screen, the Wifi usage would be working. Whoever opened cost control app from notification bar, the Wifi usage would not be working.

If this bug is confirmed, next time I should probably add flash your phone before testing and/or follow the STR.
Flags: needinfo?(ladamski)
QA Contact: wachen
Flags: needinfo?(poirot.alex)
Flags: needinfo?(overholt)
Flags: needinfo?(francisco.jordano)
Flags: needinfo?(carlos.martinez)
(In reply to Walter Chen from comment #16)
> Now I roughly know about how to reproduce this.
> Still reproducible for 2013-1-11 build from pvt public b2g18 nightly
> 
> **For the following test, please flash the phone before you start to test.
> 
> Please confirm that whoever see the Wifi usage works for them was trying to
> reproduce "Homescreen" > "Cost Control" app or did open cost control app
> from home screen after you flashed the phone. (Lucas Adamski, Francisco
> Jordano, Andrew Overholt)
> 
> Also, if anyone can confirm that Wifi usage doesn't work for them if they
> pull down the notification bar and click on 3G usage to enter cost control
> app and see the Wifi usage that way, I would be appreciate. (Alexandre
> Poirot, Carlos Martínez Toral)
> 
> Conclusion for the above: Whoever opened cost control app directly from home
> screen, the Wifi usage would be working. Whoever opened cost control app
> from notification bar, the Wifi usage would not be working.
> 
> If this bug is confirmed, next time I should probably add flash your phone
> before testing and/or follow the STR.

I´m having this problem when opening cost control from notification bar, tapping in the data usage module. I haven´t tried the app from homescreen yet.
Flags: needinfo?(carlos.martinez)
This bug can be reproduced when timezone is set to africa / abidjan and the database of the NetworkStats API is empty.
On the 2013-01-10 build (updated via OTA) I cannot reproduce this problem from the notification bar.  My unagi is in the CET timezone.

We can't block basecamp without a reliable STR but we can fix this for v1.
blocking-b2g: --- → tef+
blocking-basecamp: ? → -
Flags: needinfo?(overholt)
:Albert, I'm unable to reproduce following those STP :(
Flags: needinfo?(poirot.alex)
Flags: needinfo?(ladamski)
Flags: needinfo?(francisco.jordano)
Francisco, did you see my comment before deleting all the people's needin??????????????????????????????????????????????????????????????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
?????????????????????????????????????????????????????????
That's for them to redo by my suggestion so that they can confirm that too...
Flags: needinfo?(francisco.jordano)
Hi Walter,

sorry, just commented with Albert, and he knows where the problem could be, that's why he assigned the bug to himself and it's working on it.

Thanks.
Flags: needinfo?(francisco.jordano)
I can reproduce this after resetting my phone data.
Blocks: 829545
The problem is caused by missing management of time / timezone changes in the NetworkStats API.

It can be reproduced changing time or timezone.

In some builds (mine for example) after flashing the timezone is set to GMT+1, so first sample before the user has completed the FTU is stored in GMT+1. When the FTU is completed, if the user has changed the timezone, will be a gap in the DB that will return no values for wifi / 3G.
Attachment #701546 - Flags: review?(philipp)
The behavior is now changed.
However, it shows "unavailable" after setting up everything.
It might be a patch somewhere?
pvt unagi b2g18 build 13-Jan-2013 08:06

Also, when you first time try to access the usage, there would be something like "a new sim inserted" on notification bar. Which is really strange to me.
blocking-basecamp: - → ?
This is already tef+ - which means it does block 1.0. We aren't using basecamp? anymore as of EOD Friday.
blocking-basecamp: ? → ---
Comment on attachment 701546 [details] [diff] [review]
Patch to avoid errors when time or timezone is changed

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +73,5 @@
>  
>    saveStats: function saveStats(stats, aResultCb) {
> +    // Convert to UTC, filter timestamp to get SAMPLE_RATE precission and
> +    // move time to midday to avoid GMT problems
> +    let timestamp = stats.date.getTime() + stats.date.getTimezoneOffset() * 60 * 1000;

If `stats.date` is in local time, to get a UTC timestamp, don't we need to *subtract* the timezone offset? The original code does that, as does your code in `findAll()` below.

@@ +122,5 @@
>  
>      // Get difference between last and new sample.
>      let diff = (newSample.timestamp - lastSample.timestamp) / SAMPLE_RATE;
> +    if(diff % 1) {
> +      // diff is decimal, so some error happened because samples are stored as a multiple

Nit: space between `if` and `(`.

@@ +124,5 @@
>      let diff = (newSample.timestamp - lastSample.timestamp) / SAMPLE_RATE;
> +    if(diff % 1) {
> +      // diff is decimal, so some error happened because samples are stored as a multiple
> +      // of SAMPLE_RATE
> +      throw Components.Exception("Error processing samples", Cr.NS_ERROR_FAILURE);

Since the exception you're throwing here isn't API-facing, it can be a normal

  throw new Error(...);

Also, you may want to txn.abort() first.

@@ +213,5 @@
> +    let request = store.openCursor(range).onsuccess = function(event) {
> +      var cursor = event.target.result;
> +      if (cursor){
> +        cursor.delete();
> +        cursor.continue();

So if I understand this correctly, the intent here is to delete any samples that have "future" timestamps after the system timezone changed. But if we're storing UTC timestamps in the database, why would this matter? Or are you worried about changes to the device's system time?

Also, nit: space between ) and {

@@ +216,5 @@
> +        cursor.delete();
> +        cursor.continue();
> +        return;
> +      }
> +      this._saveStats(txn, store, newSample);

Ideally we would delete any erroneous samples (I'm still not convinced those should ever arise, but I'll hear your answer on this) and save a new one in the same transaction. We can leave this for a follow-up bug if this one is very urgent. Just please make sure that it gets filed and fixed. Thanks!
Attachment #701546 - Flags: review?(philipp) → feedback+
(In reply to Philipp von Weitershausen [:philikon] from comment #30)
> Comment on attachment 701546 [details] [diff] [review]
> Patch to avoid errors when time or timezone is changed
> 
> Review of attachment 701546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/NetworkStatsDB.jsm
> @@ +73,5 @@
> >  
> >    saveStats: function saveStats(stats, aResultCb) {
> > +    // Convert to UTC, filter timestamp to get SAMPLE_RATE precission and
> > +    // move time to midday to avoid GMT problems
> > +    let timestamp = stats.date.getTime() + stats.date.getTimezoneOffset() * 60 * 1000;
> 
> If `stats.date` is in local time, to get a UTC timestamp, don't we need to
> *subtract* the timezone offset? The original code does that, as does your
> code in `findAll()` below.

The offset, here 'stats.date.getTimezoneOffset()' return the value with sign, so substract is not needed. For example, for GMT +1 it returns -60.

> @@ +213,5 @@
> > +    let request = store.openCursor(range).onsuccess = function(event) {
> > +      var cursor = event.target.result;
> > +      if (cursor){
> > +        cursor.delete();
> > +        cursor.continue();
> 
> So if I understand this correctly, the intent here is to delete any samples
> that have "future" timestamps after the system timezone changed. But if
> we're storing UTC timestamps in the database, why would this matter? Or are
> you worried about changes to the device's system time?

Both, changes to device's system time and also the trick of adding the offset will
cause 'future' samples when timezone is changed to a minor one.

> @@ +216,5 @@
> > +        cursor.delete();
> > +        cursor.continue();
> > +        return;
> > +      }
> > +      this._saveStats(txn, store, newSample);
> 
> Ideally we would delete any erroneous samples (I'm still not convinced those
> should ever arise, but I'll hear your answer on this) and save a new one in
> the same transaction. We can leave this for a follow-up bug if this one is
> very urgent. Just please make sure that it gets filed and fixed. Thanks!

Although samples are deleted, the new sample will keep the sent / received amount of data. But is true that history is lost. To avoid losing history I can update the timestamp of each sample stored in the database, but it seems to be a heavy process.
Attachment #701546 - Attachment is obsolete: true
Attachment #702109 - Flags: review?(philipp)
(In reply to Albert from comment #31)
> > >    saveStats: function saveStats(stats, aResultCb) {
> > > +    // Convert to UTC, filter timestamp to get SAMPLE_RATE precission and
> > > +    // move time to midday to avoid GMT problems
> > > +    let timestamp = stats.date.getTime() + stats.date.getTimezoneOffset() * 60 * 1000;
> > 
> > If `stats.date` is in local time, to get a UTC timestamp, don't we need to
> > *subtract* the timezone offset? The original code does that, as does your
> > code in `findAll()` below.
> 
> The offset, here 'stats.date.getTimezoneOffset()' return the value with
> sign, so substract is not needed. For example, for GMT +1 it returns -60.

You have a point there. So then why are we subtracting the offset in `findAll()`? Also, we assume that both `start` and `end` have the same offset in `findAll()`... it's unlikely that they don't, but it would be cleaner to use their own offsets -- maybe we should introduce a helper to convert a Date object to a UTC timestamp!

> > > +    let request = store.openCursor(range).onsuccess = function(event) {
> > > +      var cursor = event.target.result;
> > > +      if (cursor){
> > > +        cursor.delete();
> > > +        cursor.continue();
> > 
> > So if I understand this correctly, the intent here is to delete any samples
> > that have "future" timestamps after the system timezone changed. But if
> > we're storing UTC timestamps in the database, why would this matter? Or are
> > you worried about changes to the device's system time?
> 
> Both, changes to device's system time

I understand this, although I'm not sure we need to worry too much about the case when the system time is changed back in time.

> and also the trick of adding the
> offset will cause 'future' samples when timezone is changed to a minor one.

That does not make sense to me. `diff` is computed from values that should be timezone-normalized, IOW, UTC. So it should always be 0 if all that was changed was the timezone.

> > > +        cursor.delete();
> > > +        cursor.continue();
> > > +        return;
> > > +      }
> > > +      this._saveStats(txn, store, newSample);
> > 
> > Ideally we would delete any erroneous samples (I'm still not convinced those
> > should ever arise, but I'll hear your answer on this) and save a new one in
> > the same transaction. We can leave this for a follow-up bug if this one is
> > very urgent. Just please make sure that it gets filed and fixed. Thanks!
> 
> Although samples are deleted, the new sample will keep the sent / received
> amount of data. But is true that history is lost. To avoid losing history I
> can update the timestamp of each sample stored in the database, but it seems
> to be a heavy process.

I never suggested that. What I meant was to carry out the deletes and the adding of a new sample atomically -- in one single transaction. That way, if there's a problem, the entire thing gets rolled back and we don't get inconsistent data (or in this case, data loss).
Whiteboard: [triage:1/16]
Attachment #702109 - Attachment is obsolete: true
Attachment #702109 - Flags: review?(philipp)
Attachment #703262 - Flags: review?(philipp)
> > > > +    let request = store.openCursor(range).onsuccess = function(event) {
> > > > +      var cursor = event.target.result;
> > > > +      if (cursor){
> > > > +        cursor.delete();
> > > > +        cursor.continue();
> > > 
> > > So if I understand this correctly, the intent here is to delete any samples
> > > that have "future" timestamps after the system timezone changed. But if
> > > we're storing UTC timestamps in the database, why would this matter? Or are
> > > you worried about changes to the device's system time?
> > 
> > Both, changes to device's system time
> 
> I understand this, although I'm not sure we need to worry too much about the
> case when the system time is changed back in time.
> 

Why not? The user can change the time and the date as he wants.

> > > > +        cursor.delete();
> > > > +        cursor.continue();
> > > > +        return;
> > > > +      }
> > > > +      this._saveStats(txn, store, newSample);
> > > 
> > > Ideally we would delete any erroneous samples (I'm still not convinced those
> > > should ever arise, but I'll hear your answer on this) and save a new one in
> > > the same transaction. We can leave this for a follow-up bug if this one is
> > > very urgent. Just please make sure that it gets filed and fixed. Thanks!
> > 
> > Although samples are deleted, the new sample will keep the sent / received
> > amount of data. But is true that history is lost. To avoid losing history I
> > can update the timestamp of each sample stored in the database, but it seems
> > to be a heavy process.
> 
> I never suggested that. What I meant was to carry out the deletes and the
> adding of a new sample atomically -- in one single transaction. That way, if
> there's a problem, the entire thing gets rolled back and we don't get
> inconsistent data (or in this case, data loss).

If I add a new sample it will overwrite the existing one:

objectStore = db.createObjectStore(STORE_NAME, { keyPath: ["connectionType", "timestamp"] });

Then, when the system triggers the next stats update there will be a sample with the current timestamp and will cause an update instead of a new sample, resulting in a wrong datausage values.

Deleting samples will solve change of timezone (in the worst case just will mean removing one sample) and also changes of date / time that the user can do.
Attachment #703262 - Flags: review?(philipp) → review+
After talking with Albert we want to propose a solution for the case the user goes to a previous date. We think the most important thing is not to loose the *total data usage* because this is the concept the user will be charged.

So we need to preserve the data usage after the new date. To do this, we could add all these values to the new date. So, staying on day T and returning to T-n we add to the T-n entry the consumption in [T-n+1, T]. Then deletes that interval. This way we preserve the information.

In the case we are going to a future date, we store 0 consumption in all days except for the last which will hold the new value for data usage.
Modified patch to ensure consitency among deleted samples. Last patch didn't acumulate rx/tx values before deleting.
Attachment #703262 - Attachment is obsolete: true
Attachment #703847 - Flags: review?(philipp)
Attachment #703847 - Flags: review?(philipp) → review+
Is this now ready to land?
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3696ad1d2e3

Thanks for the patch, Albert! One request - to make life easier for those checking in on your behalf, can you make sure that your future patches follow the guidelines at the link below? That way all the necessary checkin metadata will be present. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in
Keywords: checkin-needed
(In reply to Ryan VanderMeulen from comment #41)

> One request - to make life easier for those
> checking in on your behalf, can you make sure that your future patches
> follow the guidelines at the link below? That way all the necessary checkin
> metadata will be present. Thanks!
> https://developer.mozilla.org/en-US/docs/
> Creating_a_patch_that_can_be_checked_in

Yes, sure!
https://hg.mozilla.org/mozilla-central/rev/b3696ad1d2e3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.
Verified fix in pvt unagi engineer build 2013-02-04:
https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/nightly/mozilla-b2g18-unagi-eng/2013/02/2013-02-04-18-06-27/unagi.zip

However, there is another new related issue out here
https://bugzilla.mozilla.org/show_bug.cgi?id=838023
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: