Bug 179157

Summary: Assert that updateStyle and updateLayout are only called when it's safe to dispatch events
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Layout and RenderingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, koivisto, simon.fraser, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=179259
Attachments:
Description Flags
Fixes the bug
none
Adds the assertions
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Fixed builds & tests zalan: review+

Description Ryosuke Niwa 2017-11-01 23:34:10 PDT 
Since updating the style or layout can currently execute scripts,
we should assert that these functions are only called when it's safe to execute scripts.
Comment 1 Ryosuke Niwa 2017-11-01 23:34:58 PDT 
<rdar://problem/35144778>
Comment 2 Ryosuke Niwa 2017-11-01 23:54:30 PDT 
Created attachment 325691 [details]
Fixes the bug
Comment 3 Ryosuke Niwa 2017-11-01 23:55:36 PDT 
Comment on attachment 325691 [details]
Fixes the bug

Wrong bug
Comment 4 Ryosuke Niwa 2017-11-02 00:27:11 PDT 
Created attachment 325695 [details]
Adds the assertions
Comment 5 Build Bot 2017-11-02 01:35:51 PDT 
Comment on attachment 325695 [details]
Adds the assertions

Attachment 325695 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5073448

New failing tests:
fast/forms/textarea-set-defaultvalue-after-value.html
svg/custom/check-intersection-basic.svg
Comment 6 Build Bot 2017-11-02 01:35:52 PDT 
Created attachment 325699 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 Antti Koivisto 2017-11-02 01:48:10 PDT 
Comment on attachment 325695 [details]
Adds the assertions

View in context: https://bugs.webkit.org/attachment.cgi?id=325695&action=review

> Source/WebCore/svg/SVGSVGElement.cpp:344
> +static bool checkIntersectionWithoutUpdatingLayout(RefPtr<SVGElement>&& element, SVGRect& rect)
> +{
> +    return element && RenderSVGModelObject::checkIntersection(element->renderer(), rect.propertyReference());
> +}
> +    
> +static bool checkEnclosureWithoutUpdatingLayout(RefPtr<SVGElement>&& element, SVGRect& rect)
> +{
> +    return element && RenderSVGModelObject::checkEnclosure(element->renderer(), rect.propertyReference());
> +}

Why rvalue references? These functions don't appear to be moving ownership. (I see existing code in SVGSVGElement::checkIntersection uses them too.)
Comment 8 Ryosuke Niwa 2017-11-02 16:36:13 PDT 
Created attachment 325794 [details]
Fixed builds & tests
Comment 9 Build Bot 2017-11-02 16:39:03 PDT 
Attachment 325794 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/ContainerNode.cpp:152:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Ryosuke Niwa 2017-11-02 20:48:13 PDT 
Committed r224378: <https://trac.webkit.org/changeset/224378>