567339 - Excessive wake-ups after visiting start page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Marko Pellikka]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11) Gecko/2009060309 Ubuntu/8.04 (hardy) Firefox/3.0.11
Build Identifier: Fennec 2.0a1pre 20100521021132

When Fennec is started it opens about:home which requires https connection for checking available extensions. This causes Fennec to do 5-13 wake-ups/s even if moved to about:blank next. One of the threads seem to keep finding or configuring the missing https connection forever even though connection is not needed anymore after moving to about:blank. Problem seems to be the same if just https proxy is not defined or wrong.


Reproducible: Always

Steps to Reproduce:
1. Start Fennec
2. Open about:blank
3. Wait couple of minutes
4. Measure with powertop tool
Actual Results:  
Scenarios:
A: HTTPS OK -> Start Fennec (about:home) -> open about:blank -> 2.4 wake-ups/s
B: HTTPS not OK -> Start Fennec (about:home) -> open about:blank -> 12.0 wake-ups/s

Expected Results:  
Target is to reduce unnecessary wake-ups. Availability of HTTPS connection shouldn't cause neverending extra wake-ups. Eventually number of wake-ups should be the same as in scenario A 2.4 wake-ups.

Usually scenario A results in wake-up about between 1-3/s whereas scenario B results between 5-13/s.

Comment 1

[Marko Pellikka]

Scenario A (HTTPS OK)

$ powertop -d -p
Top causes for wakeups:
  18.0% (  4.2)   [      ] [ata_piix] <interrupt>
  15.1% (  3.5)   [     0] [kernel scheduler] Load balancing tick
  11.1% (  2.6)   [     0] [kernel core] hrtimer_start (tick_sched_timer)
   9.1% (  2.1)   [  1397] gnome-terminal
   8.9% (  2.1)   [      ] fennec
   8.6% (  2.0)   [     0] [kernel core] clocksource_watchdog (clocksource_watchdog)

Scenario B (HTTPS not OK) (5 minutes after going to about:blank)

$ powertop -d -p
Top causes for wakeups:
  22.2% (  6.1)   [      ] fennec
  14.1% (  3.9)   [     0] [kernel scheduler] Load balancing tick
  13.7% (  3.7)   [      ] [ata_piix] <interrupt>
   9.5% (  2.6)   [     0] [kernel core] hrtimer_start (tick_sched_timer)
   7.6% (  2.1)   [  1397] gnome-terminal
   7.3% (  2.0)   [     0] [kernel core] clocksource_watchdog (clocksource_watchdog)

Comment 2

[Alon Zakai (:azakai)]

Marko: I am investigating this issue, and can confirm it. However, it doesn't seem to be related to HTTPS, but to something else - imgContainer::Notify is called 4 times/second, which accounts for the 4 additional wakeups between the two cases shown in your last comment.

HTTPS is indeed used (downloading CRLs, etc.), but that activity appears to stop eventually.

Perhaps there are two issues here? What led you to suspect HTTPS as the problem?

Comment 3

[Marko Pellikka]

Alon: The reason I suspect HTTPS is that if I have set the HTTPS/SSL proxy correctly, I get 2-3 wakeups/s. If HTTPS proxy isn't set or internet connection is not working, I get 5-13 wakeups/s. Also on start page about:home Fennec is starting to check up over HTTPS if there are extension updates available. If browser is started with -url about:blank although proxy is not configured, we are still between 2-3 wakeups/s.

Of course it's possible that this is only a way to trigger another problem, for example calling of imgContainer::Notify function 4 times/second.

Comment 4

[Alon Zakai (:azakai)]

Marko: Thanks for the info, that is the same data as I am seeing. Then it turns out that indeed this is just triggering another problem: Depending on HTTPS working or not, a progress animation is shown, and that animation starts a timer that persists even after it is needed. I am investigating now how to resolve this.

Comment 5

[Alon Zakai (:azakai)]

I marked this as depending on bug 359608, however, I am not certain anymore. That bug report's comments imply that it's fixed by disabling bfcache (max_total_viewers), however, that does *not* fix this bug here. So there isn't a workaround for this bug.

