Bug 161318 - The FlowThreadState should not be set for any of the SVG root descendants
Summary: The FlowThreadState should not be set for any of the SVG root descendants
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-08-29 11:08 PDT by Said Abou-Hallawa
Modified: 2016-09-03 12:37 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.65 KB, patch)
2016-08-29 16:55 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.33 MB, application/zip)
2016-08-29 17:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-yosemite (1.16 MB, application/zip)
2016-08-29 17:48 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2 (5.15 MB, application/zip)
2016-08-29 17:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (1.78 MB, application/zip)
2016-08-29 17:58 PDT, Build Bot
no flags Details
Patch (5.38 KB, patch)
2016-08-29 18:01 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (5.24 KB, patch)
2016-08-31 14:40 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (3.65 KB, patch)
2016-08-31 16:33 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (3.18 KB, patch)
2016-08-31 16:51 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (3.16 KB, patch)
2016-08-31 16:56 PDT, Said Abou-Hallawa
darin: review+
Details | Formatted Diff | Diff
test case (726 bytes, text/html)
2016-08-31 18:50 PDT, Said Abou-Hallawa
no flags Details
Expected results for the test case (726 bytes, text/html)
2016-08-31 18:52 PDT, Said Abou-Hallawa
no flags Details
Expected results for the test case (706 bytes, text/html)
2016-08-31 18:54 PDT, Said Abou-Hallawa
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2016-08-29 11:08:19 PDT
Open the following test case in WK1 or on iOS.

<div style="-webkit-columns:2;">
    <svg style="position:absolute;">
        <foreignObject>
            <div style="-webkit-columns:2;">
                <div style="position:absolute;"></div>
            </div>
        </foreignObject>
    </svg>
</div>

Result: The following assertion fires.

#0	0x0000000104a521b4 in ::WTFCrash() at /Volumes/Data/WebKit/OpenSource/Source/WTF/wtf/Assertions.cpp:323
#1	0x0000000108a3270a in WebCore::RenderFlowThread::collectLayerFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul>&, WebCore::LayoutRect const&, WebCore::LayoutRect const&)
#2	0x0000000108aa5b8c in WebCore::RenderLayer::collectFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul>&, WebCore::RenderLayer const*, WebCore::LayoutRect const&, WebCore::RenderLayer::PaginationInclusionMode, WebCore::ClipRectsType, WebCore::OverlayScrollbarSizeRelevancy, WebCore::ShouldRespectOverflowClip, WebCore::LayoutSize const&, WebCore::LayoutRect const*, WebCore::ShouldApplyRootOffsetToFragments)
#3	0x0000000108aa2065 in WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#4	0x0000000108aa17b5 in WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#5	0x0000000108aa0121 in WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#6	0x0000000108aa62b1 in WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow, 16ul>*, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#7	0x0000000108aa236c in WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#8	0x0000000108aa17b5 in WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#9	0x0000000108aa0121 in WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#10	0x0000000108aa62b1 in WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow, 16ul>*, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#11	0x0000000108aa236c in WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#12	0x0000000108aa17b5 in WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#13	0x0000000108aa0121 in WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#14	0x0000000108aa62b1 in WebCore::RenderLayer::paintList(WTF::Vector<WebCore::RenderLayer*, 0ul, WTF::CrashOnOverflow, 16ul>*, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#15	0x0000000108aa239c in WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#16	0x0000000108aa17b5 in WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#17	0x0000000108aa0121 in WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
#18	0x0000000108a9fa2f in WebCore::RenderLayer::paint(WebCore::GraphicsContext&, WebCore::LayoutRect const&, WebCore::LayoutSize const&, unsigned int, WebCore::RenderObject*, unsigned int)
#19	0x00000001078ed050 in WebCore::FrameView::paintContents(WebCore::GraphicsContext&, WebCore::IntRect const&)

This assertion happens when RenderFlowThread::invalidateRegions() was called but was not followed by running the layout and calling RenderFlowThread::validateRegions() which means the renderer is either dirty or its RenderFlowThread::invalidateRegions() was called by mistake.
Comment 1 Said Abou-Hallawa 2016-08-29 11:09:17 PDT
<rdar://problem/25099209>
Comment 2 Said Abou-Hallawa 2016-08-29 13:43:25 PDT
Here is the problematic call stack:

