| Summary: | Remove virtual function calls in GraphicsLayer destructors | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||||||
| Component: | Platform | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | aperez, bugs-noreply, cgarcia, cmarcelo, ews-watchlist, kondapallykalyan, luiz, mcatanzaro, noam, sergio, simon.fraser, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | Other | ||||||||||||||||
| Hardware: | PC | ||||||||||||||||
| OS: | Linux | ||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=166568 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
Created attachment 328066 [details]
Patch
This is actually more closely-related to bug #166568 than I thought. See that bug also. (In reply to Michael Catanzaro from comment #2) > This is actually more closely-related to bug #166568 than I thought. See > that bug also. Wrong. It's a separate problem, just touches on the same code. Sorry for the noise. This patch doesn't fix anything, but it (hopefully) makes the code a bit more robust... any reviewers interested? The rollout in bug #184406 illustrates the sort of problem that can occur when we call virtual functions from constructors and destructors. The existing GraphicsLayer code here is quite fragile. So this would still be worth a review, IMO. Zan suggested a less-intrusive approach Created attachment 341697 [details]
Patch
(In reply to Michael Catanzaro from comment #6) > Zan suggested a less-intrusive approach From the EWS failures, we see that marking CoordinatedGraphicsLayerCA as final does not work because it is inherited by GraphicsLayerCARemote. We should go back to the original approach. Created attachment 355534 [details]
Patch
Ping reviewers I'm tired of seeing this in my request queue, so if nobody wants to review it, I'm going to close it. (In reply to Michael Catanzaro from comment #11) > I'm tired of seeing this in my request queue, so if nobody wants to review > it, I'm going to close it. Please let's not abandon the patch to bitrot =) I took a look at this, and as far as I understand the proposed changes are fine and, like you, I also think that this should not break anything — if I was reviewer, this would be a r+ for me. At any rate, this touches GraphicsLayerCA.{h,cpp} so probably we want someone familiar with the Apple ports to rubber-stamp the patch anyway. Comment on attachment 355534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355534&action=review > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:539 > +void GraphicsLayerCA::doRemoveFromParent() It's confusing to have removeFromParent, removeFromParentInternal and doRemoveFromParent. Can we factor this more cleanly? Comment on attachment 355534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355534&action=review >> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:539 >> +void GraphicsLayerCA::doRemoveFromParent() > > It's confusing to have removeFromParent, removeFromParentInternal and doRemoveFromParent. Can we factor this more cleanly? I can replace the doRemoveFromParent function with its two-line implementation in the two places it's used, would that be OK? I'll do that. Created attachment 369726 [details]
Patch
Created attachment 369727 [details]
Patch
Ping reviewers (In reply to Adrian Perez from comment #12) > Please let's not abandon the patch to bitrot =) > > I took a look at this, and as far as I understand the proposed > changes are fine and, like you, I also think that this should not > break anything — if I was reviewer, this would be a r+ for me. At > any rate, this touches GraphicsLayerCA.{h,cpp} so probably we want > someone familiar with the Apple ports to rubber-stamp the patch > anyway. Hey Adrian... you're a reviewer now. Refresh incoming. Act soon and we can land before the patch turns four. Created attachment 426588 [details]
Patch
Comment on attachment 426588 [details]
Patch
LGTM, please keep an eye on bots after landing, but I do not
expect much trouble given that the EWS is happy with this :)
Committed r276513 (236969@main): <https://commits.webkit.org/236969@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426588 [details]. |
I notice that ~CoordinatedGraphicsLayer makes a virtual function call to GraphicsLayer::willBeDestroyed, which makes a virtual function call to CoordinatedGraphicsLayer::removeFromParent. I think that the functions are being called as intended, because ~CoordinatedGraphicsLayer has not yet been fully destroyed. However, I'm reminded of Effective C++ item #9: Never call virtual functions during construction or destruction ("because such calls will never go to a more derived class than that of the currently executing constructor or destructor"). This code is almost certain to break if anyone tries in the future to subclass any of the existing subclasses of GraphicsLayer, so let's refactor it a bit. I'm a bit nervous touching this code, but I think my changes are safe.