432028 - Undetermined progressmeter timer isn't stopped, causing high CPU load

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Markus Stange [:mstange]]

Attachment: Fix v1.0

This bug is the cause for bug 429929: "CPU consumption while inactive too high"
and was introduced by me in bug 304147.

The timer has to be stopped in the following cases:

(1) The progressmeter's mode changes from "undetermined" to "normal".
(2) The progressmeter is removed from the document.

Furthermore, the timer shouldn't have any effect when

(3) the progressmeter is invisible due to display: none or
    visibility: collapse.

Cases (1) and (2) should theoretically be handled by the binding's destructor, but that's not possible because of bug 83635. However, in these cases, the binding becomes a zombie binding and all its fields become undefined which can be used to detect it.

For case (3) I've introduced an invisible <spacer> which I test for spacer.boxObject.width == 0.

The same bug has been fixed before in bug 420254.

Comment 1

[Markus Stange [:mstange]]

Requesting blocking because it blocks a (Thunderbird) blocker.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Will Thunderbird 3 be built on Gecko 1.9 or 1.9.0.x? If the latter, I'd suggest that we not block gecko1.9 on this, and move it to wanted 1.9.0.x

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 319188 [details] [diff] [review]
Fix v1.0

It just occurred to me that asking Olli for review is probably a bad idea since he can't test it (it's a Mac bug)...
Neil, could you have a look at this?