RenderObject::insertedIntoTree() is called for the innermost "<div style="position:absolute;"></div>" in the test case above to insert its renderer in the render tree. The following statement in RenderObject::insertedIntoTree() is the actual problem

    if (RenderFlowThread* flowThread = flowThreadContainingBlock())
        flowThread->flowThreadDescendantInserted(this);


It returns a pointer to the outermost RenderMultiColumnFlowThread which is created for the outermost "<div style="-webkit-columns:2;">". The then-statement "flowThread->flowThreadDescendantInserted(this);" causes RenderFlowThread::invalidateRegions() to be called at the end.

#0	0x0000000108b61520 in WebCore::RenderObject::insertedIntoTree() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderObject.cpp:1495
#1	0x0000000108a0b2b7 in WebCore::RenderElement::insertedIntoTree() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderElement.cpp:1033
#2	0x0000000108963d5f in WebCore::RenderBlockFlow::insertedIntoTree() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:142
#3	0x0000000108a08ee5 in WebCore::RenderElement::insertChildInternal(WebCore::RenderObject*, WebCore::RenderObject*, WebCore::RenderElement::NotifyChildrenType) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderElement.cpp:569
#4	0x0000000108a08ad3 in WebCore::RenderElement::addChild(WebCore::RenderObject*, WebCore::RenderObject*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderElement.cpp:493
#5	0x000000010892c8b0 in WebCore::RenderBlock::addChildIgnoringContinuation(WebCore::RenderObject*, WebCore::RenderObject*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp:636
#6	0x000000010892c386 in WebCore::RenderBlock::addChild(WebCore::RenderObject*, WebCore::RenderObject*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp:551
#7	0x00000001089821a6 in WebCore::RenderBlockFlow::addChild(WebCore::RenderObject*, WebCore::RenderObject*) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:3869
#8	0x00000001089e7f41 in WebCore::RenderBoxModelObject::moveChildTo(WebCore::RenderBoxModelObject*, WebCore::RenderObject*, WebCore::RenderObject*, bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBoxModelObject.cpp:2508
#9	0x00000001089e817d in WebCore::RenderBoxModelObject::moveChildrenTo(WebCore::RenderBoxModelObject*, WebCore::RenderObject*, WebCore::RenderObject*, WebCore::RenderObject*, bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBoxModelObject.cpp:2547
#10	0x000000010892cd91 in WebCore::RenderBoxModelObject::moveChildrenTo(WebCore::RenderBoxModelObject*, WebCore::RenderObject*, WebCore::RenderObject*, bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBoxModelObject.h:292
#11	0x0000000108b36782 in WebCore::RenderMultiColumnFlowThread::populate() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:153
#12	0x0000000108963c59 in WebCore::RenderBlockFlow::createMultiColumnFlowThread() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:130
#13	0x00000001089697e7 in WebCore::RenderBlockFlow::setComputedColumnCountAndWidth(int, WebCore::LayoutUnit) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:3932
#14	0x0000000108969719 in WebCore::RenderBlockFlow::computeColumnCountAndWidth() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:428
#15	0x0000000108968ead in WebCore::RenderBlockFlow::recomputeLogicalWidthAndColumnWidth() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:388
#16	0x000000010896999e in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:440
#17	0x000000010892ef79 in WebCore::RenderBlock::layout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp:1075
#18	0x000000010896db66 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:709
#19	0x000000010896b003 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:632
#20	0x0000000108969d0a in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:487
#21	0x000000010892ef79 in WebCore::RenderBlock::layout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp:1075
#22	0x0000000108ba99c1 in WebCore::RenderSVGForeignObject::layout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/svg/RenderSVGForeignObject.cpp:166
#23	0x0000000108ba5761 in WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderElement&, bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/svg/SVGRenderSupport.cpp:291
#24	0x0000000108bc519d in WebCore::RenderSVGRoot::layout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/svg/RenderSVGRoot.cpp:179
#25	0x0000000107782f4c in WebCore::RenderElement::layoutIfNeeded() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderElement.h:131
#26	0x0000000108931cd1 in WebCore::RenderBlock::layoutPositionedObject(WebCore::RenderBox&, bool, bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp:1483
#27	0x00000001089315b6 in WebCore::RenderBlock::layoutPositionedObjects(bool, bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp:1506
#28	0x000000010896a196 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlockFlow.cpp:526
#29	0x000000010892ef79 in WebCore::RenderBlock::layout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderBlock.cpp:1075
#30	0x0000000108ca0691 in WebCore::RenderView::layoutContent(WebCore::LayoutState const&) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderView.cpp:244
#31	0x0000000108ca1702 in WebCore::RenderView::layout() at /Volumes/Data/WebKit/OpenSource/Source/WebCore/rendering/RenderView.cpp:370
#32	0x00000001078de9b3 in WebCore::FrameView::layout(bool) at /Volumes/Data/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1438
Comment 3 Said Abou-Hallawa 2016-08-29 16:55:07 PDT
Created attachment 287350 [details]
Patch
Comment 4 Build Bot 2016-08-29 17:34:12 PDT
Comment on attachment 287350 [details]
Patch

