-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Description
We've had bugs in a number of plugins where just setting them up creates a retain cycle that keeps the engine alive forever (e.g., due to engine foot-guns like #102990).
We should have a test in every plugin that calls registerWithRegistrar with a fake registrar that holds a strong reference to it when a channel handler is added, and also that returns itself as any of the called helper objects (binary messenger, texture registrar, etc.—using a strict mock to make sure we aren't missing any and accidentally returning nil). The test should drop its reference to the fake registrar, and then ensure that it nils out.
This would be more strict than the engine actually is (e.g., the binary messenger won't make a loop any more), but it should probably never actually be necessary to have a strong reference to something that's owned by the engine anyway, because if the engine is gone calling it is at best a no-op anyway (and in some cases an error).
This would ensure we aren't relying on add-to-app customers to find these one at a time, and also be a good backstop against accidentally introducing retain loop foot-guns with any new engine APIs.