mirrors/electron
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: ensure MessagePorts get GCed when not referenced (#40189)

Browse source
This commit is contained in:
Shelley Vohr 2023-10-13 22:09:28 +02:00 committed by GitHub
parent 5d6023ae0d
commit bbd2236bdd
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 75 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
patches/chromium/.patches
View file

@ -135,3 +135,4 @@ fix_activate_background_material_on_windows.patch
fix_move_autopipsettingshelper_behind_branding_buildflag.patch
revert_remove_the_allowaggressivethrottlingwithwebsocket_feature.patch
fix_handle_no_top_level_aura_window_in_webcontentsimpl.patch
ensure_messageports_get_gced_when_not_referenced.patch

73
patches/chromium/ensure_messageports_get_gced_when_not_referenced.patch Normal file
View file

@ -0,0 +1,73 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Yoav Weiss <yoavweiss@chromium.org>
Date: Mon, 9 Oct 2023 14:21:44 +0000
Subject: Ensure MessagePorts get GCed when not referenced
[1] regressed MessagePort memory and caused them to no longer be
collected after not being referenced.
This CL fixes that by turning the mutual pointers between attached
MessagePorts to be WeakMembers.
[1] https://chromium-review.googlesource.com/4919476
Bug: 1487835
Change-Id: Icd7eba09a217ad5d588958504d5c4794d7d8a295
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4919476
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1207067}
diff --git a/third_party/blink/renderer/core/messaging/message_port.cc b/third_party/blink/renderer/core/messaging/message_port.cc
index 0fd5bbf170efa02d3e710de3cb6733158faec858..15f5f0f6aa05d3b17adae87286c92df9cc26a712 100644
--- a/third_party/blink/renderer/core/messaging/message_port.cc
+++ b/third_party/blink/renderer/core/messaging/message_port.cc
@@ -162,8 +162,10 @@ MessagePortChannel MessagePort::Disentangle() {
DCHECK(!IsNeutered());
port_descriptor_.GiveDisentangledHandle(connector_->PassMessagePipe());
connector_ = nullptr;
- if (initially_entangled_port_) {
- initially_entangled_port_->OnEntangledPortDisconnected();
+ // Using a variable here places the WeakMember pointer on the stack, ensuring
+ // it doesn't get GCed while it's being used.
+ if (auto* entangled_port = initially_entangled_port_.Get()) {
+ entangled_port->OnEntangledPortDisconnected();
}
OnEntangledPortDisconnected();
return MessagePortChannel(std::move(port_descriptor_));
@@ -340,7 +342,10 @@ bool MessagePort::Accept(mojo::Message* mojo_message) {
// lifetime of this function.
std::unique_ptr<scheduler::TaskAttributionTracker::TaskScope>
task_attribution_scope;
- if (initially_entangled_port_ && message.sender_origin &&
+ // Using a variable here places the WeakMember pointer on the stack, ensuring
+ // it doesn't get GCed while it's being used.
+ auto* entangled_port = initially_entangled_port_.Get();
+ if (entangled_port && message.sender_origin &&
message.sender_origin->IsSameOriginWith(context->GetSecurityOrigin()) &&
context->IsSameAgentCluster(message.sender_agent_cluster_id) &&
context->IsWindow()) {
@@ -364,9 +369,9 @@ bool MessagePort::Accept(mojo::Message* mojo_message) {
ThreadScheduler::Current()->GetTaskAttributionTracker()) {
// Since `initially_entangled_port_` is not nullptr, neither should be
// its `post_message_task_container_`.
- CHECK(initially_entangled_port_->post_message_task_container_);
+ CHECK(entangled_port->post_message_task_container_);
scheduler::TaskAttributionInfo* parent_task =
- initially_entangled_port_->post_message_task_container_
+ entangled_port->post_message_task_container_
->GetAndDecrementPostMessageTask(message.parent_task_id);
task_attribution_scope = tracker->CreateTaskScope(
script_state, parent_task,
diff --git a/third_party/blink/renderer/core/messaging/message_port.h b/third_party/blink/renderer/core/messaging/message_port.h
index 98ed58a9a765f5101d9b421507bf25db4359d7e5..a178d15c11b1e5fb1ff74d182021fe39e9d9b107 100644
--- a/third_party/blink/renderer/core/messaging/message_port.h
+++ b/third_party/blink/renderer/core/messaging/message_port.h
@@ -195,7 +195,7 @@ class CORE_EXPORT MessagePort : public EventTarget,
// The entangled port. Only set on initial entanglement, and gets unset as
// soon as the ports are disentangled.
- Member<MessagePort> initially_entangled_port_;
+ WeakMember<MessagePort> initially_entangled_port_;
Member<PostMessageTaskContainer> post_message_task_container_;
};

6
spec/api-ipc-spec.ts
View file

@ -317,11 +317,7 @@ describe('ipc module', () => {
await once(ipcMain, 'closed');
});
// TODO(@vertedinde): This broke upstream in CL https://chromium-review.googlesource.com/c/chromium/src/+/4831380
// The behavior seems to be an intentional change, we need to either A) implement the task_container_ model in
// our renderer message ports or B) patch how we handle renderer message ports being garbage collected
// crbug: https://bugs.chromium.org/p/chromium/issues/detail?id=1487835
it.skip('is emitted when the other end of a port is garbage-collected', async () => {
it('is emitted when the other end of a port is garbage-collected', async () => {
const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, contextIsolation: false } });
w.loadURL('about:blank');
await w.webContents.executeJavaScript(`(${async function () {