Skip to content

Commit

Permalink
Support non-persistent background pages in Web Extensions.
Browse files Browse the repository at this point in the history
https://webkit.org/b/246483
rdar://problem/114823336

Reviewed by Brian Weinstein.

Fixes a number of things related to non-persistent pages. The main one was that all current uses
of wakeUpBackgroundContentIfNecessaryToFireEvents() were not capturing the lambda variables correctly.
This was mostly fine since the lambda was almost always called immediately, but not it will be stored
and called later when the page needs woken back up. Changed all lambdas to protect the context, and
copy their simple parameters and use Ref / RefPtr for counted objects.

The next issue was that ports were not being disconnected when the extension pages went away, leaving
a dangling port that the other pages could still message to no avail. We now disconnect ports for any
page when it is removed from the WebExtensionController, which is when WebPageProxy is deallocated.
This required having the UI process track what ports were open for which page.

And third, the runtime.connect() and runtime.sendMessage() implementations were not waking the background.
This was fine before since it was almost always guaranteed to exist (but likely a race condition). Now
we wait for the page to load before trying to deliver the messages.

Currently almost all tests use non-persistent pages, and this change is well exercised with the tests.
When using the TestWebKitAPI runner, the timeouts for the background page is shortened to 3 seconds, vs
the typical 30 seconds. And open and active ports is 6 seconds, vs the typical 2 minutes. Most tests will
only wake the background page once, but some do run long enough to have it terminated and get relaunched.

