Load the following HTML in a debug build: <!doctype html> <html> <head> <style> #article { -webkit-flow-into: flow; } </style> </head> <body> <div class="flowContent" id="article"> <span> a <p>b</p> a </span> </div> </body> </html> After page is loaded, reload it and you will get an assertion with the following trace: 1 0x1085d482b WebCore::Node::detach() 2 0x10763227f WebCore::ContainerNode::detach() 3 0x107a52f00 WebCore::Element::detach() 4 0x107634b97 WebCore::ContainerNode::detachChildren() 5 0x107632288 WebCore::ContainerNode::detach() 6 0x10784b57d WebCore::Document::detach() 7 0x10784b8c3 WebCore::Document::prepareForDestruction() 8 0x107b5bce6 WebCore::Frame::setView(WTF::PassRefPtr<WebCore::FrameView>) 9 0x107b5e748 WebCore::Frame::createView(WebCore::IntSize const&, WebCore::Color const&, bool, WebCore::IntSize const&, WebCore::IntRect const&, bool, WebCore::ScrollbarMode, bool, WebCore::ScrollbarMode, bool) 10 0x105336fa7 WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage() 11 0x107b716bd WebCore::FrameLoader::transitionToCommitted(WTF::PassRefPtr<WebCore::CachedPage>) 12 0x107b70afc WebCore::FrameLoader::commitProvisionalLoad() 13 0x10789bb9c WebCore::DocumentLoader::commitIfReady() 14 0x10789c1bc WebCore::DocumentLoader::commitLoad(char const*, int) 15 0x10789c73b WebCore::DocumentLoader::receivedData(char const*, int) 16 0x108522167 WebCore::MainResourceLoader::addData(char const*, int, bool) 17 0x108997b0c WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) 18 0x1085238da WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) 19 0x10899843f WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) 20 0x108994daa -[WebCoreResourceHandleAsDelegate connection:didReceiveData:lengthReceived:] 21 0x7fff871f4f58 __65-[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:]_block_invoke_0 22 0x7fff871f4e9c -[NSURLConnectionInternal _withConnectionAndDelegate:onlyActive:] 23 0x7fff871f4d98 -[NSURLConnectionInternal _withActiveConnectionAndDelegate:] 24 0x7fff871f792b _NSURLConnectionDidReceiveData_LengthReceived 25 0x7fff8c3816c4 ___delegate_didReceiveDataArray_block_invoke_0 26 0x7fff8c3743ca ___withDelegateAsync_block_invoke_0 27 0x7fff8c40456a __block_global_1 28 0x7fff87caa724 CFArrayApplyFunction 29 0x7fff8c365554 RunloopBlockContext::perform() 30 0x7fff8c36542b MultiplexerSource::perform() 31 0x7fff87c8c101 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
Created attachment 185434 [details] patch
Created attachment 185436 [details] 1st patch
Comment on attachment 185436 [details] 1st patch View in context: https://bugs.webkit.org/attachment.cgi?id=185436&action=review > LayoutTests/fast/regions/detaching-regions-with-anonymous-blocks.html:27 > + var b = document.createElement("body"); Let's use real variable name, not 1 letter. > LayoutTests/fast/regions/detaching-regions-with-anonymous-blocks.html:29 > + b.innerHTML = document.getElementById("testResult").innerHTML; I would prefer if you just inlined the tag above here instead of having a hack using script / template: b.innerHTML = "<p>Bug <a href="https://bugs.webkit.org/show_bug.cgi?id=104517">104517</a>: [CSSRegions] Assertion failure in Node::detach (!renderer || renderer->inRenderFlowThread())</p><p>This test PASSES if it does not CRASH or ASSERT.</p>"; > LayoutTests/fast/regions/detaching-regions-with-anonymous-blocks.html:32 > + if(window.testRunner) > + window.testRunner.notifyDone(); No need for that. DRT by default waits until the 'load' event listener has run. > Source/WebCore/rendering/RenderObject.cpp:286 > + ASSERT(b != child->inRenderFlowThread()); I am not totally convinced you can ASSERT here. Is it possible to have a RenderFlowThread inside a RenderFlowThread? (in which case you can totally trigger it) This ASSERT seems to be performance related (as in you could skip a subtree), not really functional in nature. > Source/WebCore/rendering/RenderObject.h:437 > + void setInRenderFlowThreadRecursive(bool = true); Better names: setInRenderFlowThreadIncludingDescendants or setSubtreeInRenderFlowThread (prefer the first one though).
Comment on attachment 185436 [details] 1st patch View in context: https://bugs.webkit.org/attachment.cgi?id=185436&action=review >> Source/WebCore/rendering/RenderObject.cpp:286 >> + ASSERT(b != child->inRenderFlowThread()); > > I am not totally convinced you can ASSERT here. Is it possible to have a RenderFlowThread inside a RenderFlowThread? (in which case you can totally trigger it) > > This ASSERT seems to be performance related (as in you could skip a subtree), not really functional in nature. It's not possible to have a RenderFlowThread inside a RenderFlowThread. This assert will catch such occurrences (apart from catching the other situations in which a subtree is already in the desired state and the call would just waste computing power).
Created attachment 186613 [details] incorporated Julien Chaffraix's feedback.
Created attachment 186616 [details] incorporated Julien Chaffraix's feedback (with updated ChangeLogs)
Comment on attachment 186616 [details] incorporated Julien Chaffraix's feedback (with updated ChangeLogs) Attachment 186616 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16368748
Comment on attachment 186616 [details] incorporated Julien Chaffraix's feedback (with updated ChangeLogs) View in context: https://bugs.webkit.org/attachment.cgi?id=186616&action=review r=me > LayoutTests/ChangeLog:16 > + The RenderObject::inRenderFlowThread bit could have become disconnected from the fact that the RenderObject > + has (or not) an enclosing RenderFlowThread. > + The cause of this was that, when setting or removing the parent of a RenderObject, the inRenderFlowThread flags > + wasn't being set/reset for the children too. > + This is now fixed by calling the new setInRenderFlowThreadIncludingDescendants. > + > + The ASSERT was hit for anonymous blocks when detaching the document. > + > + The test starts from such an anonymous block and the detaches the body of document. Nit: We usually don't copy the WebCore ChangeLog here. (Most of the time I keep the LayoutTests ChangeLog empty unless there is something important to mention).
Created attachment 186792 [details] Patch for landing. Includes the change Julien requested for LayoutTests/ChangeLog.
Comment on attachment 186792 [details] Patch for landing. Includes the change Julien requested for LayoutTests/ChangeLog. Clearing flags on attachment: 186792 Committed r141982: <http://trac.webkit.org/changeset/141982>