Bug 180232 – Remove virtual function calls in GraphicsLayer destructors

• Component: Platform
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 328066 [details]
Patch

[Michael Catanzaro]

This is actually more closely-related to bug #166568 than I thought. See that bug also.

[Michael Catanzaro]

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

This patch doesn't fix anything, but it (hopefully) makes the code a bit more robust... any reviewers interested?

[Michael Catanzaro]

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]

Zan suggested a less-intrusive approach

[Michael Catanzaro]

Created attachment 341697 [details]
Patch

[Michael Catanzaro]

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

Created attachment 355534 [details]
Patch

[Michael Catanzaro]

Ping reviewers

[Michael Catanzaro]

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]

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

I'll do that.

[Michael Catanzaro]

Created attachment 369726 [details]
Patch

[Michael Catanzaro]

Created attachment 369727 [details]
Patch

[Michael Catanzaro]

Ping reviewers

[Michael Catanzaro]

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

Created attachment 426588 [details]
Patch

[Adrian Perez]

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]

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]

<rdar://problem/77110017>