Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(229)

Issue 82843003: Fire overflowchanged events at raf timing (Closed)

Created:
7 years ago by esprehn
Modified:
7 years ago
Reviewers:
tkent, ojan, inferno, eseidel
CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), zoltan1, eae+blinkwatch, Timothy Loh, leviw+renderwatch, dstockwell, dglazkov+blink, adamk+blink_chromium.org, jchaffraix+rendering, darktears, bemjb+rendering_chromium.org, Steve Block, dino_apple.com, Eric Willigers
Visibility:
Public.

Description

Fire overflowchanged events at raf timing Running script inside layout leads to nasty security bugs and crashes, instead we should defer overflowchanged events until raf time. This still lets the author take an action before the paint preventing blinking and jumpiness which was the reason we ran it inside layout, while avoiding the pitfalls of synchronous script. Unfortunately this patch makes us start firing overflowchanged even when a node has been removed from the tree, but the old protection against it was bad since it only checked inDocument() so removing a node and putting it in another document would still let the event fire. Instead I plan to fix the detach problem in a future patch since scroll events shouldn't fire for detached nodes either. This patch also lets us remove the paused-event-dispatch.html test which was testing the suspend/resume logic I removed and for crashes that happen with synchronous script inside layout which doesn't apply after this patch. BUG=293534, 323283 TEST=fast/events/overflowchanged-event-raf-timing.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162655

Patch Set 1 #

Patch Set 2 : fix comments #

Patch Set 3 : Add a test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+103 lines, -136 lines) Patch
D LayoutTests/fast/dynamic/paused-event-dispatch.html View 1 chunk +0 lines, -47 lines 0 comments Download
D LayoutTests/fast/dynamic/paused-event-dispatch-expected.txt View 1 chunk +0 lines, -5 lines 0 comments Download
D LayoutTests/fast/dynamic/resources/paused-event-dispatch-iframe.html View 1 chunk +0 lines, -2 lines 0 comments Download
A LayoutTests/fast/events/overflowchanged-event-raf-timing.html View 1 2 1 chunk +55 lines, -0 lines 0 comments Download
A LayoutTests/fast/events/overflowchanged-event-raf-timing-expected.txt View 1 2 1 chunk +9 lines, -0 lines 0 comments Download
M Source/core/dom/Document.h View 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/dom/Document.cpp View 2 chunks +10 lines, -3 lines 0 comments Download
M Source/core/dom/ScriptedAnimationController.h View 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/dom/ScriptedAnimationController.cpp View 1 3 chunks +13 lines, -6 lines 0 comments Download
M Source/core/frame/FrameView.h View 3 chunks +0 lines, -8 lines 0 comments Download
M Source/core/frame/FrameView.cpp View 11 chunks +6 lines, -47 lines 0 comments Download
M Source/core/page/EventHandler.h View 1 chunk +1 line, -2 lines 0 comments Download
M Source/core/page/EventHandler.cpp View 1 chunk +0 lines, -7 lines 0 comments Download
M Source/core/rendering/RenderBlock.cpp View 1 chunk +3 lines, -5 lines 0 comments Download
M Source/web/WebViewImpl.cpp View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 8 (0 generated)
esprehn
7 years ago (2013-11-25 22:05:10 UTC) #1
ojan
lgtm Your change description has some missing words. Would be nice to add a test. ...
7 years ago (2013-11-25 22:21:31 UTC) #2
esprehn
On 2013/11/25 22:21:31, ojan wrote: > lgtm > > Your change description has some missing ...
7 years ago (2013-11-25 22:53:30 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/esprehn@chromium.org/82843003/40001
7 years ago (2013-11-25 22:55:48 UTC) #4
eseidel
The security team probably owes you a hug. :) Fantastic!
7 years ago (2013-11-26 00:03:56 UTC) #5
commit-bot: I haz the power
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink_rel&number=13690
7 years ago (2013-11-26 00:25:39 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/esprehn@chromium.org/82843003/40001
7 years ago (2013-11-26 01:26:40 UTC) #7
commit-bot: I haz the power
7 years ago (2013-11-26 02:06:02 UTC) #8
Message was sent while issue was closed.
Change committed as 162655

Powered by Google App Engine
This is Rietveld 408576698