420254 - thunderbird often uses ~10% cpu when idle for no apparent reason

Status: VERIFIED FIXED

(Thunderbird :: General, defect)

Description

[Tuukka Tolvanen (sp3000)]

linux trunk, this probably started fairly recently (a week or three?)
Thunderbird often starts using some 7--20% cpu for long periods of time (minutes? forever?) when idle for no apparent reason. I suck, so oprofile and gdb just tell me "it's doing js" so far ;)

Comment 1

[Wayne Mery (:wsmwk)]

have you narrowed the regression?

version 3.0a1pre (2008022803) windows - I'm seeing sustained 5-11% usage.   This instance had been running for about 12 hours.

** I set thunderbird offline and cpu continued unchanged.
   back online, no change
   => timer?

I've restarted and after 20 minutes I am unable to recreate cpu usage after going through: AB, news, virtual, send, save draft, advanced search, SA window, ...

At 30 minutes I'm in a modestly sustained 1% cpu usage - perhaps some with cumulative effect is occuring?

Comment 2

[Tuukka Tolvanen (sp3000)]

I get a range of http://bonsai.mozilla.org/cvsquery.cgi?date=explicit&mindate=2008-02-14+12:45&maxdate=2008-02-14+13:00 -> bug 409111, which sounds surprising to me, but I'm finding it hard to argue with it after double-double-checking with local backout, even if the test may not be totally reliable...

Comment 3

[Bob Clary [:bc] (inactive)]

I did a jprof enabled debug build on linux and let it run for 1 cpu minute. The bzip2'd file is still pretty large, and I can send it to anyone who is interested. The following did seem interesting though:

index Count    Hits Function Name
               8114 NS_ProcessNextEvent_P(nsIThread*, int)
                  2 NS_ProcessPendingEvents_P(nsIThread*, unsigned int)
                  1 nsBaseAppShell::Run()
...

              4100 nsBaseAppShell::Run()
               3883 nsXULWindow::ShowModal()
                 98 nsThread::ThreadFunc(void*)
                 34 nsSocketTransportService::Run()
 18402   0     4198 NS_ProcessNextEvent_P(nsIThread*, int)
               8114 nsThread::ProcessNextEvent(int, int*)
                  1 nsCOMPtr<nsIThreadObserver>::operator->() const

Comment 4

[Tuukka Tolvanen (sp3000)]

Attachment: oprofile 1min idle snapshot without 409111

Comment 5

[Tuukka Tolvanen (sp3000)]

Attachment: oprofile 1min idle snapshot with 409111

These are the quietest minute of the first 5 to 6 for a build with bug 409111 backed out, and not backed out, respectively. The loop was, for reference, like

tag=with-409111; i=0; while true; do
    sudo opcontrol --reset; sleep 60; pgrep thunderbird || continue
    opreport --symbols ~/src/moz/obj/thunderbird-debug/dist/bin/*{,/*} \
        > $tag-$i
    i=$((i+1))
done

Comment 6

[Magnus Melin [:mkmelin]]

Smaug, any idea why bug 409111 would cause this?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Something in TB JS code uses boxObjects with elements which aren't in document?
In 1.8 and after fixing regression bug 409111, xulelement.boxObject doesn't 
throw, unlike before bug 409111. Are there some try-catch blocks in TB js
which check .boxObject validity? That way the exception doesn't show up in
error console.
One could perhaps debug this by adding some warnings to
nsDocument::GetBoxObjectFor to check if element is in document.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

The problem seems to be that some "undetermined" progressmeter has the interval running. Writing a patch. I think progressmeter needs a fix.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

This is a possible patch. Would have to ask Enn and/or Neil what they think 
about this, but if someone could first test this to ensure that this really fixes the problem.

Comment 10

[Tuukka Tolvanen (sp3000)]

no, a307547 doesn't seem to fix this issue for me.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase for progressmeter

no? At least it fixes this testcase.
Is there still some other similar problem...
Couldn't see any other problematic boxObject use cases

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better test

Shows that putting elements back to dom works.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a bit different patch for progressmeter

Comment 14

[Tuukka Tolvanen (sp3000)]

looks like a307579 helps :)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

This is a bit faster than the previous. If someone could verify that this still fixes the problem.

Comment 16

[Bob Clary [:bc] (inactive)]

I believe this fixes it for me. Not having this load on thunderbird certainly improves the user experience greatly. 

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test width, not width >> 2

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v4

Comment 19

[[e-mail]]

Comment on attachment 307760 [details] [diff] [review]
v4

>               spacer.left = width * position;
It makes no sense for this width to be zero, as then nothing will animate.

Also is there a reason to leave in the try/catch?

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

You want me to start fixing other things in this code?

I use the !width test as a hint that element is removed from document and then
check if that is the case. This way progressmeter still works if it is hidden
and then set back visible.

try/catch is possibly needed in case document's presshell is destroyed but scripts are still running.

Comment 21

[Tuukka Tolvanen (sp3000)]

ftr, a307585 and a307760 also wfm.

Comment 22

[[e-mail]]

Comment on attachment 307760 [details] [diff] [review]
v4

OK, I see your point.

>+                // Maybe we've been removed from the document.
>+                var currentNode = stack;
>+                while (currentNode && currentNode != stack.ownerDocument) {
>+                  currentNode = currentNode.parentNode;
>+                }
>+                if (!currentNode) {
>+                  clearInterval(interval);
>+                  return;
>+                }
I don't know what you think about this:
for (var currentNode = stack; currentNode != stack.ownerDocument;
                              currentNode = currentNode.parentNode) {
  if (!currentNode) {
    clearInterval(interval);
    return;
  }
}

Or maybe you could try to select the stack in a range; I understand that these days that's invalid if the stack isn't in a document :-)

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #22)
> Or maybe you could try to select the stack in a range; I understand that these
> days that's invalid if the stack isn't in a document :-)
It was invalid, but not anymore.

That for loop is just doing the same as the while, so I don't see big difference.
|while| is vertically longer, 'for' horizontally. Matter of taste.

Comment 24

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

Comment on attachment 307760 [details] [diff] [review]
v4

a1.9=beltzner

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Checked in, but please verify

Comment 26

[Tuukka Tolvanen (sp3000)]

v