Comment 6

[Justin Dolske [:Dolske]]

This sounds like it might just be a symptom of bug 502694? Are you sure the timers are all from the animated image, and not just everything else?

Comment 7

[Boris Zbarsky [:bzbarsky]]

We should really just stop using self-timers for animated images and switch them to refresh driver....  :(

Comment 8

[Alon Zakai (:azakai)]

(In reply to comment #6)
> This sounds like it might just be a symptom of bug 502694? Are you sure the
> timers are all from the animated image, and not just everything else?

I don't think this is the same as that bug. I am seeing imgContainer::Notify being called at the rate of the animation, and not imgContainer::sDiscardTimerCallback (which should anyhow be called only every 15 seconds).

It looks like the animation timer is only halted when the imgContainer is destroyed (it then destroys mAnim, which cancels the timer). And if I am reading the stack traces I gathered correctly, the imgContainer is only destroyed when the HTML image element is destroyed, which depends on garbage collection.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> It looks like the animation timer is only halted when the imgContainer is
> destroyed 

That's not quite true; see the StopAnimation call in imgRequest::RemoveProxy.

> when the HTML image element is destroyed, which depends on garbage collection.

This part is true, though, since the proxy is owned by the <img>.  There was talk at some point about differentiation which proxies care about animation (the one owned by <img> does not, really).

Comment 10

[Jon Hemming]

I tried this issue by just opening an animated gif or png from local file system and noticed that when you move to about:blank after that there will be the same frequency of wake-ups that is seen with animated picture. This doesn't happen if you do the same thing with a web page that has animated pictures, so there is some difference on how timers behave when comparing local and web handling.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Do we need a separate Core:Imagelib bug covering that issue?

Comment 12

[Alon Zakai (:azakai)]

(In reply to comment #10)
> I tried this issue by just opening an animated gif or png from local file
> system and noticed that when you move to about:blank after that there will be
> the same frequency of wake-ups that is seen with animated picture. This doesn't
> happen if you do the same thing with a web page that has animated pictures, so
> there is some difference on how timers behave when comparing local and web
> handling.

This is different from what I am seeing. I see the same wakeups regardless of whether the image is from the web or is local content. How are you measuring wakeups? (Over here I am measuring wakeups by printed output from the imgContainer animation callback, and also using powertop).

Comment 13

[Jon Hemming]

(In reply to comment #12)
> This is different from what I am seeing. I see the same wakeups regardless of
> whether the image is from the web or is local content. How are you measuring
> wakeups? (Over here I am measuring wakeups by printed output from the
> imgContainer animation callback, and also using powertop).

 I noticed what I reported using http://www.gifs.net. Now I tested it again and going to about:blank was working. I guess it is easiest to notice when selecting some category and viewing many animated GIFs at the same time, but it can also be noticed by viewing a preview of a single picture. I'm using powertop for measurements.
 Since you didn't get the same result, I also tested this with my own web page with only text and one picture. Going to about:blank didn't work with that one, so now I can only say that going to about:blank will stop most of the timers after viewing animated pictures in from http://www.gifs.net. Not much of a test case.

Comment 14

[Jon Hemming]

Attachment: Patch that would enable suspending the animation

These wake-up issues may have been tolerated in firefox, but with fennec they would need to be resolved. Could the first step be to allow suspending animation for all the imgContainers, by making it an observer? There is a lot of these problems with animation in meta bug 119597 and many, possible duplicates, added later not visible in that bug. With this approach it would be easy to suspend all imgContainers when wanted.

Comment 15

[Alon Zakai (:azakai)]

The different results might be related to garbage collection, btw - GC will eventually remove the unnecessary timers. So that could explain seeing somewhat different results at different times / different systems.

About the patch, I am not sure when we would want to suspend all the animations?

Comment 16

[Jon Hemming]

(In reply to comment #15)
> The different results might be related to garbage collection, btw - GC will
> eventually remove the unnecessary timers. So that could explain seeing somewhat
> different results at different times / different systems.
> 
> About the patch, I am not sure when we would want to suspend all the
> animations?

Suspending all animations are needed for example, when mobile device is idle. Especially if system goes to sleep, all this kind of unnecessary things need to be suspended. On that note there is also a general problem with nsIIdleService. It wont start counting the idle time before user has been active. This leads to situation where idle timer wont start if you only start the fennec and user wont do anything after that. This was noticed when doing patch for this that would use nsIIdleService for suspending the timers after some period of time.

Comment 17

[Alon Zakai (:azakai)]

Makes sense, I would prefer though to actually freeze the entire child process when the mobile device is idle (bug 568054) instead of writing code to manually suspend every system separately. But both approaches have advantages over the other.

As the problem with the idle service is a separate issue, maybe it makes sense to file a separate bug.

Comment 18

[Alon Zakai (:azakai)]

I posted a patch to bug 359608, which according to my tests also fixes this bug,

https://bugzilla.mozilla.org/show_bug.cgi?id=359608#c16

See details there about how it relates to the refresh driver.

Comment 19

[Alon Zakai (:azakai)]

(In reply to comment #16)
> Suspending all animations are needed for example, when mobile device is idle.
> Especially if system goes to sleep, all this kind of unnecessary things need to
> be suspended.

Regarding that, btw, it seems mobile OSes may (eventually, or already?) do something like it,

http://lwn.net/Articles/390369/

OS-level suspending of all applications (even running ones) would be better than anything we can do, probably.

Comment 20

[Alon Zakai (:azakai)]

It is starting to look like the patch for bug 359608 (which we depend on) will fix everything *but* chrome pages... and the Fennec start page is chrome. So we may need a different solution here.

Comment 21

[Oleg Romashin (:romaxa)]

Jon is this fix still needed? Alon any progress here

Comment 22

[Jon Hemming]

Comment on attachment 450633 [details] [diff] [review]
Patch that would enable suspending the animation

At least this patch is obsolete and not approach that is wanted. Alon were you planning on some fix for this? I guess based on previous comments this still needs one?

Comment 23

[Alon Zakai (:azakai)]

We definitely need a fix for this. Some thoughts, about discussion with bholley and dbaron:

1. aboutHome.xhtml, the page for this, just hides the image

  document.getElementById("loadingAddons").style.display = "none";

  Perhaps we can fix this by removing the element, not just hiding it.

2. We can in theory let hidden images be properly frozen, but we'd need
   to fast-forward them when shown again - so we need the fast-forward
   code that will be written for suspending animation in background tabs,
   bug 588975. It would not be trivial to get this to work with
   propagation though - in the sense of freezing an image in an element,
   and that element is hidden (but not directly the image).

3. We need to make sure that mIsActive is set properly in the Fennec
   browser code; Firefox's browser code does this. It is necessary for
   freezing animations in the bfcache.

4. To get this page to work as is, we would need to add code so that
   chrome stuff handles animation freezing etc. - not sure if it would
   or wouldn't (it is html, not xul - but it's chrome).

5. Hacks/workarounds: We can make the image animate once instead of
   repeatedly. If the animation is 5 seconds long, would users ever
   see that much of it anyhow?

Comment 24

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

(In reply to comment #23)
> We definitely need a fix for this. Some thoughts, about discussion with bholley
> and dbaron:
> 
> 1. aboutHome.xhtml, the page for this, just hides the image
> 
>   document.getElementById("loadingAddons").style.display = "none";
> 
>   Perhaps we can fix this by removing the element, not just hiding it.

File a bug and I'll make a patch

> 3. We need to make sure that mIsActive is set properly in the Fennec
>    browser code; Firefox's browser code does this. It is necessary for
>    freezing animations in the bfcache.

What is mIsActive? We do set nsIDocShell.isActive for content pages here:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/content.js#462 and 468

but we don't for chrome pages! File a bug?

Comment 25

[Alon Zakai (:azakai)]

Filed those two bugs, added as blockers for this bug.

Comment 26

[Alon Zakai (:azakai)]

With the resolving of those two bugs, this bug is resolved as well. The startup page no longer kills our battery.

Comment 27

[Cristian Nicolae (:xti)]

Verified fixed on:
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110922
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2