RESOLVED FIXED180232
Remove virtual function calls in GraphicsLayer destructors 
https://bugs.webkit.org/show_bug.cgi?id=180232
Summary Remove virtual function calls in GraphicsLayer destructors
Michael Catanzaro
Reported 2017-11-30 19:06:23 PST
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.
Attachments
Patch (9.75 KB, patch)
2017-11-30 19:35 PST, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (4.32 KB, patch)
2018-05-31 15:17 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (9.64 KB, patch)
2018-11-23 12:22 PST, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (9.19 KB, patch)
2019-05-13 06:27 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (7.46 KB, patch)
2019-05-13 06:38 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Patch (7.46 KB, patch)
2021-04-20 13:45 PDT, Michael Catanzaro 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 2017-11-30 19:35:13 PST
Created attachment 328066 [details] Patch
Michael Catanzaro
Comment 2 2017-11-30 21:29:30 PST
This is actually more closely-related to bug #166568 than I thought. See that bug also.
Michael Catanzaro
Comment 3 2017-12-01 09:31:43 PST
(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.
Michael Catanzaro
Comment 4 2018-01-03 10:35:10 PST
This patch doesn't fix anything, but it (hopefully) makes the code a bit more robust... any reviewers interested?
Michael Catanzaro
Comment 5 2018-04-09 17:14:22 PDT
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.
Michael Catanzaro
Comment 6 2018-05-31 15:14:59 PDT
Zan suggested a less-intrusive approach
Michael Catanzaro
Comment 7 2018-05-31 15:17:25 PDT
Created attachment 341697 [details] Patch
Michael Catanzaro
Comment 8 2018-11-23 11:59:43 PST
(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.
Michael Catanzaro
Comment 9 2018-11-23 12:22:37 PST
Created attachment 355534 [details] Patch
Michael Catanzaro
Comment 10 2019-01-01 12:03:32 PST
Ping reviewers
Michael Catanzaro
Comment 11 2019-05-06 19:01:34 PDT
I'm tired of seeing this in my request queue, so if nobody wants to review it, I'm going to close it.
Adrian Perez
Comment 12 2019-05-07 01:48:19 PDT
(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.
Simon Fraser (smfr)
Comment 13 2019-05-07 13:38:52 PDT
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?
Michael Catanzaro
Comment 14 2019-05-07 14:55:28 PDT
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?
Michael Catanzaro
Comment 15 2019-05-10 08:56:37 PDT
I'll do that.
Michael Catanzaro
Comment 16 2019-05-13 06:27:52 PDT
Created attachment 369726 [details] Patch
Michael Catanzaro
Comment 17 2019-05-13 06:38:45 PDT
Created attachment 369727 [details] Patch
Michael Catanzaro
Comment 18 2019-06-06 06:24:46 PDT
Ping reviewers
Michael Catanzaro
Comment 19 2021-04-20 13:44:19 PDT
(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.
Michael Catanzaro
Comment 20 2021-04-20 13:45:46 PDT
Created attachment 426588 [details] Patch
Adrian Perez
Comment 21 2021-04-23 12:09:12 PDT
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 :)
EWS
Comment 22 2021-04-23 12:51:53 PDT
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].
Radar WebKit Bug Importer
Comment 23 2021-04-24 15:02:16 PDT
<rdar://problem/77110017>
Note You need to log in before you can comment on or make changes to this bug.