Bug 182518 - [RenderTreeBuilder] Remove RenderElement::destroyLeftoverChildren.
Summary: [RenderTreeBuilder] Remove RenderElement::destroyLeftoverChildren.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-05 16:57 PST by zalan
Modified: 2018-02-07 11:52 PST (History)
8 users (show)

See Also:


Attachments
Patch (6.83 KB, patch)
2018-02-05 17:02 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (7.41 KB, patch)
2018-02-06 13:45 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (7.50 KB, patch)
2018-02-06 13:52 PST, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews201 for win-future (10.79 MB, application/zip)
2018-02-06 16:33 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews200 for win-future (11.58 MB, application/zip)
2018-02-06 18:13 PST, EWS Watchlist
no flags Details
Patch (7.59 KB, patch)
2018-02-06 19:24 PST, zalan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (2.26 MB, application/zip)
2018-02-06 20:42 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews204 for win-future (11.50 MB, application/zip)
2018-02-06 21:47 PST, EWS Watchlist
no flags Details
Patch (7.40 KB, patch)
2018-02-06 22:12 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (7.21 KB, patch)
2018-02-07 10:19 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (7.37 KB, patch)
2018-02-07 10:28 PST, zalan
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2018-02-07 11:18 PST, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.