Bug 171643 - Should never hit layout while updating the render tree.
Summary: Should never hit layout while updating the render tree.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-03 20:17 PDT by zalan
Modified: 2017-05-04 23:48 PDT (History)
5 users (show)

See Also:

Attachments
Patch (1.55 KB, patch)
2017-05-03 20:24 PDT, zalan 		no flags Details | Formatted Diff | Diff
Patch (1.58 KB, patch)
2017-05-03 21:18 PDT, zalan 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 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.
Comment 1 zalan 2017-05-03 20:24:31 PDT 
Created attachment 309007 [details]
Patch
Comment 2 Brent Fulgham 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?
Comment 3 Simon Fraser (smfr) 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.
Comment 4 zalan 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.
Comment 5 zalan 2017-05-03 21:18:16 PDT 
Created attachment 309009 [details]
Patch
Comment 6 zalan 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.
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2017-05-04 11:27:14 PDT 
All reviewed patches have been landed.  Closing bug.