| Summary: | [CSSRegions] Assert failure in RenderBlock::locateFlowThreadContainingBlock when showing the render tree debug info | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mihnea Ovidenie <mihnea> | ||||
| Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abucur, commit-queue, esprehn+autocc, glenn, hyatt, kondapallykalyan, WebkitBugTracker | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
Created attachment 244601 [details]
patch
Comment on attachment 244601 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=244601&action=review > Source/WebCore/rendering/RenderObject.cpp:1398 > + return downcast<RenderBlock>(*renderer).cachedFlowThreadContainingBlock(); We should also check if the cached value is dirty and return null as well because the value is unreliable. > Source/WebCore/rendering/RenderObject.cpp:1412 > + ftcb = flowThreadContainingBlockFromRenderer(containingBlock()); The information in this case may be misleading. Another option would be to display "N/A" instead of some values that may be inaccurate. (In reply to comment #2) > Comment on attachment 244601 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244601&action=review > > > Source/WebCore/rendering/RenderObject.cpp:1398 > > + return downcast<RenderBlock>(*renderer).cachedFlowThreadContainingBlock(); > > We should also check if the cached value is dirty and return null as well > because the value is unreliable. > The current implementation does not have a dirty bit. If the value is dirty, then the cached value is null, which is covered by the proposed code. > > Source/WebCore/rendering/RenderObject.cpp:1412 > > + ftcb = flowThreadContainingBlockFromRenderer(containingBlock()); > > The information in this case may be misleading. Another option would be to > display "N/A" instead of some values that may be inaccurate. In this case, i attempt to retrieve the flow thread containing block for a box from its containing block cached information. If we call showRenderTree* methods before invalidating the cached flow thread information, the displayed information is still appropriate. After invalidating the cached flow thread containing block, we will not display the regions information. Comment on attachment 244601 [details]
patch
Ok, sounds good then, r=me
Comment on attachment 244601 [details] patch Clearing flags on attachment: 244601 Committed r178496: <http://trac.webkit.org/changeset/178496> All reviewed patches have been landed. Closing bug. |
While debugging the following test file: <!DOCTYPE html> <html> <head> <style> #article { -webkit-column-count: 2; } </style> </head> <body> <div id="article"> <div id="target"> <div id="inside"></div> </div> </div> <script type="text/javascript"> document.body.offsetTop; document.getElementById("target").style.position = "absolute"; </script> </body> </html> i noticed that if i placed a breakpoint in RenderBlock::styleDidChange(..) on the line with invalidateFlowThreadContainingBlockIncludingDescendants() and i call showRenderTreeForThis() upon hitting the breakpoint, before the invalidation code is executed, i hit the assert ASSERT(rareData->m_flowThreadContainingBlock.value() == RenderBox::locateFlowThreadContainingBlock()) inside the RenderBlock::locateFlowThreadContainingBlock() for the element that changed its position from static to absolute. The reason is that showRenderTreeForThis() uses the RenderObject::showRegionsInformation(..) method which in turn calls flowThreadContainingBlock() for the renderer. However, in this situation, we did not yet invalidate the cached flow thread information and since the element changed its position, it is not contained inside the multicol flow thread anymore and the computation of flow thread containing block issues a different value, hence the assert. I will fix this assert by changing the showRegionsInformation(..) method to always use the cached flow thread information. I will leave the existing assert in RenderBlock::locateFlowThreadContainingBlock(..) in place.