Bug 13455 – FrameView::layout() not always resuming scheduled events

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Kevin Ollivier]

Recently, the wx port started triggering an assert in FrameView::~FrameView, caused by the fact that d->m_enqueueEvents > 0. Tracking this down, I've found the reason for this appears to be that in FrameView::layout(), while pauseSelectedEvents() is always called, resumeSelectedEvents() is only triggered if root->needsLayout() (the if statement in line #453) is false. If root->needsLayout() is true, a layout is scheduled and the function exits without resuming scheduled events.

Since I'm not too familiar with the bugs fixed by pause/resumeSelectedEvents(), I'm not sure how best to fix this problem (though Mitz said on IRC that he might :-), so I thought I'd just file a bug for this so that those familiar with the code can take a look.

Admittedly, I don't know how to trigger the early return code path, but I think the fix should be along the lines of calling resumeScheduledEvents from there but also patching resumeScheduledEvents to not fire the events when the counter reaches 0 if a layout is scheduled. When that layout happens, it will call pause/resume again and the events will be fired.

Created attachment 14153 [details]
Possible fix

This should fix the pause/resume unbalance. Kevin, I would like to know whether it fixes the bug for you or maybe you hit a different assert down the line. Also, so that I could make a layout test for the patch, can you tell me how you trigger the early return from layout()?

[Dave Hyatt]

I actually don't think it should be possible to have the root still needing layout without having something miscoded somewhere.  I think it might be better to just yank the early return and replace it with an assert that the root doesn't need layout.

(In reply to comment #3)
> I actually don't think it should be possible to have the root still needing
> layout without having something miscoded somewhere.  I think it might be better
> to just yank the early return and replace it with an assert that the root
> doesn't need layout.
> 

I think the client can dirty the layout in its didFirstLayout callback.

[Dave Hyatt]

That is completely unsupported and won't work right now even with the early return.

Many times layout is called as a result of paint.  If you still need layout after a paint, we're going to assert anyway when we try to paint (and possibly crash if didFirstLayout invalidated table sections).

[Dave Hyatt]

Also, layout scheduling has been reenabled by the time didFirstLayout gets called, so if it did cause a dirty of the root, the layout would be scheduled already anyway.

[Dave Hyatt]

(updateDashboardRegions is another possible route through which a rogue layout could be scheduled.)

[Dave Hyatt]

Basically it's not clear to me what is dangerous about any of the lines of code that execute after that early return anyway.

[Kevin Ollivier]

I added the assert, and it is being triggered very early on, even before the page gets displayed. I removed my one client call to setNeedsLayout()/scheduleRelayout() (which is used by wx when the native control's size changes), but the assert still gets triggered.

The assert is fired from a layout called by FrameView::layoutTimerFired. I put a break in the scheduleRelayout function, and I found that the first three times it is called, it does lead to the assert. (The calls are coming from Document::updateStyleSelector in response to receiving data from the ResourceManager.)

The fourth time the call comes from HTMLBodyElement::InsertIntoDocument (starting from HTMLTokenizer::timerFired), and it is from this code path that it eventually asserts. However, since I'm not too familiar with the rendering engine yet, I'm not sure what exactly I should be looking for in terms of needsLayout() being set incorrectly, or not reset properly. 

Any tips on how I can track this down further? Thanks for all your help!

[Kevin Ollivier]

Sorry, typo in my last message. First sentence in the second paragraph should say: 

and I found that the first three times it is called, it does **not** lead to the assert

[Kevin Ollivier]

BTW, I wanted to give an update on this. I've done some more testing and found that the underlying cause was that I was trying to schedule layouts during size events. I was doing this for performance reasons - we support live resize on Mac and I didn't want the repaints to cause the resize to be sluggish. In any case, I realize now that this solution wasn't the correct one, and have corrected my code so that it does not assert. 

Based on that, I think Hyatt's comment about asserting is a good idea - if it had asserted when I first added the code, I wouldn't have had to track down what change caused it. I'll try to take some time out this weekend to prepare a patch and submit it.

Thanks for all your help on this and sorry I mistook a problem with the port for a WebCore issue. :-/

Fixed in <http://trac.webkit.org/projects/webkit/changeset/23866>.