* Source/WTF/wtf/HashCountedSet.h:
(WTF::Traits>::isValidValue): Added. The only impl passing off to HashMap was dead code, and
was removed from HashMap. Add back an implemetation copied from HashSet.
* Source/WebKit/Platform/IPC/ArgumentCoders.h: Fix return to be std::nullopt. This was also dead
code that is now being hit since we are using it again.
* Source/WebKit/Scripts/webkit/messages.py:
(class_template_headers): Added HashCountedSet.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIActionCocoa.mm:
(WebKit::WebExtensionContext::fireActionClickedEventIfNeeded): Fix lambda to properly capture.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIAlarmsCocoa.mm:
(WebKit::WebExtensionContext::fireAlarmsEventIfNeeded): Ditto.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPICommandsCocoa.mm:
(WebKit::WebExtensionContext::fireCommandEventIfNeeded): Ditto.
(WebKit::WebExtensionContext::fireCommandChangedEventIfNeeded): Ditto.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPICookiesCocoa.mm:
(WebKit::WebExtensionContext::fireCookiesChangedEventIfNeeded): Ditto.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIEventCocoa.mm:
(WebKit::WebExtensionContext::addListener): Use isBackgroundPage() helper.
(WebKit::WebExtensionContext::removeListener): Ditto.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIMenusCocoa.mm:
(WebKit::WebExtensionContext::fireMenusClickedEventIfNeeded): Fix lambda to properly capture.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIPermissionsCocoa.mm:
(WebKit::WebExtensionContext::firePermissionsEventListenerIfNecessary): Ditto.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIPortCocoa.mm:
(WebKit::WebExtensionContext::portPostMessage): Track last time background used port. Don't use
wakeUpBackgroundContentIfNecessaryToFireEvents(), since the page needs loaded for ports to be
active and receive messages.
(WebKit::WebExtensionContext::portRemoved): Added WebPageProxyIdentifier param.
(WebKit::WebExtensionContext::pageHasOpenPorts): Added.
(WebKit::WebExtensionContext::disconnectPortsForPage): Added.
(WebKit::WebExtensionContext::addPorts): Added HashCountedSet param instead of simple count.
(WebKit::WebExtensionContext::removePort): Added WebPageProxyIdentifier param.
(WebKit::WebExtensionContext::addNativePort): Don't use addPorts() now, since that requires a page.
(WebKit::WebExtensionContext::removeNativePort): Ditto for removePort().
(WebKit::WebExtensionContext::firePortDisconnectEventIfNeeded): Don't wake up the background,
since the page needs loaded for ports to be active and receive messages.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIRuntimeCocoa.mm:
(WebKit::WebExtensionContext::runtimeSendMessage): This was missing the wake up the background call.
(WebKit::WebExtensionContext::runtimeConnect): Ditto.
(WebKit::WebExtensionContext::runtimeConnectNative): Track added ports by page.
(WebKit::WebExtensionContext::runtimeWebPageConnect): Ditto.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIStorageCocoa.mm:
(WebKit::WebExtensionContext::fireStorageChangedEventIfNeeded): Fix lambda to properly capture.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPITabsCocoa.mm:
(WebKit::WebExtensionContext::tabsConnect): Track added ports by page.
(WebKit::WebExtensionContext::fireTabsCreatedEventIfNeeded): Fix lambda to properly capture.
(WebKit::WebExtensionContext::fireTabsUpdatedEventIfNeeded): Ditto.
(WebKit::WebExtensionContext::fireTabsReplacedEventIfNeeded): Ditto.
(WebKit::WebExtensionContext::fireTabsDetachedEventIfNeeded): Ditto.
(WebKit::WebExtensionContext::fireTabsMovedEventIfNeeded): Ditto.
(WebKit::WebExtensionContext::fireTabsAttachedEventIfNeeded): Ditto.
(WebKit::WebExtensionContext::fireTabsActivatedEventIfNeeded): Ditto.
(WebKit::WebExtensionContext::fireTabsHighlightedEventIfNeeded): Ditto.
(WebKit::WebExtensionContext::fireTabsRemovedEventIfNeeded): Ditto.
* Source/WebKit/UIProcess/Extensions/Cocoa/API/WebExtensionContextAPIWindowsCocoa.mm:
(WebKit::WebExtensionContext::fireWindowsEventIfNeeded): Ditto.
* Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionContextCocoa.mm:
(WebKit::WebExtensionContext::removePage): Added. Disconnect ports.
(WebKit::WebExtensionContext::getCurrentTab const): Use isBackgroundPage() helper.
(WebKit::WebExtensionContext::didChangeTabProperties): Return early in lambda for !isLoaded().
(WebKit::WebExtensionContext::didStartProvisionalLoadForFrame): Fix lambda to properly capture.
(WebKit::WebExtensionContext::didCommitLoadForFrame): Ditto.
(WebKit::WebExtensionContext::didFinishLoadForFrame): Ditto.
(WebKit::WebExtensionContext::didFailLoadForFrame): Ditto.
(WebKit::WebExtensionContext::resourceLoadDidSendRequest): Fix lambda to properly capture. Also
fetch the window outside the lambda so it is known before time passes and the tab might move windows.
(WebKit::WebExtensionContext::resourceLoadDidPerformHTTPRedirection): Ditto.
(WebKit::WebExtensionContext::resourceLoadDidReceiveChallenge): Ditto.
(WebKit::WebExtensionContext::resourceLoadDidReceiveResponse): Ditto.
(WebKit::WebExtensionContext::resourceLoadDidCompleteWithError): Ditto.
(WebKit::WebExtensionContext::relatedWebView): Skip over Inspector pages, they have different process pools.
(WebKit::WebExtensionContext::isBackgroundPage const): Added.
(WebKit::WebExtensionContext::backgroundContentIsLoaded const): Added.
(WebKit::WebExtensionContext::loadBackgroundWebViewIfNeeded): Added.
(WebKit::WebExtensionContext::loadBackgroundWebView): Added logging. Remove unused m_lastBackgroundContentLoadDate.
(WebKit::WebExtensionContext::unloadBackgroundWebView):
(WebKit::isNotRunningInTestRunner): Added.
(WebKit::WebExtensionContext::scheduleBackgroundContentToUnload): Removed FIXME and schedule a timer.
(WebKit::WebExtensionContext::unloadBackgroundContentIfPossible): Added.
(WebKit::WebExtensionContext::performTasksAfterBackgroundContentLoads): Shorten delay and added !isLoaded() early return.
(WebKit::WebExtensionContext::wakeUpBackgroundContentIfNecessary): Drastically simplified.
(WebKit::WebExtensionContext::wakeUpBackgroundContentIfNecessaryToFireEvents): Simplied. Uses wakeUpBackgroundContentIfNecessary().
(WebKit::WebExtensionContext::webViewWebContentProcessDidTerminate): Only reload background page if persistent. Removed FIXME
since that is handled by removePage() now.
(WebKit::WebExtensionContext::processes const): Return early if !isLoaded().
(WebKit::WebExtensionContext::queueEventToFireAfterBackgroundContentLoads): Deleted.
* Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionControllerCocoa.mm:
(WebKit::WebExtensionController::removePage): Call removePage() on each context.
* Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionMessagePortCocoa.mm:
(WebKit::WebExtensionMessagePort::remove): Use firePortDisconnectEventIfNeeded() instead of page sentic portRemoved().
* Source/WebKit/UIProcess/Extensions/WebExtensionContext.cpp:
(WebKit::WebExtensionContext::pageListensForEvent const): Return early if !isLoaded().
(WebKit::WebExtensionContext::processes const): Ditto.
* Source/WebKit/UIProcess/Extensions/WebExtensionContext.h:
(WebKit::WebExtensionContext::sendToProcesses const): Return early if !isLoaded().
* Source/WebKit/UIProcess/Extensions/WebExtensionContext.messages.in:
* Source/WebKit/WebProcess/Extensions/API/Cocoa/WebExtensionAPIPortCocoa.mm:
(WebKit::WebExtensionAPIPort::remove): Padd owningPageProxyIdentifier() in the message.
* Source/WebKit/WebProcess/Extensions/API/Cocoa/WebExtensionAPIRuntimeCocoa.mm:
(WebKit::WebExtensionAPIRuntime::connectNative): Pass webPageProxyIdentifier() in the message.
(WebKit::WebExtensionContextProxy::internalDispatchRuntimeConnectEvent): Use a HashCountedSet to track page port counts.
(WebKit::WebExtensionContextProxy::dispatchRuntimeConnectEvent): Use HashCountedSet.
* Source/WebKit/WebProcess/Extensions/WebExtensionContextProxy.h:
* Source/WebKit/WebProcess/Extensions/WebExtensionContextProxy.messages.in:

