Bug 182518

Summary: [RenderTreeBuilder] Remove RenderElement::destroyLeftoverChildren.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, ews-watchlist, koivisto, rniwa, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews201 for win-future
none
Archive of layout-test-results from ews200 for win-future
none
Patch
none
Archive of layout-test-results from ews100 for mac-sierra
none
Archive of layout-test-results from ews204 for win-future
none
Patch
none
Patch
none
Patch
none
Patch none

Description zalan 2018-02-05 16:57:35 PST
delete them right before destroying the parent.
Comment 1 Radar WebKit Bug Importer 2018-02-05 17:00:13 PST
<rdar://problem/37256035>
Comment 2 zalan 2018-02-05 17:02:09 PST
Created attachment 333133 [details]
Patch
Comment 3 zalan 2018-02-06 13:45:12 PST
Created attachment 333211 [details]
Patch
Comment 4 zalan 2018-02-06 13:52:48 PST
Created attachment 333213 [details]
Patch
Comment 5 EWS Watchlist 2018-02-06 16:33:23 PST
Comment on attachment 333213 [details]
Patch

Attachment 333213 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6389312

New failing tests:
fast/parser/area-in-div.html
Comment 6 EWS Watchlist 2018-02-06 16:33:33 PST
Created attachment 333234 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 7 EWS Watchlist 2018-02-06 18:13:15 PST
Comment on attachment 333213 [details]
Patch

Attachment 333213 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6390379

New failing tests:
fast/parser/area-in-div.html
Comment 8 EWS Watchlist 2018-02-06 18:13:26 PST
Created attachment 333244 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 9 zalan 2018-02-06 19:24:18 PST
Created attachment 333257 [details]
Patch
Comment 10 EWS Watchlist 2018-02-06 20:42:00 PST
Comment on attachment 333257 [details]
Patch

Attachment 333257 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6393272

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html
Comment 11 EWS Watchlist 2018-02-06 20:42:01 PST
Created attachment 333261 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 12 EWS Watchlist 2018-02-06 21:47:45 PST
Comment on attachment 333257 [details]
Patch

Attachment 333257 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6393515

New failing tests:
fast/parser/area-in-div.html
Comment 13 EWS Watchlist 2018-02-06 21:47:55 PST
Created attachment 333263 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 14 zalan 2018-02-06 22:12:49 PST
Created attachment 333266 [details]
Patch
Comment 15 Antti Koivisto 2018-02-07 01:34:16 PST
Comment on attachment 333266 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:-1481
>      RELEASE_ASSERT(!m_bitfields.beingDestroyed());
>  
> -    if (is<RenderElement>(*this))
> -        downcast<RenderElement>(*this).destroyLeftoverChildren();

Can we assert here there are no leftover children?

> LayoutTests/TestExpectations:1706
> +webkit.org/b/182547 fast/parser/append-child-followed-by-document-write.html [ Pass Crash ]
> +webkit.org/b/182547 fast/parser/area-in-div.html [ Pass Crash ]

We shouldn't land crashes
Comment 16 zalan 2018-02-07 08:19:48 PST
(In reply to Antti Koivisto from comment #15)
> Comment on attachment 333266 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333266&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:-1481
> >      RELEASE_ASSERT(!m_bitfields.beingDestroyed());
> >  
> > -    if (is<RenderElement>(*this))
> > -        downcast<RenderElement>(*this).destroyLeftoverChildren();
> 
> Can we assert here there are no leftover children?
I added the assertion to RenderElement d'tor. RenderObjects can't have children.
Comment 17 zalan 2018-02-07 10:19:07 PST
Created attachment 333301 [details]
Patch
Comment 18 zalan 2018-02-07 10:28:06 PST
Created attachment 333303 [details]
Patch
Comment 19 Antti Koivisto 2018-02-07 10:36:17 PST
Comment on attachment 333303 [details]
Patch

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

> Source/WebCore/rendering/RenderView.cpp:619
> +    if (firstChild()) {
> +        ASSERT(!firstChild()->nextSibling());

Are you sure there can never be more than one of these?
Comment 20 zalan 2018-02-07 10:49:39 PST
(In reply to Antti Koivisto from comment #19)
> Comment on attachment 333303 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333303&action=review
> 
> > Source/WebCore/rendering/RenderView.cpp:619
> > +    if (firstChild()) {
> > +        ASSERT(!firstChild()->nextSibling());
> 
> Are you sure there can never be more than one of these?
Let me test it.
Comment 21 Antti Koivisto 2018-02-07 11:02:15 PST
(In reply to zalan from comment #20)
> Let me test it.

Might be easier to just turn this into a loop.
Comment 22 zalan 2018-02-07 11:18:22 PST
Created attachment 333306 [details]
Patch
Comment 23 Antti Koivisto 2018-02-07 11:21:08 PST
Comment on attachment 333306 [details]
Patch

r=me
Comment 24 WebKit Commit Bot 2018-02-07 11:52:50 PST
Comment on attachment 333306 [details]
Patch

Clearing flags on attachment: 333306

Committed r228238: <https://trac.webkit.org/changeset/228238>
Comment 25 WebKit Commit Bot 2018-02-07 11:52:52 PST
All reviewed patches have been landed.  Closing bug.