Bug 104517

Summary: [CSSRegions] Assertion failure in Node::detach (!renderer || renderer->inRenderFlowThread())
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Mihai Maerean <mmaerean>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, hyatt, jchaffraix, ojan.autocc, tony, WebkitBugTracker, webkit.review.bot
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
patch
none
1st patch
none
incorporated Julien Chaffraix's feedback.
none
incorporated Julien Chaffraix's feedback (with updated ChangeLogs)
jchaffraix: review+, buildbot: commit-queue-
Patch for landing. Includes the change Julien requested for LayoutTests/ChangeLog. none

Mihnea Ovidenie
Reported 2012-12-10 00:07:14 PST
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__
Attachments
patch (6.93 KB, patch)
2013-01-30 01:37 PST, Mihai Maerean 		no flags
DetailsFormatted DiffDiff
1st patch (6.93 KB, patch)
2013-01-30 01:42 PST, Mihai Maerean 		no flags
DetailsFormatted DiffDiff
incorporated Julien Chaffraix's feedback. (6.42 KB, patch)
2013-02-05 06:09 PST, Mihai Maerean 		no flags
DetailsFormatted DiffDiff
incorporated Julien Chaffraix's feedback (with updated ChangeLogs) (6.44 KB, patch)
2013-02-05 06:26 PST, Mihai Maerean 		jchaffraix: review+
buildbot: commit-queue-
DetailsFormatted DiffDiff
Patch for landing. Includes the change Julien requested for LayoutTests/ChangeLog. (5.97 KB, patch)
2013-02-06 01:51 PST, Mihai Maerean 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Mihai Maerean
Comment 1 2013-01-30 01:37:12 PST
Created attachment 185434 [details] patch
Mihai Maerean
Comment 2 2013-01-30 01:42:11 PST
Created attachment 185436 [details] 1st patch
Julien Chaffraix
Comment 3 2013-02-04 08:35:22 PST
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).
Mihai Maerean
Comment 4 2013-02-05 04:52:42 PST
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).
Mihai Maerean
Comment 5 2013-02-05 06:09:01 PST
Created attachment 186613 [details] incorporated Julien Chaffraix's feedback.
Mihai Maerean
Comment 6 2013-02-05 06:26:47 PST
Created attachment 186616 [details] incorporated Julien Chaffraix's feedback (with updated ChangeLogs)
Build Bot
Comment 7 2013-02-05 09:03:24 PST
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
Julien Chaffraix
Comment 8 2013-02-05 09:58:26 PST
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).
Mihai Maerean
Comment 9 2013-02-06 01:51:30 PST
Created attachment 186792 [details] Patch for landing. Includes the change Julien requested for LayoutTests/ChangeLog.
WebKit Review Bot
Comment 10 2013-02-06 03:33:51 PST
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>
Note You need to log in before you can comment on or make changes to this bug.