Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure MessagePorts get GCed when not referenced #40201

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -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
@@ -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 changes: 1 addition & 5 deletions spec/api-ipc-spec.ts
Expand Up @@ -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 () {
Expand Down