From fe6870e03d07b9bdf52df972d3b15214e3ce30eb Mon Sep 17 00:00:00 2001 From: Ari Young Date: Wed, 4 Sep 2024 01:53:40 -0700 Subject: [PATCH] Remove reference cycle from WebExtensionAction. https://webkit.org/b/278800 rdar://134866275 Reviewed by Timothy Hatcher. This patch removes a reference cycle previously present in WebExtensionAction. Since actions are stored in a WeakHashMap within WebExtensionContext where the key is either a WebExtensionWindow or a WebExtensionTab, and since each action would store a strong reference to whatever window or tab was used as its key, this forms a reference cycle. This fix changes the m_tab and m_window instance variables on WebExtensionAction to be of type std::optional>. This patch also contains supporting changes within the class to handle this new type. * Source/WebKit/UIProcess/API/Cocoa/WKWebExtensionAction.mm: (-[WKWebExtensionAction associatedTab]): Take reference to an action's tab in a RefPtr rather than a raw pointer to prevent it dropping out while in use. * Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionActionCocoa.mm: (WebKit::WebExtensionAction::tab const): Made method const, moved implementation to .mm file, updated to handle optional member variable. (WebKit::WebExtensionAction::window const): Made method const, moved implementation to .mm file, updated to handle optional member variable. (WebKit::WebExtensionAction::fallbackAction const): Updated to handle new type of m_tab (optional>) (WebKit::WebExtensionAction::platformMenuItems const): Use tab getter rather than member variable * Source/WebKit/UIProcess/Extensions/WebExtensionAction.h: (WebKit::WebExtensionAction::tab): Deleted (moved implementation to WebExtensionActionCocoa.mm). (WebKit::WebExtensionAction::window): Deleted (moved implementation to WebExtensionActionCocoa.mm). Canonical link: https://commits.webkit.org/283143@main --- .../API/Cocoa/WKWebExtensionAction.mm | 2 +- .../Cocoa/WebExtensionActionCocoa.mm | 32 ++++++++++++++----- .../UIProcess/Extensions/WebExtensionAction.h | 8 ++--- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/Source/WebKit/UIProcess/API/Cocoa/WKWebExtensionAction.mm b/Source/WebKit/UIProcess/API/Cocoa/WKWebExtensionAction.mm index a5bf029035633..a34dc213424c6 100644 --- a/Source/WebKit/UIProcess/API/Cocoa/WKWebExtensionAction.mm +++ b/Source/WebKit/UIProcess/API/Cocoa/WKWebExtensionAction.mm @@ -72,7 +72,7 @@ - (WKWebExtensionContext *)webExtensionContext - (id)associatedTab { - if (auto *tab = _webExtensionAction->tab()) + if (RefPtr tab = _webExtensionAction->tab()) return tab->delegate(); return nil; } diff --git a/Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionActionCocoa.mm b/Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionActionCocoa.mm index c481e509f5f44..cdb2082434870 100644 --- a/Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionActionCocoa.mm +++ b/Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionActionCocoa.mm @@ -589,6 +589,20 @@ - (void)_otherPopoverWillShow:(NSNotification *)notification return m_extensionContext.get(); } +RefPtr WebExtensionAction::tab() const +{ + return m_tab.and_then([](auto const& maybeTab) { + return std::optional(RefPtr(maybeTab.get())); + }).value_or(nullptr); +} + +RefPtr WebExtensionAction::window() const +{ + return m_window.and_then([](auto const& maybeWindow) { + return std::optional(RefPtr(maybeWindow.get())); + }).value_or(nullptr); +} + void WebExtensionAction::clearCustomizations() { if (!m_customIcons && m_customPopupPath.isNull() && m_customLabel.isNull() && m_customBadgeText.isNull() && !m_customEnabled && !m_blockedResourceCount) @@ -628,12 +642,12 @@ - (void)_otherPopoverWillShow:(NSNotification *)notification if (!extensionContext()) return nullptr; - // Tab actions fallback to the window action. - if (m_tab) - return extensionContext()->getAction(m_tab->window().get()).ptr(); + // Tab actions whose tab references have not dropped fallback to the window action. + if (RefPtr tab = this->tab()) + return extensionContext()->getAction(tab->window().get()).ptr(); - // Window actions fallback to the default action. - if (m_window) + // Window actions and tab actions whose tab references have dropped fallback to the default action. + if (m_window.has_value() || m_tab.has_value()) return &extensionContext()->defaultAction(); // Default actions have no fallback. @@ -1160,9 +1174,11 @@ - (void)_otherPopoverWillShow:(NSNotification *)notification if (!extensionContext()) return @[ ]; - RefPtr tab = m_tab; - if (!tab && m_window) - tab = m_window->activeTab(); + RefPtr tab = this->tab(); + if (!tab) { + if (RefPtr window = this->window()) + tab = window->activeTab(); + } WebExtensionMenuItemContextParameters contextParameters; contextParameters.types = WebExtensionMenuItemContextType::Action; diff --git a/Source/WebKit/UIProcess/Extensions/WebExtensionAction.h b/Source/WebKit/UIProcess/Extensions/WebExtensionAction.h index ed89c8fafe212..ddbedca956265 100644 --- a/Source/WebKit/UIProcess/Extensions/WebExtensionAction.h +++ b/Source/WebKit/UIProcess/Extensions/WebExtensionAction.h @@ -78,8 +78,8 @@ class WebExtensionAction : public API::ObjectImpl tab() const; + RefPtr window() const; void clearCustomizations(); void clearBlockedResourceCount(); @@ -149,8 +149,8 @@ class WebExtensionAction : public API::ObjectImpl m_extensionContext; - RefPtr m_tab; - RefPtr m_window; + std::optional> m_tab; + std::optional> m_window; #if PLATFORM(IOS_FAMILY) RetainPtr<_WKWebExtensionActionViewController> m_popupViewController;