Canonical link: https://commits.webkit.org/275819@main
  • Loading branch information
xeenon committed Mar 8, 2024
1 parent 347b11e commit cc9d7c9
Show file tree
Hide file tree
Showing 26 changed files with 515 additions and 266 deletions.
19 changes: 18 additions & 1 deletion Source/WTF/wtf/HashCountedSet.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class HashCountedSet final {
template<typename V = ValueType> typename std::enable_if<IsSmartPtr<V>::value && !IsSmartPtr<V>::isNullable, unsigned>::type count(std::add_const_t<typename GetPtrHelper<V>::UnderlyingType>&) const;
template<typename V = ValueType> typename std::enable_if<IsSmartPtr<V>::value && !IsSmartPtr<V>::isNullable, bool>::type remove(std::add_const_t<typename GetPtrHelper<V>::UnderlyingType>&);

static bool isValidValue(const ValueType& value) { return ImplType::isValidValue(value); }
static bool isValidValue(const ValueType&);

private:
ImplType m_impl;
Expand Down Expand Up @@ -286,6 +286,23 @@ inline void HashCountedSet<Value, HashFunctions, Traits>::clear()
m_impl.clear();
}

template<typename Value, typename HashFunctions, typename Traits>
inline bool HashCountedSet<Value, HashFunctions, Traits>::isValidValue(const ValueType& value)
{
if (Traits::isDeletedValue(value))
return false;

if (HashFunctions::safeToCompareToEmptyOrDeleted) {
if (value == Traits::emptyValue())
return false;
} else {
if (isHashTraitsEmptyValue<Traits>(value))
return false;
}

return true;
}

