Bug 182518 – [RenderTreeBuilder] Remove RenderElement::destroyLeftoverChildren.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[alan]

delete them right before destroying the parent.

[Radar WebKit Bug Importer]

<rdar://problem/37256035>

[alan]

Created attachment 333133 [details]
Patch

[alan]

Created attachment 333211 [details]
Patch

[alan]

Created attachment 333213 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[alan]

Created attachment 333257 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[alan]

Created attachment 333266 [details]
Patch

[Antti Koivisto]

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

[alan]

(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.

[alan]

Created attachment 333301 [details]
Patch

[alan]

Created attachment 333303 [details]
Patch

[Antti Koivisto]

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?

[alan]

(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.

[Antti Koivisto]

(In reply to zalan from comment #20)
> Let me test it.

Might be easier to just turn this into a loop.

[alan]

Created attachment 333306 [details]
Patch

[Antti Koivisto]

Comment on attachment 333306 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 333306 [details]
Patch

Clearing flags on attachment: 333306

Committed r228238: <https://trac.webkit.org/changeset/228238>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.