Bug 140441

Summary: [CSSRegions] Assert failure in RenderBlock::locateFlowThreadContainingBlock when showing the render tree debug info
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: Layout and RenderingAssignee: 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:
Description Flags
patch none

Mihnea Ovidenie
Reported 2015-01-14 07:06:13 PST
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.
Attachments
patch (3.16 KB, patch)
2015-01-14 07:16 PST, Mihnea Ovidenie 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Mihnea Ovidenie
Comment 1 2015-01-14 07:16:06 PST
Created attachment 244601 [details] patch
Andrei Bucur
Comment 2 2015-01-14 07:21:08 PST
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.
Mihnea Ovidenie
Comment 3 2015-01-14 07:33:04 PST
(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.
Andrei Bucur
Comment 4 2015-01-14 08:06:35 PST
Comment on attachment 244601 [details] patch Ok, sounds good then, r=me
WebKit Commit Bot
Comment 5 2015-01-15 01:04:40 PST
Comment on attachment 244601 [details] patch Clearing flags on attachment: 244601 Committed r178496: <http://trac.webkit.org/changeset/178496>
WebKit Commit Bot
Comment 6 2015-01-15 01:04:56 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.