77653 - long web pages don't paint

Status: VERIFIED WONTFIX

(Core :: XUL, defect, P1)

Description

[Chris Waterson]

After landing the paint suppression in bug 77002, long web pages fail to 
properly paint on Win32 only.

Comment 1

[Chris Waterson]

Attachment: fix to process timers on each trip through event loop, diff -wu

Comment 2

[Chris Waterson]

hyatt and I tracked this down to the fact than, on Win32 only, we starve timers 
if there are ever *any* events in the event queue. This is distinctly different 
from the other platforms, and seems wrong.

This patch processes timers on each trip through the event loop, and is pretty 
much equivalent to the implementation on Mac. (Not sure what gtk really does, 
maybe blizzard can clue us in.)

Comment 3

[Chris Waterson]

As best we can tell, this hasn't changed since Michael Lowe first implemented 
repeating timers way back in bug 22979. Still seems wrong though. rods, 
kmcclusk, you guys r='d that checkin. Whatdya think?

Comment 4

[Chris Waterson]

Attachment: patch, -u, in case you want to apply it.

Comment 5

[sean echevarria]

This might explain the reasoning behind the original event queue:
"The WM_TIMER message is a low-priority message. The GetMessage and PeekMessage 
functions post this message only when no other higher-priority messages are in 
the thread's message queue."
http://msdn.microsoft.com/library/psdk/winui/timers_1w6q.htm
http://msdn.microsoft.com/library/psdk/winui/messques_10kl.htm

Comment 6

[David Hyatt]

I tested jrgm's tests with this patch applied and didn't see any slowdown.  IT 
also made the paint suppression behave nicely.

Comment 7

[Chris Waterson]

sean: Hmm. Just took a look at mozilla/widget/timer/src/windows/nsTimer.cpp 
(which I should've done before). It looks like we'll still starve the timer 
queue, as FireTimeout() (which is called when WM_TIMER is received) will only 
process timers that have NS_PRIORITY_HIGHEST unless the event queue is empty.

I suppose hyatt may be able to hack around the Win32 problem simply by making 
his paint suppression timer be NS_PRIORITY_HIGHEST?

Regardless, there seems to be some weird disparity between per-platform 
implementation of timer handling. I believe the above patch brings the ``Mozilla 
timer semantics'' in-line with Mac. I'm not sure exactly how GTK timers work. 
blizzard?

Comment 8

[Chris Waterson]

*** Bug 77667 has been marked as a duplicate of this bug. ***

Comment 9

[Kevin McCluskey (gone)]

The original reason for not processing the timers every time through the event 
loop was to prevent paint and reflow event starvation when we encounter pages 
which have JavaScript timers which are continually re-scheduled from within the 
timer callback in addition to scheduling reflows and paints. (This is a typical 
on pages with JavaScript animation.)  

Comment 10

[Kevin McCluskey (gone)]

Michael Lowe's implemention was intended to starve the timers, so all paint and 
reflow events would be processed before another timer would fire. This was to 
ensure the following sequence:

1) timer_callback 
2) scheduling of reflow + paint events + timer events (Result of JavaScript 
manipulation of the DOM)
3) reflow + paint events are processed
4) timer_callback (scheduled by action 2)

Comment 11

[Christopher Blizzard (:blizzard)]

I do the best I can to make sure that PL events, xlib events and timers are all
interleaved properly on gtk.  Timers are actually run from the gtk main loop and
have the same priority as the X event queue so if you have 3 timers that need to
fire it looks something like:

X event
timer
timer
timer
X event

and so on

PL events, since the wake up to the gtk event loop by posting to a file
descriptor to cause select() or a friend to wake up are handled a little
differently but are handled with extra care and feeding to make sure that they
are procced in order and such that there is no queue starvation of timers, xlib
events or plevents.

I do this by putting a serial stamp onto every event that is put into every pl
event queue.  This serial stamp is the stamp of the next event that is going to
be posted to the X event queue.  As events are pulled off the X event queue the
stamp for each one of those events is compared with the top of the list of
events in all of the currently active PL event queues and if any of the events
in the PL event queues are older than the X event they are processed after the X
event is processed.  This means that if the X event loop is really busy or there
are a flood of pl events neither of them can starve each other.

Actually, as I think about it I suspect that a flood of PL events might be able
to starve the X event queue.  I should look and see if the pl event file
descriptor is added with a really low priority so that it doesn't starve the X
event queue.  OK, I checked and it's added with a lower priority than the X
queue so this should work fine.

I hope this helps explain things a little bit.

Comment 12

[David Hyatt]

waterson's patch doesn't starve paints or reflows.  You stop processing timers 
the minute a new event is in the queue, so you don't end up causing any real 
starvation of paints and reflows.  (I ran test 13 with the bouncing text, and 
everything was fine.)

Comment 13

[David Hyatt]

Ok, this patch does have some issues.  Timers can starve the ability to move to
another page in test 13.  I'm going to try just using a HIGHEST priority timer
without this patch and see if that improves things.

Comment 14

[David Hyatt]

Ok, not using this patch and bumping my timer up to a priority of highest fixes 
all my problems.  The timer on Win32 fires flawlessly on jrgm's page load tests 
once I do that.

I don't think we should take this patch, since it does clearly regress UI 
responsiveness when DHTML pages are loading, although maybe there's still a 
happy medium here.

Comment 15

[Chris Waterson]

Ok.

hyatt: reassigning to you; if you want to close as WONTFIX or INVALID, go for 
it.

Comment 16

[David Hyatt]

76495 ups the priority of the timer to HIGHEST, which makes win32 happy.

Comment 17

[John Morrison]

verified fixed (win2k 20010522nn; win32only bug).