758193 - Places maintenance causing excessive battery usage

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Matthieu Pupat]

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?)]

Did you setup Firefox Sync ?
Any other webpages loaded?
How long were you "away" from Firefox?

Comment 2

[Matthieu Pupat]

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 [Ex-Mozilla]]

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]

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

[Kartikaya Gupta (email:[e-mail])]

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]

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]

Attachment: Log from when Firefox has high battery usage

Comment 8

[Kartikaya Gupta (email:[e-mail])]

CCing gcp, looks like infinite loop of migration errors?

Comment 9

[Gian-Carlo Pascutto [:gcp]]

There's nothing from migration in this log.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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]]

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?)]

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]

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]

Attachment: about:firefox screenshot

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

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]]

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]]

Attachment: 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]]

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?)]

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]]

Attachment: Patch 1. v2 Prevent places Javascript from being bundled

This axes some more.

Comment 21

[Matthieu Pupat]

Not sure if this is related but Firefox Beta also has a lot of Background Data Usage

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Completely unrelated.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

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]]

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]]

Filed bug 761588 for fixing building with MOZ_PLACES=

Comment 26

[Marco Bonardo [:mak]]

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]]

Attachment: 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]]

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)]

(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]]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cbd2009a5cd

Comment 32

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/6cbd2009a5cd

Comment 33

[Alex Keybl [:akeybl]]

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 34

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/releases/mozilla-beta/rev/81d428506e52
https://hg.mozilla.org/releases/mozilla-beta/rev/0391eb4449c0

Comment 35

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/18b27d7bcf7a

Comment 36

[Cristian Nicolae (:xti)]

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