Last Comment Bug 758193 - Places maintenance causing excessive battery usage
: Places maintenance causing excessive battery usage
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 14 Branch
: ARM Android
: -- normal (vote)
: Firefox 16
Assigned To: Gian-Carlo Pascutto [:gcp]
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-24 06:30 PDT by Matthieu Pupat
Modified: 2016-07-29 14:25 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
verified
fixed
betaN+
15+


Attachments
Log from when Firefox has high battery usage (102.74 KB, text/plain)
2012-05-25 02:21 PDT, Matthieu Pupat
no flags Details
about:firefox screenshot (275.21 KB, image/png)
2012-05-25 05:46 PDT, Matthieu Pupat
no flags Details
Patch 1. Prevent places javascript from being bundled (1.07 KB, patch)
2012-06-01 08:09 PDT, Gian-Carlo Pascutto [:gcp]
mark.finkle: review-
Details | Diff | Splinter Review
Patch 1. v2 Prevent places Javascript from being bundled (4.30 KB, patch)
2012-06-04 08:01 PDT, Gian-Carlo Pascutto [:gcp]
mark.finkle: review+
mak77: review-
Details | Diff | Splinter Review
Patch 1. v3 Prevent places javascript from being bundled (3.70 KB, patch)
2012-06-06 03:09 PDT, Gian-Carlo Pascutto [:gcp]
mak77: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Matthieu Pupat 2012-05-24 06:30:23 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

Browse to www.google.com and leave my phone alone


Actual results:

When I came back Firefox Beta 14 was using 130+M of memory anf 70+% of CPU

Settings > Battery indicates that Firefox is responsible for 21% of CPU usage (2nd position)


Expected results:

Firefox should not use has much CPU and battery
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-24 07:03:47 PDT
Did you setup Firefox Sync ?
Any other webpages loaded?
How long were you "away" from Firefox?
Comment 2 Matthieu Pupat 2012-05-24 08:30:39 PDT
Sync is setup.
That was the only loaded page at this time.
I was away for maybe 15 minutes or so. I did not use the phone during that time.
Comment 3 Kevin Brosnan [:kbrosnan] 2012-05-24 10:53:57 PDT
I don't see this with a Galaxy SII. Sync was not enabled. Memory usage was around 50 MB and CPU usage was always below 5%.
Comment 4 Matthieu Pupat 2012-05-24 11:37:56 PDT
I do not know if this is relevant but I have Galaxy S II with Ice Cream Sandwich. I have seen this battery and CPU usage problem with Firefox 10.0 as well on Ice Cream Sandwich but never seen it on Gingerbread with Firefox 10. I did not try Firefox 14 on Gingerbread.

It does not do it all the time though. I do not know if there is a special pattern that triggers it. When it does not happen memory is abut 30MB and CPU usage is 1 or 2 %.

Is there some logfiles or special things I could do to give you more information? I really like FF14 but this battery thing is really annoying.
Comment 5 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-24 12:38:21 PDT
If you are able to reproduce it, getting the logcat output from when the CPU usage is high would be useful. See https://wiki.mozilla.org/Mobile/Fennec/Android#Using_logcat for how to get logcat output - there's an app you can use if you don't have the Android SDK installed.

Even better would be if we could reproduce it so that we could attach gdb and see exactly what it's doing, but hopefully the logcat will shed some light on it.
Comment 6 Matthieu Pupat 2012-05-25 02:20:53 PDT
This morning my battery went under 50% in about 2 hours. Reported battery usage was Firefox 73%, screen 16%. Attached is a log of when Firefox had high CPU usage.
Comment 7 Matthieu Pupat 2012-05-25 02:21:43 PDT
Created attachment 627144 [details]
Log from when Firefox has high battery usage
Comment 8 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-25 05:09:08 PDT
CCing gcp, looks like infinite loop of migration errors?
Comment 9 Gian-Carlo Pascutto [:gcp] 2012-05-25 05:11:27 PDT
There's nothing from migration in this log.
Comment 10 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-25 05:24:22 PDT
Oh, sorry. Do you know what all those PlacesDBUtils errors are, or why they would be in the log?
Comment 11 Gian-Carlo Pascutto [:gcp] 2012-05-25 05:28:33 PDT
If I would I'd have taken the bug :)

