823286 - GeckoBatteryManager.java has the wrong value for kDefaultRemainingTime

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

I've been seeing this assertion-failure when starting up my B2G debug builds...
{
###!!! ASSERTION: Battery API: When charging and level at 1.0, remaining time should be 0. Please fix your backend!: 'Error', file gecko/dom/battery/BatteryManager.cpp, line 125
}
and finally decided to investigate.

In digging I found something that's definitely broken (though it may or may not be the cause of the assertion-failure) -- Constants.h has:
> 17   static const double kDefaultLevel         = 1.0;
> 18   static const bool   kDefaultCharging      = true;
> 19   static const double kDefaultRemainingTime = 0;
> 20   static const double kUnknownRemainingTime = -1;
https://mxr.mozilla.org/mozilla-central/source/dom/battery/Constants.h

whereas GeckoBatteryManager.java has:
> 24   private final static double  kDefaultLevel         = 1.0;
> 25   private final static boolean kDefaultCharging      = true;
> 26   private final static double  kDefaultRemainingTime = -1.0;
> 27   private final static double  kUnknownRemainingTime = -1.0;
https://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoBatteryManager.java

Note the different kDefaultRemainingTime values there. Looks like this has been broken ever since we added kDefaultRemainingTime, in bug 705084, about a year ago. Here are that bug's changes for those two files:

GeckoBatteryManager.java: https://hg.mozilla.org/mozilla-central/rev/aca0075d85cf#l1.12
Constants.h:  https://hg.mozilla.org/mozilla-central/rev/fc7d25349c10#l2.12

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Comment 2

[Daniel Holbert [:dholbert]]

Actually, from a quick MXR search*, it looks like this is wrong in https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoBatteryManager.java ,  too (note the "/mobile/android", distinct from "/embedding/android").

We need the same (trivial) fix there.  I don't think this problem exists elsewhere beyond that, though.

* https://mxr.mozilla.org/mozilla-central/ident?i=kDefaultRemainingTime

Comment 3

[Daniel Holbert [:dholbert]]

(looks like that latter .java file was copied from the former one, in this cset:
 https://hg.mozilla.org/mozilla-central/rev/02f6f69e8e2f )

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (now with 2 GeckoBatteryManager.java files)

Comment 5

[Mounir Lamouri (:mounir)]

Comment on attachment 694120 [details] [diff] [review]
fix v2 (now with 2 GeckoBatteryManager.java files)

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

This has actually nothing to do with the assert you saw on B2G. The B2G assertion should have been fixed with bug 758103 (landed in m-c two days ago). I doubt this is going to land in m-a or m-b.

Regarding the bug in the Android backend, nice catch. However, I think it is not as trivial as it seems. As you can see kDefaultRemainingTime is only used once, to initialize sRemainingTime. Everytime we should have been using kDefaultRemainingTime, hard-coded 0 is being used. We should stop using hard-coded 0 in those cases and use kDefaultRemainingTime instead.
Then, we should see if the initial value should actually be 0 or -1 (aka kDefaultRemainingTime or kUnknownRemainingTime). I believe it should be kDefaultRemainingTime.

In a nutshell, I think it would be nice to attach another patch to change the two |sRemainingTime = 0;| to |sRemainingTime = kDefaultRemainingTime;|

And thank you for finding, filling and fixing this bug :)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Mounir Lamouri (:mounir) from comment #5)
------------------------------------
> This has actually nothing to do with the assert you saw on B2G. The B2G
> assertion should have been fixed with bug 758103 (landed in m-c two days
> ago). I doubt this is going to land in m-a or m-b.

Ah, great!  The patch here didn't seem to fix the assert, from my local testing, so that's good to hear. :)

> In a nutshell, I think it would be nice to attach another patch to change
> the two |sRemainingTime = 0;| to |sRemainingTime = kDefaultRemainingTime;|

Cool -- patch coming up to fix those.

Note: One of the s/0/kDefaultRemainingTime/ tweaks is only needed in one .java file but not the other -- that 0 only exists in one .java file, because it was changed to 0 there (from kDefaultRemainingTime) after the file-copy noted in comment 3.  That change-to-0 was from this cset:
  https://hg.mozilla.org/mozilla-central/rev/f754a314d61d
The other file still has kDefaultRemainingTime, because it never received that patch, so it doesn't need that tweak.

> And thank you for finding, filling and fixing this bug :)

Sure -- thanks for the quick response!

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (now with s/0/kDefaultRemainingTime/)

(I also fixed a double-semicolon in some contextual code, too, because it hurt my eyes. :))

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> (looks like that latter .java file was copied from the former one, in this
> cset:
>  https://hg.mozilla.org/mozilla-central/rev/02f6f69e8e2f )

(For the record: blassey & cpeterson tell me that bug 743998 is the metabug for merging/refactoring those two "/android"  directories back together. In the meantime, my patch fixes both copies of GeckoBatteryManager.java, because we might as well have more-correct code in both spots.)

Comment 9

[Mounir Lamouri (:mounir)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> (In reply to Daniel Holbert [:dholbert] from comment #3)
> > (looks like that latter .java file was copied from the former one, in this
> > cset:
> >  https://hg.mozilla.org/mozilla-central/rev/02f6f69e8e2f )
> 
> (For the record: blassey & cpeterson tell me that bug 743998 is the metabug
> for merging/refactoring those two "/android"  directories back together. In
> the meantime, my patch fixes both copies of GeckoBatteryManager.java,
> because we might as well have more-correct code in both spots.)

Actually, I was told that anything in embedding/android/ was deprecated so I tend to not bother anymore with that.

Comment 10

[Daniel Holbert [:dholbert]]

Yeah, the first step in bug 743998 comment 0 is "remove everything in embedding/android" -- so, I'm happy to drop the fixes to that version of the file, if you'd like.

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Yeah, the first step in bug 743998 comment 0 is "remove everything in
> embedding/android" -- so, I'm happy to drop the fixes to that version of the
> file, if you'd like.

No need to. Fixing stuff in embedding/android/ doesn't hurt. I tend to not do it because I don't test it and I don't want to worry about it but it's a personal opinion. No need to follow it ;)

Comment 12

[Daniel Holbert [:dholbert]]

I think I agree with your opinion on that -- seems like it's better to just let the stale code remain unmodified, because whenever we eventually try to (merge &) delete /embedding/android, it's better to have it largely-unchanged (so it's clear that nothing needs to be saved from it) rather than changed-by-some-fixes-but-not-others.

So, I'll drop the embedding/android changes.

Try push (w/ that changed), as a sanity-check:
  https://tbpl.mozilla.org/?tree=Try&rev=59d793c37b9e

Comment 13

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eee94feb23ac

Comment 14

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/eee94feb23ac