Skip to content

Commit

Permalink
Remove reference cycle from WebExtensionAction.
Browse files Browse the repository at this point in the history
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<WeakPtr<..>>. 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<WeakPtr<..>>)
(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
  • Loading branch information
AriYoung00 authored and xeenon committed Sep 4, 2024
1 parent e7b28d6 commit fe6870e
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 13 deletions.
2 changes: 1 addition & 1 deletion Source/WebKit/UIProcess/API/Cocoa/WKWebExtensionAction.mm
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ - (WKWebExtensionContext *)webExtensionContext

- (id<WKWebExtensionTab>)associatedTab
{
if (auto *tab = _webExtensionAction->tab())
if (RefPtr tab = _webExtensionAction->tab())
return tab->delegate();
return nil;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,20 @@ - (void)_otherPopoverWillShow:(NSNotification *)notification
return m_extensionContext.get();
}

RefPtr<WebExtensionTab> WebExtensionAction::tab() const
{
return m_tab.and_then([](auto const& maybeTab) {
return std::optional(RefPtr(maybeTab.get()));
}).value_or(nullptr);
}

RefPtr<WebExtensionWindow> 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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down
8 changes: 4 additions & 4 deletions Source/WebKit/UIProcess/Extensions/WebExtensionAction.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ class WebExtensionAction : public API::ObjectImpl<API::Object::Type::WebExtensio
bool operator==(const WebExtensionAction&) const;

WebExtensionContext* extensionContext() const;
WebExtensionTab* tab() { return m_tab.get(); }
WebExtensionWindow* window() { return m_window.get(); }
RefPtr<WebExtensionTab> tab() const;
RefPtr<WebExtensionWindow> window() const;

void clearCustomizations();
void clearBlockedResourceCount();
Expand Down Expand Up @@ -149,8 +149,8 @@ class WebExtensionAction : public API::ObjectImpl<API::Object::Type::WebExtensio
#endif

WeakPtr<WebExtensionContext> m_extensionContext;
RefPtr<WebExtensionTab> m_tab;
RefPtr<WebExtensionWindow> m_window;
std::optional<WeakPtr<WebExtensionTab>> m_tab;
std::optional<WeakPtr<WebExtensionWindow>> m_window;

#if PLATFORM(IOS_FAMILY)
RetainPtr<_WKWebExtensionActionViewController> m_popupViewController;
Expand Down

0 comments on commit fe6870e

Please sign in to comment.