140441 2015-01-14 07:06:13 -0800 [CSSRegions] Assert failure in RenderBlock::locateFlowThreadContainingBlock when showing the render tree debug info 2015-01-15 01:04:56 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mihnea webkit-unassigned abucur commit-queue esprehn+autocc glenn hyatt kondapallykalyan WebkitBugTracker oldest_to_newest 1061165 0 mihnea 2015-01-14 07:06:13 -0800 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. 1061166 1 244601 mihnea 2015-01-14 07:16:06 -0800 Created attachment 244601 patch 1061168 2 244601 abucur 2015-01-14 07:21:08 -0800 Comment on attachment 244601 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. 1061169 3 mihnea 2015-01-14 07:33:04 -0800 (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. 1061174 4 244601 abucur 2015-01-14 08:06:35 -0800 Comment on attachment 244601 patch Ok, sounds good then, r=me 1061508 5 244601 commit-queue 2015-01-15 01:04:40 -0800 Comment on attachment 244601 patch Clearing flags on attachment: 244601 Committed r178496: <http://trac.webkit.org/changeset/178496> 1061509 6 commit-queue 2015-01-15 01:04:56 -0800 All reviewed patches have been landed. Closing bug. 244601 2015-01-14 07:16:06 -0800 2015-01-15 01:04:40 -0800 patch 140441.patch text/plain 3236 mihnea ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAzOWE0ODBjLi5hZTAwMDczIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjEg QEAKKzIwMTUtMDEtMTQgIE1paG5lYSBPdmlkZW5pZSAgPG1paG5lYUBhZG9iZS5jb20+CisKKyAg ICAgICAgW0NTU1JlZ2lvbnNdIEFzc2VydCBmYWlsdXJlIGluIFJlbmRlckJsb2NrOjpsb2NhdGVG bG93VGhyZWFkQ29udGFpbmluZ0Jsb2NrIHdoZW4gc2hvd2luZyB0aGUgcmVuZGVyIHRyZWUgZGVi dWcgaW5mbworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTQwNDQxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg V2hlbiBzaG93aW5nIHRoZSByZW5kZXIgdHJlZSBkZWJ1ZyBpbmZvcm1hdGlvbiBmb3IgYW4gZWxl bWVudCBpbnNpZGUgYSBmbG93IHRocmVhZCwKKyAgICAgICAgd2Ugd2lsbCBkaXNwbGF5IHRoZSBy ZWdpb24gcmFuZ2UgaW5mb3JtYXRpb24gZm9yIGFsbCB0aGUgcmVuZGVyIGJveGVzLiBUbyBhdm9p ZAorICAgICAgICBjb21wdXRhdGlvbiBvZiBmbG93IHRocmVhZCBjb250YWluaW5nIGJsb2NrIGlu IHRoZXNlIHNpdHVhdGlvbnMsIHdlIHdpbGwgdXNlCisgICAgICAgIG9ubHkgdGhlIGNhY2hlZCBm bG93IHRocmVhZCBjb250YWluaW5nIGJsb2NrIGluZm9ybWF0aW9uLgorCisgICAgICAgIE5vIG5l dyB0ZXN0cyBhcyB0aGlzIGNvZGUgcGF0aCBpcyBvbmx5IHRvdWNoZWQgd2hlbiB1c2luZyBzaG93 UmVuZGVyVHJlZSogbWV0aG9kcy4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJPYmplY3Qu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6Zmxvd1RocmVhZENvbnRhaW5pbmdCbG9ja0Zyb21SZW5k ZXJlcik6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJPYmplY3Q6OnNob3dSZWdpb25zSW5mb3Jt YXRpb24pOgorCiAyMDE1LTAxLTEzICBBbmRyZWFzIEtsaW5nICA8YWtsaW5nQGFwcGxlLmNvbT4K IAogICAgICAgICBFbGVtZW50Ojpub3JtYWxpemVBdHRyaWJ1dGVzKCkgbmVlZHMgdG8gaGFuZGxl IGFyYml0cmFyeSBKUyBleGVjdXRpbmcgYmV0d2VlbiBsb29wIGl0ZXJhdGlvbnMuCmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmNwcCBiL1NvdXJjZS9X ZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJPYmplY3QuY3BwCmluZGV4IDM2NDg2YmIuLmNjZDY4NDMg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJPYmplY3QuY3BwCisr KyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJPYmplY3QuY3BwCkBAIC0xMzgzLDE2 ICsxMzgzLDQyIEBAIHZvaWQgUmVuZGVyT2JqZWN0OjpzaG93TGluZVRyZWVGb3JUaGlzKCkgY29u c3QKICAgICBkb3duY2FzdDxSZW5kZXJCbG9ja0Zsb3c+KCp0aGlzKS5zaG93TGluZVRyZWVBbmRN YXJrKG51bGxwdHIsIDIpOwogfQogCitzdGF0aWMgY29uc3QgUmVuZGVyRmxvd1RocmVhZCogZmxv d1RocmVhZENvbnRhaW5pbmdCbG9ja0Zyb21SZW5kZXJlcihjb25zdCBSZW5kZXJPYmplY3QqIHJl bmRlcmVyKQoreworICAgIGlmICghcmVuZGVyZXIpCisgICAgICAgIHJldHVybiBudWxscHRyOwor CisgICAgaWYgKHJlbmRlcmVyLT5mbG93VGhyZWFkU3RhdGUoKSA9PSBSZW5kZXJPYmplY3Q6Ok5v dEluc2lkZUZsb3dUaHJlYWQpCisgICAgICAgIHJldHVybiBudWxscHRyOworCisgICAgaWYgKGlz PFJlbmRlckZsb3dUaHJlYWQ+KCpyZW5kZXJlcikpCisgICAgICAgIHJldHVybiBkb3duY2FzdDxS ZW5kZXJGbG93VGhyZWFkPihyZW5kZXJlcik7CisKKyAgICBpZiAoaXM8UmVuZGVyQmxvY2s+KCpy ZW5kZXJlcikpCisgICAgICAgIHJldHVybiBkb3duY2FzdDxSZW5kZXJCbG9jaz4oKnJlbmRlcmVy KS5jYWNoZWRGbG93VGhyZWFkQ29udGFpbmluZ0Jsb2NrKCk7CisKKyAgICByZXR1cm4gbnVsbHB0 cjsKK30KKwogdm9pZCBSZW5kZXJPYmplY3Q6OnNob3dSZWdpb25zSW5mb3JtYXRpb24oKSBjb25z dAogewotICAgIGlmIChSZW5kZXJGbG93VGhyZWFkKiBmbG93VGhyZWFkID0gZmxvd1RocmVhZENv bnRhaW5pbmdCbG9jaygpKSB7Ci0gICAgICAgIGlmIChpczxSZW5kZXJCb3g+KCp0aGlzKSkgewot ICAgICAgICAgICAgUmVuZGVyUmVnaW9uKiBzdGFydFJlZ2lvbiA9IG51bGxwdHI7Ci0gICAgICAg ICAgICBSZW5kZXJSZWdpb24qIGVuZFJlZ2lvbiA9IG51bGxwdHI7Ci0gICAgICAgICAgICBmbG93 VGhyZWFkLT5nZXRSZWdpb25SYW5nZUZvckJveChkb3duY2FzdDxSZW5kZXJCb3g+KHRoaXMpLCBz dGFydFJlZ2lvbiwgZW5kUmVnaW9uKTsKLSAgICAgICAgICAgIGZwcmludGYoc3RkZXJyLCAiIFtS czolcCBSZTolcF0iLCBzdGFydFJlZ2lvbiwgZW5kUmVnaW9uKTsKLSAgICAgICAgfQorICAgIGNv bnN0IFJlbmRlckZsb3dUaHJlYWQqIGZ0Y2IgPSBmbG93VGhyZWFkQ29udGFpbmluZ0Jsb2NrRnJv bVJlbmRlcmVyKHRoaXMpOworCisgICAgaWYgKCFmdGNiKSB7CisgICAgICAgIC8vIE9ubHkgdGhl IGJveGVzIGhhdmUgcmVnaW9uIHJhbmdlIGluZm9ybWF0aW9uLgorICAgICAgICAvLyBUcnkgdG8g Z2V0IHRoZSBmbG93IHRocmVhZCBjb250YWluaW5nIGJsb2NrIGluZm9ybWF0aW9uCisgICAgICAg IC8vIGZyb20gdGhlIGNvbnRhaW5pbmcgYmxvY2sgb2YgdGhpcyBib3guCisgICAgICAgIGlmIChp czxSZW5kZXJCb3g+KCp0aGlzKSkKKyAgICAgICAgICAgIGZ0Y2IgPSBmbG93VGhyZWFkQ29udGFp bmluZ0Jsb2NrRnJvbVJlbmRlcmVyKGNvbnRhaW5pbmdCbG9jaygpKTsKICAgICB9CisKKyAgICBp ZiAoIWZ0Y2IpCisgICAgICAgIHJldHVybjsKKworICAgIFJlbmRlclJlZ2lvbiogc3RhcnRSZWdp b24gPSBudWxscHRyOworICAgIFJlbmRlclJlZ2lvbiogZW5kUmVnaW9uID0gbnVsbHB0cjsKKyAg ICBmdGNiLT5nZXRSZWdpb25SYW5nZUZvckJveChkb3duY2FzdDxSZW5kZXJCb3g+KHRoaXMpLCBz dGFydFJlZ2lvbiwgZW5kUmVnaW9uKTsKKyAgICBmcHJpbnRmKHN0ZGVyciwgIiBbUnM6JXAgUmU6 JXBdIiwgc3RhcnRSZWdpb24sIGVuZFJlZ2lvbik7CiB9CiAKIHZvaWQgUmVuZGVyT2JqZWN0Ojpz aG93UmVuZGVyT2JqZWN0KGJvb2wgbWFyaywgaW50IGRlcHRoKSBjb25zdAo=