303987 - Resetting fonts causes Camino to hang

Status: RESOLVED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Spinning this off from bug 281770; when you reset fonts, it resets all of them
(bug 281770) but also can cause Camino to hang, particularly if there are many
tabs open (but can also with only one tab).  URL is zipped sample of Camino hung
after hitting reset with several tabs open (from bug 281770).

(In reply to bug 281770 comment #5)
> It looks like it re-renders the page, then beachballs
> while doing who knows what. 

This is what's really bad.  The other day I made a fresh profile to test
something, changed only two fonts (Western Serif and Sans-Serif), did my
testing, came back and hit "reset".  There was one tab open, Gmail, and it
re-rendered quickly.  Clicking the close widget on the prefs completely hung
Camino.  Sample coming, FWIW.  Camino 0.9a2+, original sample prob. from 0.8+

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Zipped sample of Camino hung after closing prefs

Comment 2

[Simon Fraser [no longer active]]

Sample shows that we're just repainting a whole bunch.

I wonder if there's a pref callback on each type of font, and when we reset the
prefs, the callbacks fire 20-odd times?

There are probably bugs already filed on poor performance after changing font prefs.

Comment 3

[Simon Fraser [no longer active]]

(In reply to comment #2)
> Sample shows that we're just repainting a whole bunch.
> 
> I wonder if there's a pref callback on each type of font, and when we reset the
> prefs, the callbacks fire 20-odd times?

Yeah, pres context registers a callback on "font." It should probably respond to
font changes on a timer, or coalesce updates with a plevent or something.

Comment 4

[Mike Pinkerton (not reading bugmail)]

this is probably the same "hang" we see when turning on and off the animated
images pref. does firefox do the same thing?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #3)
> (In reply to comment #2)
> > Sample shows that we're just repainting a whole bunch.
> > 
> > I wonder if there's a pref callback on each type of font, and when we reset
> > the prefs, the callbacks fire 20-odd times?
> 
> Yeah, pres context registers a callback on "font." It should probably respond
> to font changes on a timer, or coalesce updates with a plevent or something.

Yes! nsPresContext::PreferenceChanged should post a PLevent to do the real work
and only allow one such outstanding event, so multiple changes are coalesced.
Patch welcome :-) (The delay is probably from the synchronous reflow that fires
on each font setting change, not paints per se.)

> this is probably the same "hang" we see when turning on and off the animated
> images pref. does firefox do the same thing?

But that's only one pref so you shouldn't see PreferenceChanged firing multiple
times, so the above fix won't help there. An image.animation change should be
handled differently, so it just forces a repaint and not a reflow.

Comment 6

[Simon Fraser [no longer active]]

Attachment: Patch: coalesce pres context prefchanged callbacks with a timer

This patch coalesces multiple pref changed callbacks in nsPresContext by using
a zero-delay, one-shot timer. I thought this was more reliable than a PLEvent,
because we don't have to worry about which event queue to post the plevent to,
and it's easier to just cancel any outstanding timer on destruction (I used a
function callback to avoid ref cycles).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> because we don't have to worry about which event queue to post the plevent to,

Instead the timer code randomly picks one for us when the timer fires...  So you
end up with a timer, then the timer posting a PLEvent to some queue, then that
PLEvent firing, then your stuff happening.

Comment 8

[Simon Fraser [no longer active]]

(In reply to comment #7)
> > because we don't have to worry about which event queue to post the plevent to,
> 
> Instead the timer code randomly picks one for us when the timer fires...  So you
> end up with a timer, then the timer posting a PLEvent to some queue, then that
> PLEvent firing, then your stuff happening.

Well, yeah. But if we use a plevent, we have to be able to cancel that event in
the dtor (in case we're destroyed before it fires), so we have to go grovelling
for an event queue a second time to call RevokeEvents(). Also, I don't see a way
to cancel a specific event.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The presshell already revokes events of all sorts in its destructor, so if this
is the real worry, we can just post the even through the presshell.  Doesn't
matter either way in the end, I guess.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 193324 [details] [diff] [review]
Patch: coalesce pres context prefchanged callbacks with a timer

This is OK, but really, it would be nice to have some mechanism for posting
events that can be handled by any queue on this thread and support easy
cancellation.

Comment 11

[Simon Fraser [no longer active]]

Checked into trunk. Do we want this on the branch?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we probably do.

Comment 13

[Asa Dotzler [:asa]]

please give this a day or two on the trunk before landing on the branch just to
catch any potential fallout. 

Comment 14

[Simon Fraser [no longer active]]

Checked in on the branch.