fix: do not leak IPC or context bridge promises #23321
Conversation
5901a13 to
f031c74
Compare
| v8::Global<v8::Context> global_then_destination_context( | ||
| destination_context->GetIsolate(), destination_context); |
There was a problem hiding this comment.
This is redundant with the promise's context right? Should we expose a reference to the promise helper's context?
There was a problem hiding this comment.
I don't think the promise should expose it's context, I'd rather the promise didn't hold a context at all 😆
| auto then_cb = base::BindOnce( | ||
| [](gin_helper::Promise<v8::Local<v8::Value>>* proxied_promise, | ||
| [](std::shared_ptr<gin_helper::Promise<v8::Local<v8::Value>>> | ||
| proxied_promise, | ||
| v8::Isolate* isolate, | ||
| v8::Global<v8::Context> global_source_context, |
There was a problem hiding this comment.
The source context should be the current context when then_cb is invoked right?
There was a problem hiding this comment.
That's making some assumptions about which context the promise was called from which isn't always gonna be the main world, E.g. an iframe could call it
| void WillReleaseScriptContext(v8::Local<v8::Context> context, | ||
| int32_t world_id) override { | ||
| if (weak_context_.IsEmpty() || | ||
| weak_context_.Get(context->GetIsolate()) == context) | ||
| electron_browser_ptr_.reset(); | ||
| } |
There was a problem hiding this comment.
If we make the promise helper's context reference weak, is this still needed?
There was a problem hiding this comment.
maybe, not sure, and it's probably more safe / correct to free this at the right time anyway
f031c74 to
9019f9d
Compare
9019f9d to
73fbc15
Compare
|
Release Notes Persisted
|
|
I was unable to backport this PR to "7-2-x" cleanly; |
|
I was unable to backport this PR to "8-x-y" cleanly; |
|
I have automatically backported this PR to "9-x-y", please check out #23338 |
|
@MarshallOfSound has manually backported this PR to "8-x-y", please check out #23339 |
This fixes three main memory leaks to do with our promise helper mostly:
IPCRendererwould hold pending promises forinvokecalls even after the render frame was removed, those promises had a strong ref to theContextso the context was never freedcontextBridgeproxy promise had a similar issue with heap allocating the promise, if it was never resolved or rejected it would never be released, thus never releasing the strong ref toContextNotes: Fixed memory leaks in sandbox mode when using
contextBridgewith promises oripcRenderer.invoke