13455 2007-04-23 11:01:07 -0700 FrameView::layout() not always resuming scheduled events 2007-07-05 13:47:17 -0700 1 1 1 Unclassified WebKit Layout and Rendering 523.x (Safari 3) All All RESOLVED FIXED P2 Normal --- 1 kevino webkit-unassigned mitz paulgod oldest_to_newest 13188 0 kevino 2007-04-23 11:01:07 -0700 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. 13178 1 mitz 2007-04-23 11:51:38 -0700 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. 13069 2 14153 mitz 2007-04-24 01:37:36 -0700 Created attachment 14153 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()? 13070 3 hyatt 2007-04-24 02:01:49 -0700 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. 13075 4 mitz 2007-04-24 02:30:50 -0700 (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. 13077 5 hyatt 2007-04-24 03:07:08 -0700 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). 13078 6 hyatt 2007-04-24 03:08:29 -0700 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. 13079 7 hyatt 2007-04-24 03:09:34 -0700 (updateDashboardRegions is another possible route through which a rogue layout could be scheduled.) 13080 8 hyatt 2007-04-24 03:14:18 -0700 Basically it's not clear to me what is dangerous about any of the lines of code that execute after that early return anyway. 12994 9 kevino 2007-04-24 11:16:21 -0700 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! 12995 10 kevino 2007-04-24 11:18:19 -0700 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 9114 11 kevino 2007-05-25 10:55:49 -0700 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. :-/ 5503 12 mitz 2007-07-05 13:47:17 -0700 Fixed in <http://trac.webkit.org/projects/webkit/changeset/23866>. 14153 2007-04-24 01:37:36 -0700 2007-04-24 01:37:36 -0700 Possible fix 13455_r0.patch text/plain 2147 mitz SW5kZXg6IFdlYkNvcmUvcGFnZS9GcmFtZVZpZXcuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUv cGFnZS9GcmFtZVZpZXcuY3BwCShyZXZpc2lvbiAyMTA2MykKKysrIFdlYkNvcmUvcGFnZS9GcmFt ZVZpZXcuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00NTIsNiArNDUyLDkgQEAgdm9pZCBGcmFtZVZp ZXc6OmxheW91dChib29sIGFsbG93U3VidHJlZQogICAgIAogICAgIGlmIChyb290LT5uZWVkc0xh eW91dCgpKSB7CiAgICAgICAgIHNjaGVkdWxlUmVsYXlvdXQoKTsKKyAgICAgICAgLy8gQmFsYW5j ZSB0aGUgcGF1c2VTY2hlZHVsZWRFdmVudHMoKSBhYm92ZS4gRXZlbnRzIHdpbGwgbm90IGJlIGRp c3BhdGNoZWQgaW1tZWRpYXRlbHkKKyAgICAgICAgLy8gYmVjYXVzZSBvZiB0aGUgc2NoZWR1bGVk IGxheW91dC4KKyAgICAgICAgcmVzdW1lU2NoZWR1bGVkRXZlbnRzKCk7CiAgICAgICAgIHJldHVy bjsKICAgICB9CiAgICAgc2V0U3RhdGljQmFja2dyb3VuZCh1c2VTbG93UmVwYWludHMoKSk7CkBA IC03MTUsMTMgKzcxOCwxMSBAQCB2b2lkIEZyYW1lVmlldzo6dW5zY2hlZHVsZVJlbGF5b3V0KCkK ICAgICBpZiAoIWQtPmxheW91dFRpbWVyLmlzQWN0aXZlKCkpCiAgICAgICAgIHJldHVybjsKIAot I2lmZGVmIElOU1RSVU1FTlRfTEFZT1VUX1NDSEVEVUxJTkcKLSAgICBpZiAobV9mcmFtZS0+ZG9j dW1lbnQoKSAmJiAhbV9mcmFtZS0+ZG9jdW1lbnQoKS0+b3duZXJFbGVtZW50KCkpCi0gICAgICAg IHByaW50ZigiTGF5b3V0IHRpbWVyIHVuc2NoZWR1bGVkIGF0ICVkXG4iLCBtX2ZyYW1lLT5kb2N1 bWVudCgpLT5lbGFwc2VkVGltZSgpKTsKLSNlbmRpZgotICAgIAogICAgIGQtPmxheW91dFRpbWVy LnN0b3AoKTsKICAgICBkLT5kZWxheWVkTGF5b3V0ID0gZmFsc2U7CisKKyAgICBpZiAoIWQtPm1f ZW5xdWV1ZUV2ZW50cykKKyAgICAgICAgZGlzcGF0Y2hTY2hlZHVsZWRFdmVudHMoKTsKIH0KIAog Ym9vbCBGcmFtZVZpZXc6OmlzVHJhbnNwYXJlbnQoKSBjb25zdApAQCAtNzQ4LDcgKzc0OSw3IEBA IHZvaWQgRnJhbWVWaWV3OjpzZXRCYXNlQmFja2dyb3VuZENvbG9yKEMKIAogdm9pZCBGcmFtZVZp ZXc6OnNjaGVkdWxlRXZlbnQoUGFzc1JlZlB0cjxFdmVudD4gZXZlbnQsIFBhc3NSZWZQdHI8RXZl bnRUYXJnZXROb2RlPiBldmVudFRhcmdldCwgYm9vbCB0ZW1wRXZlbnQpCiB7Ci0gICAgaWYgKCFk LT5tX2VucXVldWVFdmVudHMpIHsKKyAgICBpZiAoIWQtPm1fZW5xdWV1ZUV2ZW50cyAmJiAhbGF5 b3V0UGVuZGluZygpKSB7CiAgICAgICAgIEV4Y2VwdGlvbkNvZGUgZWMgPSAwOwogICAgICAgICBl dmVudFRhcmdldC0+ZGlzcGF0Y2hFdmVudChldmVudCwgZWMsIHRlbXBFdmVudCk7CiAgICAgICAg IHJldHVybjsKQEAgLTc2MywxNiArNzY0LDE1IEBAIHZvaWQgRnJhbWVWaWV3OjpzY2hlZHVsZUV2 ZW50KFBhc3NSZWZQdHIKIAogdm9pZCBGcmFtZVZpZXc6OnBhdXNlU2NoZWR1bGVkRXZlbnRzKCkK IHsKLSAgICBBU1NFUlQoIWQtPm1fc2NoZWR1bGVkRXZlbnRzIHx8IGQtPm1fc2NoZWR1bGVkRXZl bnRzLmlzRW1wdHkoKSB8fCBkLT5tX2VucXVldWVFdmVudHMpOwogICAgIGQtPm1fZW5xdWV1ZUV2 ZW50cysrOwogfQogCiB2b2lkIEZyYW1lVmlldzo6cmVzdW1lU2NoZWR1bGVkRXZlbnRzKCkKIHsK ICAgICBkLT5tX2VucXVldWVFdmVudHMtLTsKLSAgICBpZiAoIWQtPm1fZW5xdWV1ZUV2ZW50cykK KyAgICBpZiAoIWQtPm1fZW5xdWV1ZUV2ZW50cyAmJiAhbGF5b3V0UGVuZGluZygpKQogICAgICAg ICBkaXNwYXRjaFNjaGVkdWxlZEV2ZW50cygpOwotICAgIEFTU0VSVCghZC0+bV9zY2hlZHVsZWRF dmVudHMgfHwgZC0+bV9zY2hlZHVsZWRFdmVudHMuaXNFbXB0eSgpIHx8IGQtPm1fZW5xdWV1ZUV2 ZW50cyk7CisgICAgQVNTRVJUKCFkLT5tX3NjaGVkdWxlZEV2ZW50cyB8fCBkLT5tX3NjaGVkdWxl ZEV2ZW50cy5pc0VtcHR5KCkgfHwgZC0+bV9lbnF1ZXVlRXZlbnRzIHx8IGxheW91dFBlbmRpbmco KSk7CiB9CiAKIHZvaWQgRnJhbWVWaWV3Ojp1cGRhdGVPdmVyZmxvd1N0YXR1cyhib29sIGhvcml6 b250YWxPdmVyZmxvdywgYm9vbCB2ZXJ0aWNhbE92ZXJmbG93KQo=