Summary: | [CSSRegions] Assertion failure in Node::detach (!renderer || renderer->inRenderFlowThread()) | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihnea Ovidenie <mihnea> | ||||||||||||
Component: | CSS | Assignee: | 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
Mihnea Ovidenie
2012-12-10 00:07:14 PST
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> |