Summary: | Move code out of renderer destructors into willBeDestroyed() | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, buildbot, koivisto, rniwa, simon.fraser, webkit-bug-importer, wenson_hsieh, zalan | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2017-03-14 18:06:13 PDT
Why not just delete rare data in RenderObject destructor instead? Because RenderWidget is special and its lifetime can be extended (by manual refcounting) after willBeDestroyed() has run. Weird. 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. Created attachment 304739 [details]
Patch
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. 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 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. 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 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. 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 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. 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
Created attachment 304754 [details]
Patch
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 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
Created attachment 304820 [details]
Patch
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 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. Created attachment 304869 [details]
Patch
Updated changelog with more justification. Created attachment 304871 [details]
Patch
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? (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; |