template<typename Value, typename HashFunctions, typename Traits>
template<typename V>
inline auto HashCountedSet<Value, HashFunctions, Traits>::find(std::add_const_t<typename GetPtrHelper<V>::UnderlyingType>* value) -> typename std::enable_if<IsSmartPtr<V>::value, iterator>::type
Expand Down
2 changes: 1 addition & 1 deletion Source/WebKit/Platform/IPC/ArgumentCoders.h
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ template<typename KeyArg, typename HashArg, typename KeyTraitsArg> struct Argume
{
unsigned hashCountedSetSize;
if (!decoder.decode(hashCountedSetSize))
return false;
return std::nullopt;

HashCountedSetType tempHashCountedSet;
for (unsigned i = 0; i < hashCountedSetSize; ++i) {
Expand Down
1 change: 1 addition & 0 deletions Source/WebKit/Scripts/webkit/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,7 @@ def class_template_headers(template_string):
class_template_types = {
'WebCore::RectEdges': {'headers': ['<WebCore/RectEdges.h>'], 'argument_coder_headers': ['"ArgumentCoders.h"']},
'Expected': {'headers': ['<wtf/Expected.h>'], 'argument_coder_headers': ['"ArgumentCoders.h"']},
'HashCountedSet': {'headers': ['<wtf/HashCountedSet.h>'], 'argument_coder_headers': ['"ArgumentCoders.h"']},
'HashMap': {'headers': ['<wtf/HashMap.h>'], 'argument_coder_headers': ['"ArgumentCoders.h"']},
'HashSet': {'headers': ['<wtf/HashSet.h>'], 'argument_coder_headers': ['"ArgumentCoders.h"']},
'KeyValuePair': {'headers': ['<wtf/KeyValuePair.h>'], 'argument_coder_headers': ['"ArgumentCoders.h"']},
Expand Down
10 changes: 10 additions & 0 deletions Source/WebKit/Shared/Extensions/WebExtensionContentWorldType.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,16 @@ enum class WebExtensionContentWorldType : uint8_t {
#endif
};

inline bool isEqual(WebExtensionContentWorldType a, WebExtensionContentWorldType b)
{
#if ENABLE(INSPECTOR_EXTENSIONS)
// Inspector content world is a special alias of Main. Consider them equal.
if ((a == WebExtensionContentWorldType::Main || a == WebExtensionContentWorldType::Inspector) && (b == WebExtensionContentWorldType::Main || b == WebExtensionContentWorldType::Inspector))
return true;
#endif
return a == b;
}

inline String toDebugString(WebExtensionContentWorldType contentWorldType)
{
switch (contentWorldType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@
void WebExtensionContext::fireActionClickedEventIfNeeded(WebExtensionTab* tab)
{
constexpr auto type = WebExtensionEventListenerType::ActionOnClicked;
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [&] {
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [=, protectedThis = Ref { *this }, tab = RefPtr { tab }] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchActionClickedEvent(tab ? std::optional(tab->parameters()) : std::nullopt));
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@
void WebExtensionContext::fireAlarmsEventIfNeeded(const WebExtensionAlarm& alarm)
{
constexpr auto type = WebExtensionEventListenerType::AlarmsOnAlarm;
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [&] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchAlarmsEvent(alarm.parameters()));
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [=, protectedThis = Ref { *this }, alarm = Ref { alarm }] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchAlarmsEvent(alarm->parameters()));
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,16 @@
void WebExtensionContext::fireCommandEventIfNeeded(const WebExtensionCommand& command, WebExtensionTab* tab)
{
constexpr auto type = WebExtensionEventListenerType::CommandsOnCommand;
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [&] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchCommandsCommandEvent(command.identifier(), tab ? std::optional(tab->parameters()) : std::nullopt));
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [=, protectedThis = Ref { *this }, command = Ref { command }, tab = RefPtr { tab }] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchCommandsCommandEvent(command->identifier(), tab ? std::optional(tab->parameters()) : std::nullopt));
});
}

void WebExtensionContext::fireCommandChangedEventIfNeeded(const WebExtensionCommand& command, const String& oldShortcut)
{
constexpr auto type = WebExtensionEventListenerType::CommandsOnChanged;
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [&] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchCommandsChangedEvent(command.identifier(), oldShortcut, command.shortcutString()));
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [=, protectedThis = Ref { *this }, command = Ref { command }] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchCommandsChangedEvent(command->identifier(), oldShortcut, command->shortcutString()));
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ static inline URL toURL(const WebCore::Cookie& cookie)
void WebExtensionContext::fireCookiesChangedEventIfNeeded()
{
constexpr auto type = WebExtensionEventListenerType::CookiesOnChanged;
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [&] {
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [=, protectedThis = Ref { *this }] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchCookiesChangedEvent());
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@

RELEASE_LOG_DEBUG(Extensions, "Registered event listener for type %{public}hhu in %{public}@ world", enumToUnderlyingType(type), (NSString *)toDebugString(contentWorldType));

if (!extension().backgroundContentIsPersistent() && m_backgroundWebView && m_backgroundWebView.get()._page->identifier() == identifier)
if (!extension().backgroundContentIsPersistent() && isBackgroundPage(identifier))
m_backgroundContentEventListeners.add(type);

auto result = m_eventListenerPages.add({ type, contentWorldType }, WeakPageCountedSet { });
Expand All @@ -66,7 +66,7 @@

RELEASE_LOG_DEBUG(Extensions, "Unregistered %{public}zu event listener(s) for type %{public}hhu in %{public}@ world", removedCount, enumToUnderlyingType(type), (NSString *)toDebugString(contentWorldType));

if (!extension().backgroundContentIsPersistent() && m_backgroundWebView && m_backgroundWebView.get()._page->identifier() == identifier) {
if (!extension().backgroundContentIsPersistent() && isBackgroundPage(identifier)) {
for (size_t i = 0; i < removedCount; ++i)
m_backgroundContentEventListeners.remove(type);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,8 @@ static bool isAncestorOrSelf(WebExtensionContext& context, const String& potenti
RefPtr tab = contextParameters.tabIdentifier ? getTab(contextParameters.tabIdentifier.value()) : nullptr;

constexpr auto type = WebExtensionEventListenerType::MenusOnClicked;
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [&] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchMenusClickedEvent(menuItem.minimalParameters(), wasChecked, contextParameters, tab ? std::optional { tab->parameters() } : std::nullopt));
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [=, protectedThis = Ref { *this }, menuItem = Ref { menuItem }] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchMenusClickedEvent(menuItem->minimalParameters(), wasChecked, contextParameters, tab ? std::optional { tab->parameters() } : std::nullopt));
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@

HashSet<String> origins = toStrings(matchPatterns);

wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [&] {
wakeUpBackgroundContentIfNecessaryToFireEvents({ type }, [=, protectedThis = Ref { *this }] {
sendToProcessesForEvent(type, Messages::WebExtensionContextProxy::DispatchPermissionsEvent(type, permissions, origins));
});
}
Expand Down
Loading

0 comments on commit cc9d7c9

Please sign in to comment.