| Summary: | ASSERTION FAILED: canHaveChildren() || canHaveGeneratedChildren() in WebCore::RenderElement::insertChildInternal | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Renata Hodovan <rhodovan.u-szeged> | ||||||||||||
| Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | commit-queue, darin, esprehn+autocc, glenn, kling, koivisto, kondapallykalyan, WebkitBugTracker, zalan | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 116980 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Renata Hodovan
2013-10-25 04:07:29 PDT
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. |