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]
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()?
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.
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).
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.
(updateDashboardRegions is another possible route through which a rogue layout could be scheduled.)
Basically it's not clear to me what is dangerous about any of the lines of code that execute after that early return anyway.
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!
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
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>.