32580 2009-12-15 14:26:30 -0800 JS Event listeners should not call Document::updateStyleForAllDocuments() 2010-03-04 21:18:19 -0800 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 32641 1 jamesr webkit-unassigned ap commit-queue darin dglazkov eric fishd hyatt webkit.review.bot oldest_to_newest 172038 0 jamesr 2009-12-15 14:26:30 -0800 Currently whenever a JS event handler for a DOM event is fired at a document context a call to Document::updateStyleForAllDocuments() is made. This used to be necessary since many parts of the codebase would assume styles were up to date and there was no useful way to defer the style updates, but I believe this is no longer the case as all layout codepaths should be ensuring that styles are up to date. This is really bad for pages that register DOM mutation event listeners since styles are updated on every DOM mutation. 172044 1 44911 jamesr 2009-12-15 14:32:56 -0800 Created attachment 44911 Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code This patch just deletes the calls for both the JSC and V8 bindings. This does not regress any layout tests (including pixel tests) so I believe it is correct. 172046 2 webkit.review.bot 2009-12-15 14:34:00 -0800 style-queue ran check-webkit-style on attachment 44911 without any errors. 172047 3 44911 eric 2009-12-15 14:35:24 -0800 Comment on attachment 44911 Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code I would agree, this seems like a bug. We should get the original author of these lines to confirm as such though. :) 172048 4 44911 darin 2009-12-15 14:35:54 -0800 Comment on attachment 44911 Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code I see how this is bad for performance. But I suspect that removing this call is wrong for correctness. It's entirely possible that a script changes something that triggers a style change and there is no other mechanism to guarantee the style change happens. We need to replace this mechanism with a better one, but just ripping this code out is probably wrong. We'll need to construct a test case to demonstrate this. I hope I'm wrong. I'm not saying review- because I'm not sure. Hyatt might know better. 172054 5 jamesr 2009-12-15 14:53:27 -0800 Thanks for the feedback Darin. This is basically a continuation of http://trac.webkit.org/changeset/42384 (which sadly doesn't seem to have an associated bugzilla bug). I think any code that queries properties derived from styles without making sure styles are up to date is wrong and should be fixed. I'm not sure if there is any code that does that currently - if there is then the layout tests do not seem to be hitting it. 172512 6 eric 2009-12-16 13:26:43 -0800 The important quote there being "For now in order to reduce the risk of regressions, I have left in all the synchronous machinery for updating style following DOM events and JavaScript timeouts. Eventually these calls will be removed." - Dave Hyatt. I'm ready to r+ this based on the comments above and hyatt's comments in http://trac.webkit.org/changeset/42384, but I agree, it would be nice to see Hyatt's stamp on this. 172585 7 jamesr 2009-12-16 16:49:53 -0800 https://bugs.webkit.org/show_bug.cgi?id=32641 is one case where this patch could cause a regression. This patch only removes the call from the individual dispatches - i.e. between multiple event handlers on the same event, so it doesn't cause a layout test regression currently. However, the logical next thing to do is to remove the call from Node::dispatchGenericEvent which would break fast/forms/HTMLOptionElement-selected.html. 173900 8 44911 eric 2009-12-21 00:20:43 -0800 Comment on attachment 44911 Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code I don't see how this patch can be up for review with the other bug depending on this one. I'm ready to r+ this (I think part of the only way to figure this out is to land it and wathc for regressions), but an r+ is not possible until bug 32641 is fixed. Clearing review flag. 188540 9 44911 abarth 2010-02-07 18:48:14 -0800 Comment on attachment 44911 Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code Re-nominating fore review. It looks like the blocking bug has been fixed. 188544 10 darin 2010-02-07 20:07:29 -0800 Hyatt, can we do this now or not? 188545 11 jamesr 2010-02-07 20:12:13 -0800 The forms code still depends on style recalcs to update internal state, so we can't remove _all_ of the Document::updateStyleForAllDocuments() calls. I think this one is safe to remove, since the call at the end of Node::dispatchGenericEvents() will still ensure that everything is up to date before anything outside of the event dispatching code runs. 188548 12 darin 2010-02-07 20:58:48 -0800 (In reply to comment #11) > The forms code still depends on style recalcs to update internal state, so we > can't remove _all_ of the Document::updateStyleForAllDocuments() calls. Really? Doesn't the style recalc timer take care of that case? 190367 13 jamesr 2010-02-14 20:00:13 -0800 No, because recalcStyle() has side effects other than updating styles. For instance. HTMLFormControl::recalcStyle() enqueues a call to the virtual RenderObject::updateFromElement() function. Several overrides of this (RenderMenuList::updateFromElement, RenderListBox::updateFromElement) cause updates within the DOM like updating selected state, etc. What happens in most tests currently is: 1.) The parser creates some DOM structure 2.) DOMContentLoaded fires, which calls Document::updateStyleForAllDocuments(), which forces pending updates to occur. 3.) The test code runs in the body's onload handler. 4.) The style time fires. 191244 14 44911 eric 2010-02-17 15:07:36 -0800 Comment on attachment 44911 Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code Looks OK to me. Would be nice to see a comment from Hyatt, but we can always back this out if it breaks things. 196308 15 44911 abarth 2010-03-04 19:10:35 -0800 Comment on attachment 44911 Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code Ready to land? This has been sitting here for a while. 196342 16 44911 commit-queue 2010-03-04 21:18:13 -0800 Comment on attachment 44911 Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code Clearing flags on attachment: 44911 Committed r55568: <http://trac.webkit.org/changeset/55568> 196343 17 commit-queue 2010-03-04 21:18:19 -0800 All reviewed patches have been landed. Closing bug. 44911 2009-12-15 14:32:56 -0800 2010-03-04 21:18:13 -0800 Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code update-styles.diff text/plain 1745 jamesr ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg Yjk2MmNkOC4uZWI5Mzc5NiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNiBAQAorMjAwOS0xMi0xNSAgSmFtZXMgUm9iaW5z b24gIDxqYW1lc3JAZ29vZ2xlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBTdHlsZXMgZG8gbm90IGhhdmUgdG8gYmUgc3luY2hyb25vdXNseSBy ZW1hdGNoZWQgYWZ0ZXIgZXZlcnkgZXZlbnQgZGlzcGF0Y2gKKworICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzI1ODAKKworICAgICAgICAqIGJpbmRpbmdz L2pzL0pTRXZlbnRMaXN0ZW5lci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpKU0V2ZW50TGlzdGVu ZXI6OmhhbmRsZUV2ZW50KToKKyAgICAgICAgKiBiaW5kaW5ncy92OC9WOEFic3RyYWN0RXZlbnRM aXN0ZW5lci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpWOEFic3RyYWN0RXZlbnRMaXN0ZW5lcjo6 aGFuZGxlRXZlbnQpOgorCiAyMDA5LTEyLTAxICBOaWtvbGFzIFppbW1lcm1hbm4gIDxuemltbWVy bWFubkByaW0uY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFNpbW9uIEZyYXNlci4KZGlmZiAt LWdpdCBhL1dlYkNvcmUvYmluZGluZ3MvanMvSlNFdmVudExpc3RlbmVyLmNwcCBiL1dlYkNvcmUv YmluZGluZ3MvanMvSlNFdmVudExpc3RlbmVyLmNwcAppbmRleCA3MzA2MGYxLi5jYzA2NjZkIDEw MDY0NAotLS0gYS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTRXZlbnRMaXN0ZW5lci5jcHAKKysrIGIv V2ViQ29yZS9iaW5kaW5ncy9qcy9KU0V2ZW50TGlzdGVuZXIuY3BwCkBAIC0xMjcsOCArMTI3LDYg QEAgdm9pZCBKU0V2ZW50TGlzdGVuZXI6OmhhbmRsZUV2ZW50KFNjcmlwdEV4ZWN1dGlvbkNvbnRl eHQqIHNjcmlwdEV4ZWN1dGlvbkNvbnRleHQKICAgICAgICAgICAgIH0KICAgICAgICAgfQogCi0g ICAgICAgIGlmIChzY3JpcHRFeGVjdXRpb25Db250ZXh0LT5pc0RvY3VtZW50KCkpCi0gICAgICAg ICAgICBEb2N1bWVudDo6dXBkYXRlU3R5bGVGb3JBbGxEb2N1bWVudHMoKTsKICAgICAgICAgZGVy ZWYoKTsKICAgICB9CiB9CmRpZmYgLS1naXQgYS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4QWJzdHJh Y3RFdmVudExpc3RlbmVyLmNwcCBiL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhBYnN0cmFjdEV2ZW50 TGlzdGVuZXIuY3BwCmluZGV4IGZhNDYyYWQuLmEzZTJiOTUgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUv YmluZGluZ3MvdjgvVjhBYnN0cmFjdEV2ZW50TGlzdGVuZXIuY3BwCisrKyBiL1dlYkNvcmUvYmlu ZGluZ3MvdjgvVjhBYnN0cmFjdEV2ZW50TGlzdGVuZXIuY3BwCkBAIC04Niw4ICs4Niw2IEBAIHZv aWQgVjhBYnN0cmFjdEV2ZW50TGlzdGVuZXI6OmhhbmRsZUV2ZW50KFNjcmlwdEV4ZWN1dGlvbkNv bnRleHQqIGNvbnRleHQsIEV2ZW50CiAgICAgdjg6OkhhbmRsZTx2ODo6VmFsdWU+IGpzRXZlbnQg PSBWOERPTVdyYXBwZXI6OmNvbnZlcnRFdmVudFRvVjhPYmplY3QoZXZlbnQpOwogCiAgICAgaW52 b2tlRXZlbnRIYW5kbGVyKGNvbnRleHQsIGV2ZW50LCBqc0V2ZW50KTsKLQotICAgIERvY3VtZW50 Ojp1cGRhdGVTdHlsZUZvckFsbERvY3VtZW50cygpOwogfQogCiB2b2lkIFY4QWJzdHJhY3RFdmVu dExpc3RlbmVyOjpkaXNwb3NlTGlzdGVuZXJPYmplY3QoKQo=