Attachment 287350 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1968609

New failing tests:
imported/mozilla/svg/foreignObject-ancestor-style-change-01.svg
media/svg-as-image-with-media-blocked.html
imported/mozilla/svg/mask-transformed-01.svg
imported/mozilla/svg/svg-in-foreignObject-01.xhtml
imported/mozilla/svg/as-image/img-foreignObject-1.html
Comment 5 Build Bot 2016-08-29 17:34:16 PDT
Created attachment 287358 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-08-29 17:48:50 PDT
Comment on attachment 287350 [details]
Patch

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

New failing tests:
imported/mozilla/svg/foreignObject-ancestor-style-change-01.svg
imported/mozilla/svg/svg-in-foreignObject-01.xhtml
imported/mozilla/svg/mask-transformed-01.svg
media/svg-as-image-with-media-blocked.html
imported/mozilla/svg/as-image/img-foreignObject-1.html
Comment 7 Build Bot 2016-08-29 17:48:55 PDT
Created attachment 287360 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 zalan 2016-08-29 17:49:19 PDT
Comment on attachment 287350 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The containingBlock of an SVG <foreignObject> should be a null pointer.

Why?
Comment 9 Build Bot 2016-08-29 17:57:38 PDT
Comment on attachment 287350 [details]
Patch

Attachment 287350 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1968676

New failing tests:
imported/mozilla/svg/foreignObject-ancestor-style-change-01.svg
media/svg-as-image-with-media-blocked.html
imported/mozilla/svg/mask-transformed-01.svg
imported/mozilla/svg/svg-in-foreignObject-01.xhtml
imported/mozilla/svg/as-image/img-foreignObject-1.html
Comment 10 Build Bot 2016-08-29 17:57:44 PDT
Created attachment 287361 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5
Comment 11 Build Bot 2016-08-29 17:58:37 PDT
Comment on attachment 287350 [details]
Patch

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

New failing tests:
imported/mozilla/svg/foreignObject-ancestor-style-change-01.svg
media/svg-as-image-with-media-blocked.html
imported/mozilla/svg/mask-transformed-01.svg
imported/mozilla/svg/svg-in-foreignObject-01.xhtml
imported/mozilla/svg/as-image/img-foreignObject-1.html
Comment 12 Build Bot 2016-08-29 17:58:44 PDT
Created attachment 287362 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 13 Said Abou-Hallawa 2016-08-29 18:01:11 PDT
Created attachment 287363 [details]
Patch
Comment 14 Simon Fraser (smfr) 2016-08-30 10:45:21 PDT
Comment on attachment 287363 [details]
Patch