(In reply to comment #2)
> Will Thunderbird 3 be built on Gecko 1.9 or 1.9.0.x?
I don't know - David, do you know?
Firefox is also affected by this bug, but it usually only shows in the rare cases of an undetermined progressbar in the Download Manager.

Comment 4

[David Ascher (:davida)]

I don't think anyone knows what version of gecko tb3 will ship on at this point.

if this doesn't get fixed soon we could probably hack our way around the bug by removing/readding progressmeters, but that would be an ugly hack.

Comment 5

[Markus Stange [:mstange]]

(In reply to comment #4)
> if this doesn't get fixed soon we could probably hack our way around the bug by
> removing/readding progressmeters, but that would be an ugly hack.
... and moreover, it might not even work - it's case (2).

David, can you try the patch and see if it fixes the bug in Thunderbird?

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #4)
> I don't think anyone knows what version of gecko tb3 will ship on at this
> point.

Right, but not imminently, so it'll definitely be off one of the support branches of Gecko 1.9. We can block on this bug for Thunderbird 3 release, but I don't think it blocks Firefox 3.

> if this doesn't get fixed soon we could probably hack our way around the bug by
> removing/readding progressmeters, but that would be an ugly hack.

No, we should definitely fix it. :)

Comment 7

[Emre Birol]

Patched TB Trunk and did a release built. it looks good, no crash nor unexpected CPU consumption so far. Testing time 2 hours.

On Mac OS X 10.5.2

Comment 8

[Neil Deakin]

Comment on attachment 319188 [details] [diff] [review]
Fix v1.0

Why are you removing the old 'in document' check?

Please use _alive rather than mAlive

Comment 9

[Markus Stange [:mstange]]

Attachment: Fix v1.1: Use _alive

(In reply to comment #8)
> (From update of attachment 319188 [details] [diff] [review])
> Why are you removing the old 'in document' check?

Because the new check detects exactly the same cases and is simpler; at least that's what my tests have shown.

> Please use _alive rather than mAlive

Done.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 319846 [details] [diff] [review]
Fix v1.1: Use _alive

Uh, toolkit changes need sr...

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 319846 [details] [diff] [review]
Fix v1.1: Use _alive

Toolkit changes do not need sr; removing request.

Comment 12

[Mike Schroepfer]

Comment on attachment 319846 [details] [diff] [review]
Fix v1.1: Use _alive

we are closed down for showstoppers. We can get this in 3.0.1 if needed.

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 319846 [details] [diff] [review]
Fix v1.1: Use _alive

So I applied this patch to a local build, and it didn't help the CPU situation much.  After leaving it run for about 4 hours, the WindowServer processes is using about 65% of the CPU and Thunderbird is using the remaining 35%.  Quitting Thunderbird frees up both (which is about the same as it was before the patch, except that it only took a half hour to get to that point before it)

Comment 14

[Markus Stange [:mstange]]

Attachment: Alternative fix: Use 'in document' check

(In reply to comment #13)
> (From update of attachment 319846 [details] [diff] [review])
> So I applied this patch to a local build, and it didn't help the CPU situation
> much.  After leaving it run for about 4 hours, the WindowServer processes is
> using about 65% of the CPU and Thunderbird is using the remaining 35%.

That's funny. I've never seen numbers that high.
I've created a new patch that uses the same check as the non-Mac binding used before; could you test if this fixes it?

Comment 15

[Dave Miller [:justdave]]

Is that replacing the other patch or in addition to it?

Comment 16

[Markus Stange [:mstange]]

Replacing.

Comment 17

[Dave Miller [:justdave]]

OK, applied the patch and tested...  The system still gets sluggish after having TB run for a while.  WindowServer is using lots of CPU, and Thunderbird is *not* this time.  However, when I quit Thunderbird, WindowServer stops using CPU.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

I've seen the weird WindowServer behavior in a weird case while having Tb stopped in the debugger for a sufficiently long amount of time (it gets progressively worse).  I wonder if this is WindowServer polling to decide whether or not something is / should be "beachballed" because the Cocoa event loop isn't getting serviced?  That's a complete stab in the dark, though.  I rather suspect this is a separate bug.  CCing Josh in case he has any insight here... 

Comment 19

[Markus Stange [:mstange]]

(In reply to comment #18)
> I rather suspect this is a separate bug.

Probably. Does it go away when you turn off the undetermined progressbar binding completely? (see bug 429929 comment 17)

Comment 20

[Dave Miller [:justdave]]

(In reply to comment #19)
> Does it go away when you turn off the undetermined progressbar
> binding completely? (see bug 429929 comment 17)

There are 6 copies of global.css in my tree.  Which one am I editing?

Comment 21

[Markus Stange [:mstange]]

toolkit/themes/pinstripe/global/global.css

Comment 22

[Dave Miller [:justdave]]

OK, that's the one I edited and it didn't seem to help.

/me does a clobber build.

Comment 23

[Dave Miller [:justdave]]

OK, after doing a clobber, I still get undetermined progress meter bars, but they no longer animate.  However, I still get massive CPU usage by WindowServer after TB has been running for a while, which goes away when I quit TB.

I did only comment out those lines (/* */ around them) rather than deleting them, does that make a difference?

Comment 24

[Markus Stange [:mstange]]

Thanks for all your testing, Dave.

(In reply to comment #23)
> OK, after doing a clobber, I still get undetermined progress meter bars, but
> they no longer animate.

Good. That's what they looked like before bug 304147.

> However, I still get massive CPU usage by WindowServer
> after TB has been running for a while, which goes away when I quit TB.

So the WindowServer issue is not caused by this bug. Could you file a new bug for it?

> I did only comment out those lines (/* */ around them) rather than deleting
> them, does that make a difference?

It doesn't (hopefully). :-)
The progressbars have stopped animating, so it worked.

-

So it looks like the "alternative fix" has worked better than "fix v1.1". Could you test this one more time, just to verify? If that's really the case, I will request review for the new patch.

Comment 25

[Dave Miller [:justdave]]

I'm still seeing CPU from both WindowServer and Thunderbird most of the time actually, so none of the above patches really did anything for whatever I'm running into.  Thunderbird's CPU goes down ocassionally and it was probably at one of those low points when I looked making the statement in comment 17.

Comment 26

[Markus Stange [:mstange]]

OK.
This bug is about the progressmeter, so I think tracking down other CPU issues should happen in bug 429929 or even in separate bugs.

(In reply to comment #25)
> none of the above patches really did anything for whatever I'm
> running into.

Nevertheless we should take the patch, even if it doesn't solve all of Thunderbird's problems; it probably does fix bug 434513.

I'll mark the "alternative fix" as obsolete; "fix v1.1" is simpler and works for Emre (comment 7) and me.

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

Dave Miller has logged bug 436897 on the WindowServer problems.

Comment 28

[Markus Stange [:mstange]]

Could somebody check in attachment 319846 [details] [diff] [review] to hg? Thanks. :)

Comment 29

[Jon Stritar]

I think this is affecting all Forecastfox installs on FF 3 on Mac OS X. Each window you have open seems to add another 20% or so CPU usage. I used the debugger and it seems to be looping between lines 119-127 on progressmeter.xml ... the _init function in the progressmeter-periodic-redraw binding. We use an undetermined progressmeter in Forecastfox, so I think this bug is causing the problem.

Comment 30

[Markus Stange [:mstange]]

Yes. That's why bug 434513 is listed in the "Blocks:" line in the upper part of this page.

Comment 31

[Phil Ringnalda (:philor)]

Pushed in http://hg.mozilla.org/mozilla-central/index.cgi/rev/aea373f5d272a469c94e332d1d7a5aa41b75361a 

Comment 32

[Mike Shaver (:shaver emeritus)]

Comment on attachment 319846 [details] [diff] [review]
Fix v1.1: Use _alive

a=shaver for 1.9.0.x landing in service of TB3a2.

Comment 33

[Mike Shaver (:shaver emeritus)]

If this is important enough to land on the 1.9.0.x branch, it's important enough to have a test.  Please, people who were so interested in this fix, provide such a test in the next 2 weeks?

Comment 34

[Phil Ringnalda (:philor)]

toolkit/content/widgets/progressmeter.xml 1.13

Thanks, powerbook-holding laps around the world will be grateful.

Comment 35

[Jesse Ruderman]

According to http://forums.ensolis.com/viewtopic.php?f=4&t=144&st=0&sk=t&sd=a&start=10 this hurts Forecastfox users.  They're going to work around the bug in the next version of Forecastfox by removing the progress meter.  I wonder if it causes issues with other extensions as well.

Comment 36

[Jesse Ruderman]

Oh, this is already "fixed1.9.0.2", so my advocacy comment was unnecessary :)