The following simple test crashes on trunk (r157966): <input style="-webkit-flow-from: thread"> The backtrace: Program received signal SIGSEGV, Segmentation fault. 0x00007ffff5d23529 in WTFCrash () at /home/reni/Data/REPOS/webkit_sec/Source/WTF/wtf/Assertions.cpp:342 342 *(int *)(uintptr_t)0xbbadbeef = 0; (gdb) bt #0 0x00007ffff5d23529 in WTFCrash () at /home/reni/Data/REPOS/webkit_sec/Source/WTF/wtf/Assertions.cpp:342 #1 0x00007ffff1c5efd7 in WebCore::RenderElement::insertChildInternal (this=0x11d9c40, newChild=0x1228250, beforeChild=0x0, notifyChildren=WebCore::RenderElement::NotifyChildren) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderElement.cpp:511 #2 0x00007ffff1c5ed25 in WebCore::RenderElement::addChild (this=0x11d9c40, newChild=0x1228250, beforeChild=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderElement.cpp:465 #3 0x00007ffff1bb6e4c in WebCore::RenderBlock::addChildIgnoringAnonymousColumnBlocks (this=0x11d9c40, newChild=0x1228250, beforeChild=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderBlock.cpp:827 #4 0x00007ffff1bb7012 in WebCore::RenderBlock::addChildIgnoringContinuation (this=0x11d9c40, newChild=0x1228250, beforeChild=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderBlock.cpp:850 #5 0x00007ffff1bb6f68 in WebCore::RenderBlock::addChild (this=0x11d9c40, newChild=0x1228250, beforeChild=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderBlock.cpp:842 #6 0x00007ffff1bf301a in WebCore::RenderBlockFlow::createRenderNamedFlowFragmentIfNeeded (this=0x11d9c40) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderBlockFlow.cpp:2645 #7 0x00007ffff1be5cc0 in WebCore::RenderBlockFlow::insertedIntoTree (this=0x11d9c40) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderBlockFlow.cpp:108 #8 0x00007ffff1c5f25d in WebCore::RenderElement::insertChildInternal (this=0x11d3730, newChild=0x11d9c40, beforeChild=0x0, notifyChildren=WebCore::RenderElement::NotifyChildren) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderElement.cpp:547 #9 0x00007ffff1c5ed25 in WebCore::RenderElement::addChild (this=0x11d3730, newChild=0x11d9c40, beforeChild=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderElement.cpp:465 #10 0x00007ffff1bb6e4c in WebCore::RenderBlock::addChildIgnoringAnonymousColumnBlocks (this=0x11d3730, newChild=0x11d9c40, beforeChild=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderBlock.cpp:827 #11 0x00007ffff1bb7012 in WebCore::RenderBlock::addChildIgnoringContinuation (this=0x11d3730, newChild=0x11d9c40, beforeChild=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderBlock.cpp:850 #12 0x00007ffff1bb6f68 in WebCore::RenderBlock::addChild (this=0x11d3730, newChild=0x11d9c40, beforeChild=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/rendering/RenderBlock.cpp:842 #13 0x00007ffff1e49875 in WebCore::Style::createRendererIfNeeded (element=..., resolvedStyle=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/style/StyleResolveTree.cpp:263 #14 0x00007ffff1e4a39d in WebCore::Style::attachRenderTree (current=..., resolvedStyle=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/style/StyleResolveTree.cpp:470 #15 0x00007ffff1e4b4cd in WebCore::Style::attachRenderTree (element=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/style/StyleResolveTree.cpp:824 #16 0x00007ffff169f496 in WebCore::executeTask (task=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLConstructionSite.cpp:104 #17 0x00007ffff169f7d5 in WebCore::HTMLConstructionSite::executeQueuedTasks (this=0x94c198) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLConstructionSite.cpp:150 #18 0x00007ffff16cc15a in WebCore::HTMLTreeBuilder::constructTree (this=0x94c180, token=0x7fffffffc200) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:368 #19 0x00007ffff16a7450 in WebCore::HTMLDocumentParser::constructTreeFromHTMLToken (this=0x11333b0, rawToken=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLDocumentParser.cpp:595 #20 0x00007ffff16a70bb in WebCore::HTMLDocumentParser::pumpTokenizer (this=0x11333b0, mode=WebCore::HTMLDocumentParser::AllowYield) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLDocumentParser.cpp:552 #21 0x00007ffff16a68ab in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible (this=0x11333b0, mode=WebCore::HTMLDocumentParser::AllowYield) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLDocumentParser.cpp:236 #22 0x00007ffff16a7991 in WebCore::HTMLDocumentParser::append (this=0x11333b0, inputSource=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLDocumentParser.cpp:742 #23 0x00007ffff13b1ff6 in WebCore::DecodedDataDocumentParser::flush (this=0x11333b0, writer=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/DecodedDataDocumentParser.cpp:60 #24 0x00007ffff1818a25 in WebCore::DocumentWriter::end (this=0x1163de0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/DocumentWriter.cpp:242 #25 0x00007ffff18062be in WebCore::DocumentLoader::finishedLoading (this=0x1163d40, finishTime=0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/DocumentLoader.cpp:408 #26 0x00007ffff180602c in WebCore::DocumentLoader::notifyFinished (this=0x1163d40, resource=0x1179d90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/DocumentLoader.cpp:345 #27 0x00007ffff189c526 in WebCore::CachedResource::checkNotify (this=0x1179d90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/cache/CachedResource.cpp:369 ---Type <return> to continue, or q <return> to quit--- #28 0x00007ffff189c600 in WebCore::CachedResource::finishLoading (this=0x1179d90) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/cache/CachedResource.cpp:385 #29 0x00007ffff189905a in WebCore::CachedRawResource::finishLoading (this=0x1179d90, data=0x8faa30) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/cache/CachedRawResource.cpp:94 #30 0x00007ffff1856e64 in WebCore::SubresourceLoader::didFinishLoading (this=0x117a300, finishTime=0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/SubresourceLoader.cpp:283 #31 0x00007ffff1853005 in WebCore::ResourceLoader::didFinishLoading (this=0x117a300, finishTime=0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/ResourceLoader.cpp:487 #32 0x00007ffff2528c7a in WebCore::readCallback (asyncResult=0x117f1b0, data=0x117abc0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1328 #33 0x00007fffe898abc9 in async_ready_callback_wrapper (source_object=0x670980, res=0x117f1b0, user_data=0x117abc0) at ginputstream.c:530 #34 0x00007fffe89acccb in g_task_return_now (task=0x117f1b0) at gtask.c:1105 #35 complete_in_idle_cb (task=<optimized out>) at gtask.c:1114 #36 0x00007fffee09a473 in g_main_dispatch (context=0x117ee70) at gmain.c:3054 #37 g_main_context_dispatch (context=0x117ee70) at gmain.c:3630 #38 0x00007ffff75c8aee in _ecore_glib_select__locked (ecore_timeout=0x117ee70, efds=<optimized out>, wfds=<optimized out>, rfds=<optimized out>, ecore_fds=1, ctx=<optimized out>) at ecore_glib.c:171 #39 _ecore_glib_select (ecore_fds=1, rfds=<optimized out>, wfds=<optimized out>, efds=<optimized out>, ecore_timeout=0x117ee70) at ecore_glib.c:205 #40 0x00007ffff75c2cb9 in _ecore_main_select (timeout=<optimized out>) at ecore_main.c:1466 #41 0x00007ffff75c3789 in _ecore_main_loop_iterate_internal (once_only=0) at ecore_main.c:1860 #42 0x00007ffff75c3b47 in ecore_main_loop_begin () at ecore_main.c:956 #43 0x0000000000406dfa in main (argc=2, argv=0x7fffffffde68) at /home/reni/Data/REPOS/webkit_sec/Tools/EWebLauncher/main.c:1044
Created attachment 215159 [details] Test case
This is funny. We assert on attaching the fragment because the _fragment_ can't have children. We force the fragment no-child policy too early while the fragment itself is being added. diff --git a/Source/WebCore/rendering/RenderBlockFlow.cpp b/Source/WebCore/rendering/RenderBlockFlow.cpp index 03f756c..5c62e13 100644 --- a/Source/WebCore/rendering/RenderBlockFlow.cpp +++ b/Source/WebCore/rendering/RenderBlockFlow.cpp @@ -3150,9 +3150,9 @@ void RenderBlockFlow::createRenderNamedFlowFragmentIfNeeded() // FIXME: Multicolumn regions not yet supported (http://dev.w3.org/csswg/css-regions/#multi-column-regions) if (style().isDisplayRegionType() && style().hasFlowFrom() && !style().specifiesColumns()) { RenderNamedFlowFragment* flowFragment = new RenderNamedFlowFragment(document(), RenderNamedFlowFragment::createStyle(style())); + addChild(flowFragment); flowFragment->initializeStyle(); setRenderNamedFlowFragment(flowFragment); - addChild(renderNamedFlowFragment()); } }
Created attachment 269031 [details] Patch
Comment on attachment 269031 [details] Patch Attachment 269031 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/692857 New failing tests: fast/regions/input-box-with-region-assert.html
Created attachment 269032 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 269034 [details] Patch
Comment on attachment 269034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269034&action=review > Source/WebCore/rendering/RenderBlockFlow.cpp:3204 > + RenderNamedFlowFragment* flowFragmentOnFlow = rareData.m_renderNamedFlowFragment; > + rareData.m_renderNamedFlowFragment = nullptr; > + if (flowFragmentOnFlow) > + flowFragmentOnFlow->destroy(); I like auto* in cases like this rather than naming the class; avoids the possibility of changing the class to a less-specific one if the code is refactored and types change. Nothing about the code depends on the type in any interesting way, so it’s nice to not specify the type here. I also like to use std::exchange for cases like this to make the "take and null out" operation a single one rather than repeating the expression. I’d write: if (auto* flowFragmentOnFlow = std::exchange(rareData.m_renderNamedFlowFragment, nullptr)) flowFragmentOnFlow->destroy(); This two line form seems clearer to me than the four line version.
(In reply to comment #7) > Comment on attachment 269034 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269034&action=review > > > Source/WebCore/rendering/RenderBlockFlow.cpp:3204 > > + RenderNamedFlowFragment* flowFragmentOnFlow = rareData.m_renderNamedFlowFragment; > > + rareData.m_renderNamedFlowFragment = nullptr; > > + if (flowFragmentOnFlow) > > + flowFragmentOnFlow->destroy(); > > I like auto* in cases like this rather than naming the class; avoids the > possibility of changing the class to a less-specific one if the code is > refactored and types change. Nothing about the code depends on the type in > any interesting way, so it’s nice to not specify the type here. I also like > to use std::exchange for cases like this to make the "take and null out" std::exchange is sweet! I didn't know about it! Thanks!
Created attachment 269069 [details] Patch
Comment on attachment 269069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269069&action=review > Source/WebCore/rendering/RenderBlockFlow.cpp:3203 > RenderBlockFlowRareData& rareData = ensureRareBlockFlowData(); > - if (rareData.m_renderNamedFlowFragment) > - rareData.m_renderNamedFlowFragment->destroy(); > + if (auto* flowFragmentOnFlow = std::exchange(rareData.m_renderNamedFlowFragment, nullptr)) > + flowFragmentOnFlow->destroy(); > rareData.m_renderNamedFlowFragment = flowFragment; One further thought. This is not equivalent, will call destroy with the new fragment already in m_renderNamedFlowFragment, but if it will work correctly, I think it’s better: if (auto* oldFlowFragment = std::exchange(ensureRareBlockFlowData().m_renderNamedFlowFragment, flowFragment)) oldFlowFragment->destroy();
(In reply to comment #10) > Comment on attachment 269069 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269069&action=review > > > Source/WebCore/rendering/RenderBlockFlow.cpp:3203 > > RenderBlockFlowRareData& rareData = ensureRareBlockFlowData(); > > - if (rareData.m_renderNamedFlowFragment) > > - rareData.m_renderNamedFlowFragment->destroy(); > > + if (auto* flowFragmentOnFlow = std::exchange(rareData.m_renderNamedFlowFragment, nullptr)) > > + flowFragmentOnFlow->destroy(); > > rareData.m_renderNamedFlowFragment = flowFragment; > > One further thought. This is not equivalent, will call destroy with the new > fragment already in m_renderNamedFlowFragment, but if it will work > correctly, I think it’s better: > > if (auto* oldFlowFragment = > std::exchange(ensureRareBlockFlowData().m_renderNamedFlowFragment, > flowFragment)) > oldFlowFragment->destroy(); I'd rather not have the new fragment set until after we cleaned up the old one. RenderElement::removeChildInternal would surely assert on canHaveChildren() when a new, non-null fragment is set.
Comment on attachment 269069 [details] Patch Clearing flags on attachment: 269069 Committed r195146: <http://trac.webkit.org/changeset/195146>
All reviewed patches have been landed. Closing bug.