The question is what is trying to use Places. We don't have Places in Native Fennec, it's compiled out.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-25 05:32:02 PDT
E/GeckoConsole( 1919): [JavaScript Error: "PlacesUtils.bookmarks is undefined" {file: "resource://gre/modules/PlacesDBUtils.jsm" line: 985}]
E/GeckoConsole( 1919): [JavaScript Error: "DBConn is not defined" {file: "resource://gre/modules/PlacesDBUtils.jsm" line: 247}]


This might seem like a silly question, but what version does "about:firefox" say you are running? We should not be using "PlacesDBUtils.jsm" in the latest version of Firefox.
Comment 13 Matthieu Pupat 2012-05-25 05:46:16 PDT
User agent is

Mozilla/5.0 (Amdroid; Mobile; rv:14.0) Gecko/14.0 Firefox/14.0

Screenshot of about:home attached

I got it from Google Play > Firefox Beta: http://play.google.com/store/apps/details?id=org.mozilla.firefox_beta
Comment 14 Matthieu Pupat 2012-05-25 05:46:49 PDT
Created attachment 627199 [details]
about:firefox screenshot
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-25 06:02:54 PDT
Looks like the JS errors are related to Telemetry:
http://mxr.mozilla.org/mozilla-aurora/source/toolkit/components/places/PlacesDBUtils.jsm#985
Comment 16 Gian-Carlo Pascutto [:gcp] 2012-05-25 06:21:24 PDT
The other is in checkIntegrity, is called from "maintenanceOnIdle" and " checkAndFixDatabase".

Perhaps it's called from here:
http://mxr.mozilla.org/mozilla-aurora/source/toolkit/components/places/PlacesCategoriesStarter.js#112

So this looks to me like we could inadvertently have left in some idle maintenance task, which fails because Places isn't really there?

The question for me is why those places JavaScript files are in there at all. Shouldn't we just axe them?
Comment 17 Gian-Carlo Pascutto [:gcp] 2012-06-01 08:09:34 PDT
Created attachment 629192 [details] [diff] [review]
Patch 1. Prevent places javascript from being bundled

Just axe the remaining places parts. This should prevent that idle-maintenance from being called and going bonkers because half of Places is missing anyway.
Comment 18 Alex Keybl [:akeybl] 2012-06-01 14:27:44 PDT
blocking-fennec1.0 was already set to soft, so minusing for tracking. With Firefox 15, I expect we'll move back to tracking Fennec issues using the tracking flags.
Comment 19 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-02 21:08:08 PDT
Comment on attachment 629192 [details] [diff] [review]
Patch 1. Prevent places javascript from being bundled

I noticed that we also bundle some of these files in package manifest:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in#373

We should avoid that too
Comment 20 Gian-Carlo Pascutto [:gcp] 2012-06-04 08:01:12 PDT
Created attachment 629779 [details] [diff] [review]
Patch 1. v2 Prevent places Javascript from being bundled

This axes some more.
Comment 21 Matthieu Pupat 2012-06-05 05:54:22 PDT
Not sure if this is related but Firefox Beta also has a lot of Background Data Usage
Comment 22 Gian-Carlo Pascutto [:gcp] 2012-06-05 05:55:36 PDT
Completely unrelated.
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2012-06-05 05:57:02 PDT
Comment on attachment 629779 [details] [diff] [review]
Patch 1. v2 Prevent places Javascript from being bundled