I don't think you meant to r+ this
Comment 15 Said Abou-Hallawa 2016-08-30 11:01:30 PDT
(In reply to comment #14)
> Comment on attachment 287363 [details]
> Patch
> 
> I don't think you meant to r+ this

Yes. You are right. I meant r?.
Comment 16 zalan 2016-08-30 11:40:38 PDT
Comment on attachment 287363 [details]
Patch

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

Not sure if the logic is correct here. Hyatt might know.

> Source/WebCore/ChangeLog:3
> +        Wrong containingBlock() calculation for a multicolumn element inside an SVG which is inside another multicolumn element

there's nothing multicolumn related in this patch (the title should reflect what the patch does)

> Source/WebCore/ChangeLog:9
> +        The containingBlock of the RenderSVGForeignObject should be the containingBlock
> +        of the containingBlock of the RenderSVGRoot. The layout of a multi-column element

too many 'containingBlock of'?

> Source/WebCore/rendering/RenderBox.cpp:3002
> +    if (!cb)
> +        return availableHeight;

only the RenderView is supposed to return nullptr for containingBlock(). -and that is already part of the while loop logic. (it should anyway return an empty optional<> to be more explicit about this error case.)

> Source/WebCore/rendering/RenderBoxModelObject.cpp:249
> +    if (!cb)
> +        return false;
> +

same as above.

> Source/WebCore/rendering/RenderObject.cpp:593
> +        const auto* root = SVGRenderSupport::findTreeRootObject(downcast<RenderElement>(*this));

const is not needed.

> Source/WebCore/rendering/RenderObject.cpp:594
> +        ASSERT(root);

this assertion does not add much value. a null check might be a better idea.
Comment 17 Simon Fraser (smfr) 2016-08-30 12:12:07 PDT
Comment on attachment 287363 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:592
> +    if (is<RenderSVGForeignObject>(*this)) {

This is a new virtual function in a hot code path, which is going to hurt performance.
Comment 18 Said Abou-Hallawa 2016-08-31 14:40:38 PDT
Created attachment 287544 [details]
Patch
Comment 19 Said Abou-Hallawa 2016-08-31 16:33:42 PDT
Created attachment 287561 [details]
Patch
Comment 20 Said Abou-Hallawa 2016-08-31 16:37:58 PDT
Comment on attachment 287363 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Wrong containingBlock() calculation for a multicolumn element inside an SVG which is inside another multicolumn element
> 
> there's nothing multicolumn related in this patch (the title should reflect what the patch does)

Title was changed.

>> Source/WebCore/ChangeLog:9
>> +        of the containingBlock of the RenderSVGRoot. The layout of a multi-column element
> 
> too many 'containingBlock of'?

Description was changed.

>> Source/WebCore/rendering/RenderBox.cpp:3002
>> +        return availableHeight;
> 
> only the RenderView is supposed to return nullptr for containingBlock(). -and that is already part of the while loop logic. (it should anyway return an empty optional<> to be more explicit about this error case.)

This part was deleted from the patch. However I still think that the condition for cb in the while (cb && ... ) should be deleted.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:249
>> +
> 
> same as above.

This part was deleted from the patch. However I still think that the condition for cb in the while (cb && ... ) should be deleted.

>> Source/WebCore/rendering/RenderObject.cpp:592
>> +    if (is<RenderSVGForeignObject>(*this)) {
> 
> This is a new virtual function in a hot code path, which is going to hurt performance.

This check was moved to setFlowThreadStateIncludingDescendants().
Comment 21 Simon Fraser (smfr) 2016-08-31 16:40:23 PDT
Comment on attachment 287561 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:172
> +    if (!is<RenderSVGForeignObject>(*this))
> +        setFlowThreadState(state);

setFlowThreadStateIncludingDescendants propagates down the tree, not up the tree. So I wonder if we should stop at the SVG root.
Comment 22 Said Abou-Hallawa 2016-08-31 16:51:48 PDT
Created attachment 287568 [details]
Patch
Comment 23 Said Abou-Hallawa 2016-08-31 16:56:16 PDT
Created attachment 287569 [details]
Patch
Comment 24 Simon Fraser (smfr) 2016-08-31 16:57:24 PDT
Comment on attachment 287569 [details]
Patch

Can we test this more directly, other than by just not crashing in an imported test?
Comment 25 Said Abou-Hallawa 2016-08-31 16:57:35 PDT
Comment on attachment 287561 [details]
Patch

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

>> Source/WebCore/rendering/RenderObject.cpp:172
>> +        setFlowThreadState(state);
> 
> setFlowThreadStateIncludingDescendants propagates down the tree, not up the tree. So I wonder if we should stop at the SVG root.

Yes I think you are right. We should set the FlowThreadState for the SVG root but not for any of its descendants.
Comment 26 Said Abou-Hallawa 2016-08-31 18:50:56 PDT
Created attachment 287581 [details]
test case

This test case fails with and without the latest patch.
Comment 27 Said Abou-Hallawa 2016-08-31 18:52:55 PDT
Created attachment 287582 [details]
Expected results for the test case

This page is the same as the test case but with replacing the <svg> and the <foreignObject> with <div> elements.
Comment 28 Said Abou-Hallawa 2016-08-31 18:54:32 PDT
Created attachment 287583 [details]
Expected results for the test case
Comment 29 zalan 2016-08-31 19:06:00 PDT
> condition for cb in the while (cb && ... ) should be deleted.
> 
> >> Source/WebCore/rendering/RenderBoxModelObject.cpp:249
> >> +
> > 
> > same as above.
> 
> This part was deleted from the patch. However I still think that the
> condition for cb in the while (cb && ... ) should be deleted.
There are several places where we travel up on the containingBlock() chain until we hit RenderView and we use the exact same logic as here. Deleting the RenderView check would also change functionality.