fix: clean up base::LinkedList in context_bridge::ObjectCache #27630
Conversation
base::LinkedList does not delete its members on destruction. We need to manually ensure the linkedlist is empty when the ObjectCache is destroyed. Fixes #27039 Notes: Fixed memory leak when sending non-primitives over the context bridge
| delete node; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a little larger scope, but -- what is the benefit of using base::LinkedList here instead of std::list, which is simpler / doesn't require a ObjectCachePairNode / doesn't have these gotchas?
According to the docs, the main reason to use base::LinkedList are for performance reasons of (1) removal is O(1) rather than O(n), and (2) insertion with base::LinkedList never requires heap allocations. But removal never called except in the destructor, and the items we insert are heap-allocated ObjectCachePairNodes, so do either of those advantages apply here?
If we use a simpler container here, this destructor paragraph would go away entirely as unnecessary
There was a problem hiding this comment.
Agree in general, I want to land this fix then I'll follow it up to see if we can remove our usage of LinkedList. I can't remember exactly why we used it originally, but this code has been through several refactors and it's possible our usage of LinkedList is no longer required.
|
Release Notes Persisted
|
|
I have automatically backported this PR to "12-x-y", please check out #27636 |
|
I have automatically backported this PR to "10-x-y", please check out #27637 |
|
I have automatically backported this PR to "11-x-y", please check out #27638 |
base::LinkedList does not delete its members on destruction. We need to manually ensure the linkedlist is empty when the ObjectCache is destroyed.
Fixes #27039
Notes: Fixed memory leak when sending non-primitives over the context bridge