Bug 169650 – Move code out of renderer destructors into willBeDestroyed()

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

[Simon Fraser (smfr)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/31053112>

[Antti Koivisto]

Why not just delete rare data in RenderObject destructor instead?

[Simon Fraser (smfr)]

Because RenderWidget is special and its lifetime can be extended (by manual refcounting) after willBeDestroyed() has run.

[Antti Koivisto]

Weird.

[Antti Koivisto]

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.

[Simon Fraser (smfr)]

Created attachment 304739 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Simon Fraser (smfr)]

Created attachment 304754 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Simon Fraser (smfr)]

Created attachment 304820 [details]
Patch

[Antti Koivisto]

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.

[Antti Koivisto]

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.

[Simon Fraser (smfr)]

Created attachment 304869 [details]
Patch

[Simon Fraser (smfr)]

Updated changelog with more justification.

[Simon Fraser (smfr)]

Created attachment 304871 [details]
Patch

[Antti Koivisto]

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?

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

https://trac.webkit.org/r214173