RESOLVED FIXED 171643
Should never hit layout while updating the render tree. 
https://bugs.webkit.org/show_bug.cgi?id=171643
Summary Should never hit layout while updating the render tree.
zalan
Reported 2017-05-03 20:17:39 PDT
Running layout on a half-backed tree is not the greatest idea. We'd better ASSERT on it.
Attachments
Patch (1.55 KB, patch)
2017-05-03 20:24 PDT, zalan 		no flags
DetailsFormatted DiffDiff
Patch (1.58 KB, patch)
2017-05-03 21:18 PDT, zalan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
zalan
Comment 1 2017-05-03 20:24:31 PDT
Created attachment 309007 [details] Patch
Brent Fulgham
Comment 2 2017-05-03 21:03:47 PDT
Comment on attachment 309007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309007&action=review > Source/WebCore/page/FrameView.cpp:1291 > + ASSERT(!frame().document()->inRenderTreeUpdate()); Should this be a release assert so we can't continue if this happens?
Simon Fraser (smfr)
Comment 3 2017-05-03 21:12:18 PDT
Comment on attachment 309007 [details] Patch What about asserting about layouts in render tree update, and about style recall in render tree update? Seems like we really want to have more stateful asserting.
zalan
Comment 4 2017-05-03 21:16:05 PDT
Comment on attachment 309007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309007&action=review >> Source/WebCore/page/FrameView.cpp:1291 >> + ASSERT(!frame().document()->inRenderTreeUpdate()); > > Should this be a release assert so we can't continue if this happens? That might work too (though we could actually finish the layout just fine), but it should be at least assertion with security implication.
zalan
Comment 5 2017-05-03 21:18:16 PDT
Created attachment 309009 [details] Patch
zalan
Comment 6 2017-05-04 08:03:08 PDT
(In reply to zalan from comment #4) > Comment on attachment 309007 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309007&action=review > > >> Source/WebCore/page/FrameView.cpp:1291 > >> + ASSERT(!frame().document()->inRenderTreeUpdate()); > > > > Should this be a release assert so we can't continue if this happens? > > That might work too (though we could actually finish the layout just fine), > but it should be at least assertion with security implication. Actually I think this should be part of the "why don't we turn all assert_with_security_implication into release asserts" conversation.
WebKit Commit Bot
Comment 7 2017-05-04 11:27:12 PDT
Comment on attachment 309009 [details] Patch Clearing flags on attachment: 309009 Committed r216196: <http://trac.webkit.org/changeset/216196>
WebKit Commit Bot
Comment 8 2017-05-04 11:27:14 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.