Bug 180232 - Remove virtual function calls in GraphicsLayer destructors
Summary: Remove virtual function calls in GraphicsLayer destructors
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-30 19:06 PST by Michael Catanzaro
Modified: 2019-06-06 06:24 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.75 KB, patch)
2017-11-30 19:35 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (4.32 KB, patch)
2018-05-31 15:17 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (9.64 KB, patch)
2018-11-23 12:22 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (9.19 KB, patch)
2019-05-13 06:27 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (7.46 KB, patch)
2019-05-13 06:38 PDT, Michael Catanzaro
mcatanzaro: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 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.
Comment 1 Michael Catanzaro 2017-11-30 19:35:13 PST
Created attachment 328066 [details]
Patch
Comment 2 Michael Catanzaro 2017-11-30 21:29:30 PST
This is actually more closely-related to bug #166568 than I thought. See that bug also.
Comment 3 Michael Catanzaro 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.
Comment 4 Michael Catanzaro 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?
Comment 5 Michael Catanzaro 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.
Comment 6 Michael Catanzaro 2018-05-31 15:14:59 PDT
Zan suggested a less-intrusive approach
Comment 7 Michael Catanzaro 2018-05-31 15:17:25 PDT
Created attachment 341697 [details]
Patch
Comment 8 Michael Catanzaro 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.
Comment 9 Michael Catanzaro 2018-11-23 12:22:37 PST
Created attachment 355534 [details]
Patch
Comment 10 Michael Catanzaro 2019-01-01 12:03:32 PST
Ping reviewers
Comment 11 Michael Catanzaro 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.
Comment 12 Adrian Perez 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.
Comment 13 Simon Fraser (smfr) 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?
Comment 14 Michael Catanzaro 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?
Comment 15 Michael Catanzaro 2019-05-10 08:56:37 PDT
I'll do that.
Comment 16 Michael Catanzaro 2019-05-13 06:27:52 PDT
Created attachment 369726 [details]
Patch
Comment 17 Michael Catanzaro 2019-05-13 06:38:45 PDT
Created attachment 369727 [details]
Patch
Comment 18 Michael Catanzaro 2019-06-06 06:24:46 PDT
Ping reviewers