Fix Python reference count leak #38
Conversation
Codecov Report
@@ Coverage Diff @@
## master #38 +/- ##
=========================================
Coverage ? 80.22%
=========================================
Files ? 16
Lines ? 1345
Branches ? 0
=========================================
Hits ? 1079
Misses ? 266
Partials ? 0
Continue to review full report at Codecov.
|
|
This will no longer free the objects if you have a single long-lived context and the weak callback does end up getting called. Can you fix that? |
|
From our experiences in production, SetWeak barely ever gets called (even with the context deleted and the v8 garbage collector pumped), and the v8 docs state SetWeak shouldn't be used for freeing resources because of this. That aside, could you give an example of the circumstances that the weak callback would be called, outside of deleting the context? My very rough understanding on how this works:
I'm guessing, but would the only other time SetWeak gets invoked be if v8 frees a JS object for whatever reason, after which the entry should be removed from js_object_cache? |
|
It sounds like in your use case the context gets cleaned up pretty quickly, but I can imagine other use cases where you create a single context that lives for a long time (e.g. a theoretical Python rewrite of Node). If you do that with this PR the js_object_cache would grow without bound. The only hope for solving that problem is hoping V8 will call the weak callback eventually. Also this introduces a UAF: if you add a Python object to context A, make that Python object accessible from context B without going through py_class_create_js_object ( I just had a look at how Chromium deals with this. The code is at https://source.chromium.org/chromium/chromium/src/+/master:gin/wrappable.cc. Ctrl-F "delete" shows that they delete the object from the weak callback, and also if NewInstance() returns nothing (which in v8py will just crash, so apparently this never happens?). So the V8 docs say not to rely on the weak callback for freeing resources, while Chromium does anyway. Not sure who to believe. |
|
The use-after-free I can't seem to replicate; my understanding is the refcount for a Python object is increased when it is added to each context, so removing one context altogether wouldn't be enough to deallocate the object. After maybe the most punishing troubleshooting process ever, I think I have a replacement weak callback. As part of garbage collection v8 will trash the contents of the v8::Context before the weak callback gets invoked, so it's impossible to use context->GetEmbedderData. I had to add another pointer for js_object_cache to each js object. |
Callbacks set by Persistent::SetWeak aren't guaranteed to be called by V8, even if the context gets destroyed. As such, the majority of Python objects passed into the context will never be freed. To fix this, we replace js_object_cache (which was a weakref.WeakKeyDictionary) with a normal dict. This ensures the reference count is managed with the lifetime of the dictionary.
scottp-dpaw
force-pushed
the
refcount_fix
branch
from
edb60a3
to
bc66490
Compare
|
Everything should be sorted now, is there anything else required for review? It would be fantastic if this could be rolled into a point release, we've found with this fix in place v8py-using threads use roughly half the memory. |
|
I've decided to accept the reality that I don't have time or motivation to maintain this project anymore and add a section in the readme asking for maintainers. This PR seems like evidence that you care about the project more than me :) so please reach out if you're interested in maintaining. |
Callbacks set by Persistent::SetWeak aren't guaranteed to be called by
V8, even if the context gets destroyed. As such, the majority of Python
objects passed into the context will never be freed.
To fix this, this patch replaces js_object_cache (which was a
weakref.WeakKeyDictionary) with a normal dict. This ensures the
reference count is managed with the lifetime of the dictionary.