Closed Bug 303987 Opened 15 years ago Closed 15 years ago

Resetting fonts causes Camino to hang

Categories

(Camino Graveyard :: Preferences, defect, major)

PowerPC
macOS
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alqahira, Assigned: sfraser_bugs)

References

()

Details

(Keywords: fixed1.8, hang)

Attachments

(2 files)

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+
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.
(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.
this is probably the same "hang" we see when turning on and off the animated
images pref. does firefox do the same thing?
(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.
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).
Attachment #193324 - Flags: superreview?(dbaron)
Attachment #193324 - Flags: review?(roc)
> 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.
(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.
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 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.
Attachment #193324 - Flags: superreview?(dbaron)
Attachment #193324 - Flags: superreview+
Attachment #193324 - Flags: review?(roc)
Attachment #193324 - Flags: review+
Checked into trunk. Do we want this on the branch?
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #193324 - Flags: approval1.8b4?
Summary: Resetting fonts casuses Camino to hang → Resetting fonts causes Camino to hang
please give this a day or two on the trunk before landing on the branch just to
catch any potential fallout. 
Flags: blocking1.8b4+
Attachment #193324 - Flags: approval1.8b4? → approval1.8b4+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: pinkerton → sfraser_bugs
Status: REOPENED → NEW
Checked in on the branch.
Status: NEW → RESOLVED
Closed: 15 years ago15 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Duplicate of this bug: 183899
You need to log in before you can comment on or make changes to this bug.