This looks OK to me. We need to get Marco to review too.
Comment 24 Marco Bonardo [::mak] 2012-06-05 06:49:59 PDT
I suggested time ago to undef MOZ_PLACES to really stop compiling places... now we are adding a further ifdef around everything... sigh.
I'll review shortly, but please file a bug to throw away MOZ_ANDROID_HISTORY and properly use ifdef MOZ_PLACES (that requires doing properly the history implementation instead of the current internal module redirect of History.h).
Comment 25 Gian-Carlo Pascutto [:gcp] 2012-06-05 07:13:25 PDT
Filed bug 761588 for fixing building with MOZ_PLACES=
Comment 26 Marco Bonardo [::mak] 2012-06-05 13:55:07 PDT
Comment on attachment 629779 [details] [diff] [review]
Patch 1. v2 Prevent places Javascript from being bundled

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

Need some answer, on the browser part mostly, and some missing removals.

::: browser/components/places/src/Makefile.in
@@ +18,5 @@
>  
>  EXTRA_JS_MODULES = \
>    PlacesUIUtils.jsm \
>    $(NULL)
> +endif

I don't think you use (and probably neither build) any part of browser/, why do you need to exclude these?
Though if you really need to, cause some part of Android browser is shared with the xul browser, you should rather ifdef the places folder in browser/components/Makefile.in

::: mobile/android/installer/package-manifest.in
@@ +372,2 @@
>  @BINPATH@/components/nsLivemarkService.js
>  @BINPATH@/components/nsTaggingService.js

you want to remove these 2 as well.

::: toolkit/components/places/Makefile.in
@@ +101,2 @@
>  
>  TEST_DIRS += tests

you likely want to also exclude tests dir, since you are excluding the manifest they wouldn't work regardless. Not that I think Android is doing anything with these but makes more sense probably.
(Ideally there's much more here you don't need, but that's what the MOZ_PLACES bug should fix)

FWIW, your issue here was PlacesCategoriesStarter.js that is registered in the idle-daily category. Strange that mobile fires idle that often, I was not expecting it to fire at all.
Comment 27 Gian-Carlo Pascutto [:gcp] 2012-06-06 03:09:25 PDT
Created attachment 630489 [details] [diff] [review]
Patch 1. v3 Prevent places javascript from being bundled

Seems like you're right about the browser/ part, that wasn't needed.
Comment 28 Marco Bonardo [::mak] 2012-06-06 09:05:59 PDT
Comment on attachment 630489 [details] [diff] [review]
Patch 1. v3 Prevent places javascript from being bundled

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

Looks good, thanks for filing the other bug!
Comment 29 (dormant account) 2012-06-07 12:06:01 PDT
(In reply to Matthieu Pupat from comment #21)
> Not sure if this is related but Firefox Beta also has a lot of Background
> Data Usage

Actually seems very related, see bug 762590
Comment 30 Gian-Carlo Pascutto [:gcp] 2012-06-07 12:09:27 PDT
I've created bug 762620 to deal with the idle-daily thing that seems to misfire. Getting rid of the unwanted database stuff can stay in this bug.
Comment 31 Gian-Carlo Pascutto [:gcp] 2012-06-10 23:53:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cbd2009a5cd
Comment 32 Graeme McCutcheon [:graememcc] 2012-06-12 03:07:15 PDT
https://hg.mozilla.org/mozilla-central/rev/6cbd2009a5cd
Comment 33 Alex Keybl [:akeybl] 2012-06-14 11:46:52 PDT
Comment on attachment 630489 [details] [diff] [review]
Patch 1. v3 Prevent places javascript from being bundled

[Triage Comment]
Please land on mozilla-aurora, mozilla-beta tip, and mozilla-beta MOBILE140_2012061216_RELBRANCH.
Comment 35 Gian-Carlo Pascutto [:gcp] 2012-06-14 12:42:33 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/18b27d7bcf7a
Comment 36 Cristian Nicolae (:xti) 2012-06-15 08:25:13 PDT
I cannot reproduce this issue on the latest Beta 7 build. The power consumption by Fennec is decent now (about 7% in  15 minutes). Closing bug as verified fixed on:

Device: Galaxy Nexus
OS: Android 4.0.2

Note You need to log in before you can comment on or make changes to this bug.