Bug 169650 - Move code out of renderer destructors into willBeDestroyed()
Summary: Move code out of renderer destructors into willBeDestroyed()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-14 18:06 PDT by Simon Fraser (smfr)
Modified: 2017-03-20 10:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (37.66 KB, patch)
2017-03-16 19:31 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (741.07 KB, application/zip)
2017-03-16 20:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (389.64 KB, application/zip)
2017-03-16 20:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (709.62 KB, application/zip)
2017-03-16 20:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (6.12 MB, application/zip)
2017-03-16 20:31 PDT, Build Bot
no flags Details
Patch (36.37 KB, patch)
2017-03-16 21:46 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-elcapitan (1.79 MB, application/zip)
2017-03-17 01:52 PDT, Build Bot
no flags Details
Patch (42.38 KB, patch)
2017-03-17 13:51 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (42.83 KB, patch)
2017-03-18 10:30 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (42.94 KB, patch)
2017-03-18 10:39 PDT, Simon Fraser (smfr)
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2017-03-14 18:06:13 PDT
It's wrong for there to be code in the destructors of RenderObjects and subclasses (e.g. RenderElement), because this code runs after rareData() has been destroyed, so may have incorrect behavior.
Comment 1 Radar WebKit Bug Importer 2017-03-14 18:06:50 PDT
<rdar://problem/31053112>
Comment 2 Antti Koivisto 2017-03-15 05:32:04 PDT
Why not just delete rare data in RenderObject destructor instead?
Comment 3 Simon Fraser (smfr) 2017-03-15 08:09:48 PDT
Because RenderWidget is special and its lifetime can be extended (by manual refcounting) after willBeDestroyed() has run.
Comment 4 Antti Koivisto 2017-03-15 12:16:15 PDT
Weird.
Comment 5 Antti Koivisto 2017-03-15 12:24:00 PDT
I don't understand how that prevents moving rare data deletion to RenderObject destructor though. It would make sense that object and its rare data have the same lifetime.
Comment 6 Simon Fraser (smfr) 2017-03-16 19:31:23 PDT
Created attachment 304739 [details]
Patch
Comment 7 Build Bot 2017-03-16 20:28:34 PDT
Comment on attachment 304739 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 8 Build Bot 2017-03-16 20:28:38 PDT
Created attachment 304743 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-03-16 20:29:35 PDT
Comment on attachment 304739 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 10 Build Bot 2017-03-16 20:29:38 PDT
Created attachment 304744 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-03-16 20:30:19 PDT
Comment on attachment 304739 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 12 Build Bot 2017-03-16 20:30:22 PDT
Created attachment 304745 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 13 Build Bot 2017-03-16 20:31:36 PDT
Comment on attachment 304739 [details]
Patch

Attachment 304739 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3345743

Number of test failures exceeded the failure limit.
Comment 14 Build Bot 2017-03-16 20:31:39 PDT
Created attachment 304746 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 15 Simon Fraser (smfr) 2017-03-16 21:46:20 PDT
Created attachment 304754 [details]
Patch
Comment 16 Build Bot 2017-03-17 01:52:07 PDT
Comment on attachment 304754 [details]
Patch

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

New failing tests:
imported/blink/fast/pagination/viewport-x-vertical-lr-rtl.html
imported/blink/fast/pagination/viewport-y-horizontal-bt-ltr.html
Comment 17 Build Bot 2017-03-17 01:52:11 PDT
Created attachment 304767 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 18 Simon Fraser (smfr) 2017-03-17 13:51:05 PDT
Created attachment 304820 [details]
Patch
Comment 19 Antti Koivisto 2017-03-18 03:42:59 PDT
Comment on attachment 304820 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        For all classes that derive from RenderObject, move code from the destructor into
> +        willBeDestroyed(). New willBeDestroyed() implementations must call the base class.

Please explain in the ChangeLog what problem this solves and how.
Comment 20 Antti Koivisto 2017-03-18 03:49:21 PDT
Comment on attachment 304820 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        For all classes that derive from RenderObject, move code from the destructor into
>> +        willBeDestroyed(). New willBeDestroyed() implementations must call the base class.
> 
> Please explain in the ChangeLog what problem this solves and how.

Please explain in the ChangeLog what problem this solves and how.

> Source/WebCore/rendering/RenderBlockFlow.cpp:123
>  RenderBlockFlow::~RenderBlockFlow()
>  {
> +    // Do not add any code here. Add it to willBeDestroyed() instead.
>  }

Destructors of members still run here implicitly and they may contain any amount of code. Is all that code banned now too? Generally this sort of rules feel pretty arbitrary.
Comment 21 Simon Fraser (smfr) 2017-03-18 10:30:40 PDT
Created attachment 304869 [details]
Patch
Comment 22 Simon Fraser (smfr) 2017-03-18 10:30:59 PDT
Updated changelog with more justification.
Comment 23 Simon Fraser (smfr) 2017-03-18 10:39:30 PDT
Created attachment 304871 [details]
Patch
Comment 24 Antti Koivisto 2017-03-20 10:10:23 PDT
Comment on attachment 304871 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:3415
> -    ASSERT(rareData->m_flowThreadContainingBlock.value() == RenderBox::locateFlowThreadContainingBlock());
> +//    ASSERT(rareData->m_flowThreadContainingBlock.value() == RenderBox::locateFlowThreadContainingBlock());

Why is this commented out?

> Source/WebCore/rendering/RenderNamedFlowThread.cpp:64
> +void RenderNamedFlowThread::willBeDestroyed()
> +{
> +    WTFLogAlways("RenderNamedFlowThread %p willBeDestroyed", this);

This seems too spammy.

> Source/WebCore/rendering/RenderObject.cpp:1482
>  

Could/should we have an assert in ~RenderObject to verify that willBeDestroyed has always been called?
Comment 25 Simon Fraser (smfr) 2017-03-20 10:25:39 PDT
(In reply to comment #24)
> Comment on attachment 304871 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304871&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:3415
> > -    ASSERT(rareData->m_flowThreadContainingBlock.value() == RenderBox::locateFlowThreadContainingBlock());
> > +//    ASSERT(rareData->m_flowThreadContainingBlock.value() == RenderBox::locateFlowThreadContainingBlock());
> 
> Why is this commented out?
> 
> > Source/WebCore/rendering/RenderNamedFlowThread.cpp:64
> > +void RenderNamedFlowThread::willBeDestroyed()
> > +{
> > +    WTFLogAlways("RenderNamedFlowThread %p willBeDestroyed", this);
> 
> This seems too spammy.

Sorry, those were debugging changes. Will remove.

> > Source/WebCore/rendering/RenderObject.cpp:1482
> >  
> 
> Could/should we have an assert in ~RenderObject to verify that
> willBeDestroyed has always been called?

I don't think that's necessary given that code in RenderObject::destroy() does:

    willBeDestroyed();
...
    delete this;
Comment 26 Simon Fraser (smfr) 2017-03-20 10:29:50 PDT
https://trac.webkit.org/r214173