Bug 32580 – JS Event listeners should not call Document::updateStyleForAllDocuments()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[James Robinson]

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.

[James Robinson]

Created attachment 44911 [details]
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.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44911 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 44911 [details]
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. :)

[Darin Adler]

Comment on attachment 44911 [details]
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.

[James Robinson]

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.

[Eric Seidel (no email)]

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.

[James Robinson]

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.

[Eric Seidel (no email)]

Comment on attachment 44911 [details]
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.

[Adam Barth]

Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

Re-nominating fore review.  It looks like the blocking bug has been fixed.

[Darin Adler]

Hyatt, can we do this now or not?

[James Robinson]

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.

[Darin Adler]

(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?

[James Robinson]

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.

[Eric Seidel (no email)]

Comment on attachment 44911 [details]
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.

[Adam Barth]

Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

Ready to land?  This has been sitting here for a while.

[WebKit Commit Bot]

Comment on attachment 44911 [details]
Removes the calls to Document::updateStyleForAllDocuments() in event dispatch code

Clearing flags on attachment: 44911

Committed r55568: <http://trac.webkit.org/changeset/55568>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.