commit a028fc35a1eb2c9ef4d6924ba54c09af86bd1213
author global-integration-roller <global-integration-roller@fuchsia.infra.roller.fuchsia-infra.iam.gserviceaccount.com> Thu May 25 19:55:31 2023 +0000
committer Copybara-Service <copybara-worker@google.com> Thu May 25 12:57:13 2023 -0700
tree 7b9f6f2851ddd905867b2f2447fd8a0928f75f81
parent 67fe949c6c5105e647ef91b745f808e3384c7cc2

[roll] Roll fuchsia [superproject] Roll third_party/pigweed pw_rpc: Add destructor to stateful Call subclasses

b5f8cd9 moved waiting for callback completion from Call's destructors
into the destructors of some subclasses. This helped ensure that the
callbacks were completed prior to the destruction of fields referenced
by those callbacks.

However, that change missed a number of classes, and applied the
destructor to some classes where no extra state was carried (except
transitively through parent classes). This resulted in some bugs,
notably b/282138354.

This change moves the DestroyClientCall and DestroyServerCall
invocations into two places:
 - The destructors of any stateful Call subclasses (those subclasses
   whose fields may be referenced by the callbacks).
 - The destructors of ServerCall and ClientCall (to guard the stateful
   `Call` class appropriately in the case of classes whose heirarchy
   would not otherwise have invoked `DestroyXXXCall`).

This results in some duplication, notably between classes where multiple
layers in the inheritance tree are stateful: each stateful layer will
`DestroyXXXCall`. However, the alternative is leaving intermediate
classes unprotected, which makes it undefined behavior to create an
instance of such an intermediate class, and makes it hard to track
whether the final subclasses are properly protected: they must include a
destructor if and only if their parents had state and did *not* include
a destructor. This condition is much too complex to validate, especially
given that Call has 40+ subclasses, many with four-deep inheritance.

After this CL, it should be easy to check a class for correctness:
it must include a `DestroyXXXCall`-invoking destructor if it has a field
whose state may be referenced from a callback.

Original-Bug: b/282138354
Original-Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/148110
Original-Revision: ea55bee9faa2b0f0249b3236355e6649c3e70da0
GitOrigin-RevId: 9ae32d2d8de4cac41c66dd991d8c3bf3845c2ed7
Roller-URL: https://ci.chromium.org/b/8780096653669839697
CQ-Do-Not-Cancel-Tryjobs: true
Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/862077
Original-Revision: 09427349894cc9a4d4e18d4b7a4ed1f6bbe8f67b
Change-Id: I828ba8247073a75e9cb4b8635c9121a313b4d99c