Bug 137273

Summary: Descendant is being put into the wrong flow thread with nested columns and spans
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: abucur, bdakin, commit-queue, darin, dbates, ddkilzer, esprehn+autocc, glenn, kling, koivisto, kondapallykalyan, mihnea, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=141612
Attachments:
Description Flags
Patch simon.fraser: review+

Description Dave Hyatt 2014-09-30 15:11:40 PDT
Descendant is being put into the wrong flow thread with nested columns and spans. See the FIXME in isValidColumnSpanner in RenderMultiColumnFlowThread.cpp.
Comment 1 Dave Hyatt 2014-11-04 17:20:46 PST
Created attachment 240986 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-11-05 09:11:58 PST
Comment on attachment 240986 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240986&action=review

> Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:387
> +                placeholder.parent()->removeChild(placeholder);

I think you want a "// FIXME: placeholder is leaked" here.
Comment 3 David Kilzer (:ddkilzer) 2014-11-05 09:14:19 PST
Comment on attachment 240986 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240986&action=review

> Source/WebCore/ChangeLog:20
> +        The second fix was to stop destroying the placeholder. Since the placeholder can just have been inserted, you
> +        can't delete it, since otherwise code further up the stack will access the deleted object. For now, we just
> +        leak the placeholder.

Would using a RenderPtr<> for the placeholder (née descendant) fix the leak/object lifetime?

>> Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp:387
>> -                ancestorBlock.multiColumnFlowThread()->handleSpannerRemoval(spanner);
>> -                placeholder.destroy();
>> -                
>> +                if (subtreeRoot == descendant)
>> +                    subtreeRoot = spanner;
>> +                placeholder.parent()->removeChild(placeholder);
> 
> I think you want a "// FIXME: placeholder is leaked" here.

And a new bug number that tracks the leak please.
Comment 4 Dave Hyatt 2014-11-05 13:26:43 PST
Fixed in r175641.
Comment 5 Radar WebKit Bug Importer 2014-11-05 13:27:02 PST
<rdar://problem/18885189>
Comment 6 David Kilzer (:ddkilzer) 2015-02-03 15:49:39 PST
*** Bug 137316 has been marked as a duplicate of this bug. ***