Skip to content

Commit 76527da

Browse files
cdumezcdumez
committed
SharedWorkerGlobalScope.close() does not then allow the SharedWorker to be replaced
https://bugs.webkit.org/show_bug.cgi?id=250839 rdar://problem/104481929 Reviewed by Youenn Fablet. Shared workers are usually terminated once all their clients go away and the NetworkProcess is in charge of this. However, the shared worker can also choose to terminate itself by calling `self.close()`. In this case, we were failing to notify the network process that the shared worker had terminated. As a result, when a shared worker connection request would come in later on, the network process would still direct it towards the existing (now terminated) shared worker. To address this issue, we now IPC back from the shared worker process to the network process when a shared worker gets terminated. This allows the network process state to remain up-to-date and to make more accurate decisions. However, doing only this would still be flaky since there can be a race between the NetworkProcess receiving the IPC about a shared worker being terminated and a new connection request received by the NetworkProcess. To address this, I now added a CompletionHandler to the PostConnectEvent IPC from the NetworkProcess to the SharedWorker process, indicating if we managed to connect this SharedWorker object to the existing SharedWorker context. In case of failure, we clean the state in the NetworkProcess and re-attempt the connection, which will relaunch a SharedWorker. * LayoutTests/http/wpt/shared-workers/relaunch-after-close-expected.txt: Added. * LayoutTests/http/wpt/shared-workers/relaunch-after-close.html: Added. * LayoutTests/http/wpt/shared-workers/resources/relaunch-after-close-page2.html: Added. * LayoutTests/http/wpt/shared-workers/resources/relaunch-after-close-sharedworker.js: Added. (shutdown): (broadcast): (sendMessageToPortRef): * LayoutTests/http/wpt/shared-workers/resources/relaunch-after-close-window.js: Added. (testComplete): (async beginTestForPage1): (async beginTestForPage2): (async initSharedWorker): (disonnectFromSharedWorker): (async tellSharedWorkerToShutdown): (log): (deferredWithTimeout): (async wait): * Source/WebCore/workers/shared/context/SharedWorkerContextManager.cpp: (WebCore::SharedWorkerContextManager::stopSharedWorker): * Source/WebCore/workers/shared/context/SharedWorkerContextManager.h: * Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorkerServer.cpp: (WebKit::WebSharedWorkerServer::sharedWorkerTerminated): * Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorkerServer.h: * Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorkerServerToContextConnection.cpp: (WebKit::WebSharedWorkerServerToContextConnection::sharedWorkerTerminated): * Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorkerServerToContextConnection.h: * Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorkerServerToContextConnection.messages.in: * Source/WebKit/WebProcess/Storage/WebSharedWorkerContextManagerConnection.cpp: (WebKit::WebSharedWorkerContextManagerConnection::sharedWorkerTerminated): * Source/WebKit/WebProcess/Storage/WebSharedWorkerContextManagerConnection.h: Canonical link: https://commits.webkit.org/259228@main
1 parent 487c9df commit 76527da

15 files changed

+306
-10
lines changed

‎LayoutTests/http/wpt/shared-workers/relaunch-after-close-expected.txt‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
CONSOLE MESSAGE: PAGE 2: Attempting to boot the shared worker
2+
CONSOLE MESSAGE: PAGE 2: Received message from worker: 'connected'
3+
CONSOLE MESSAGE: PAGE 2: SharedWorker connected
4+
CONSOLE MESSAGE: PAGE 2: Telling the SharedWorker to shutdown
5+
CONSOLE MESSAGE: PAGE 2: Received message from worker: 'shutdown'
6+
CONSOLE MESSAGE: PAGE 2: SharedWorker successfully shutdown
7+
CONSOLE MESSAGE: PAGE 2: Attempting to boot the shared worker
8+
CONSOLE MESSAGE: PAGE 2: Received message from worker: 'connected'
9+
CONSOLE MESSAGE: PAGE 2: SharedWorker connected
10+
CONSOLE MESSAGE: PAGE 2: Telling the SharedWorker to shutdown
11+
CONSOLE MESSAGE: PAGE 2: Received message from worker: 'shutdown'
12+
CONSOLE MESSAGE: PAGE 2: SharedWorker successfully shutdown
13+
CONSOLE MESSAGE: PAGE 2: Attempting to boot the shared worker
14+
CONSOLE MESSAGE: PAGE 2: Received message from worker: 'connected'
15+
CONSOLE MESSAGE: PAGE 2: SharedWorker connected
16+
CONSOLE MESSAGE: PAGE 2: Telling the SharedWorker to shutdown
17+
CONSOLE MESSAGE: PAGE 2: Received message from worker: 'shutdown'
18+
CONSOLE MESSAGE: PAGE 2: SharedWorker successfully shutdown
19+
CONSOLE MESSAGE: PAGE 2: Attempting to boot the shared worker
20+
CONSOLE MESSAGE: PAGE 2: Received message from worker: 'connected'
21+
CONSOLE MESSAGE: PAGE 2: SharedWorker connected
22+
CONSOLE MESSAGE: PAGE 2: Telling the SharedWorker to shutdown
23+
CONSOLE MESSAGE: PAGE 2: Received message from worker: 'shutdown'
24+
CONSOLE MESSAGE: PAGE 2: SharedWorker successfully shutdown
25+
CONSOLE MESSAGE: PAGE 2: Attempting to boot the shared worker
26+
CONSOLE MESSAGE: PAGE 2: Received message from worker: 'connected'
27+
CONSOLE MESSAGE: PAGE 2: SharedWorker connected
28+
CONSOLE MESSAGE: PAGE 2: PASS: Successfully shutdown and reconstructed the SharedWorker many times
29+
30+
PASS Shared worker should be able to relaunch after calling SharedWorkerGlobalScope.close()
31+

‎LayoutTests/http/wpt/shared-workers/relaunch-after-close.html‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<html>
2+
<head>
3+
<script src="/resources/testharness.js"></script>
4+
<script src="/resources/testharnessreport.js"></script>
5+
<script src="resources/relaunch-after-close-window.js"></script>
6+
</head>
7+
<body>
8+
<script>
9+
10+
promise_test(async (t) => {
11+
const worker = new SharedWorker('resources/relaunch-after-close-sharedworker.js');
12+
13+
await beginTestForPage1();
14+
w = open("resources/relaunch-after-close-page2.html");
15+
16+
return new Promise((resolve, reject) => {
17+
window.addEventListener("result", (e) => {
18+
assert_equals(e.data, "PASS: Successfully shutdown and reconstructed the SharedWorker many times");
19+
resolve();
20+
});
21+
});
22+
}, "Shared worker should be able to relaunch after calling SharedWorkerGlobalScope.close()");
23+
</script>
24+
</body>
25+
</html>

‎LayoutTests/http/wpt/shared-workers/resources/relaunch-after-close-page2.html‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<script src="relaunch-after-close-window.js"></script>
5+
</head>
6+
<body>
7+
<script>
8+
onload = () => {
9+
beginTestForPage2();
10+
};
11+
</script>
12+
</body>
13+
</html>

‎LayoutTests/http/wpt/shared-workers/resources/relaunch-after-close-sharedworker.js‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
const connections = new Set()
2+
3+
self.onconnect = e => {
4+
const port = e.ports[0]
5+
const ref = new WeakRef(port)
6+
7+
port.onmessage = e => {
8+
if (e.data === 'shutdown') {
9+
shutdown()
10+
}
11+
}
12+
13+
connections.add(ref)
14+
port.postMessage('connected')
15+
}
16+
17+
function shutdown() {
18+
broadcast('shutdown')
19+
20+
try {
21+
for (const ref of connections) {
22+
const port = ref.deref()
23+
24+
if (port) {
25+
try {
26+
port.onmessage = null
27+
port.close()
28+
} catch { }
29+
}
30+
}
31+
} finally {
32+
self.close()
33+
}
34+
}
35+
36+
function broadcast(msg) {
37+
for (const ref of connections) {
38+
sendMessageToPortRef(ref, msg)
39+
}
40+
}
41+
42+
function sendMessageToPortRef(ref, msg) {
43+
const port = ref.deref()
44+
let failed = false
45+
46+
try {
47+
port?.postMessage(msg)
48+
} catch {
49+
failed = true
50+
}
51+
52+
if (!port || failed) {
53+
connections.delete(ref)
54+
}
55+
}
56+

‎LayoutTests/http/wpt/shared-workers/resources/relaunch-after-close-window.js‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
let sharedWorker
2+
let count = 0
3+
const messageLogger = e => { log(` Received message from worker: '${e.data}'`) }
4+
5+
function testComplete(result) {
6+
opener.dispatchEvent(new MessageEvent("result", { data: result }))
7+
}
8+
9+
// TESTS
10+
async function beginTestForPage1() {
11+
if (!await initSharedWorker('resources/relaunch-after-close-sharedworker.js')) { return }
12+
13+
// Prepare for when Page 2 will shutdown the SharedWorker
14+
const shutdownListener = e => {
15+
if (e.data === 'shutdown') {
16+
sharedWorker.port.removeEventListener('message', shutdownListener)
17+
disonnectFromSharedWorker()
18+
}
19+
}
20+
21+
sharedWorker.port.addEventListener('message', shutdownListener)
22+
}
23+
24+
async function beginTestForPage2() {
25+
if (!await initSharedWorker('relaunch-after-close-sharedworker.js')) { return }
26+
27+
while (count < 5) {
28+
if (!await tellSharedWorkerToShutdown()) { return }
29+
// NOTE: 1000 is arbitrary
30+
await wait(500)
31+
if (!await initSharedWorker('relaunch-after-close-sharedworker.js')) { return }
32+
}
33+
34+
log('PASS: Successfully shutdown and reconstructed the SharedWorker many times')
35+
testComplete("PASS: Successfully shutdown and reconstructed the SharedWorker many times")
36+
}
37+
38+
// UTILITY FUNCTIONS
39+
async function initSharedWorker(workerURL) {
40+
count += 1
41+
log(' Attempting to boot the shared worker')
42+
43+
const def = deferredWithTimeout(3000)
44+
45+
sharedWorker = new SharedWorker(workerURL)
46+
47+
const connectedListener = e => {
48+
if (e.data === 'connected') {
49+
def.resolve()
50+
}
51+
}
52+
53+
sharedWorker.port.addEventListener('message', messageLogger)
54+
sharedWorker.port.addEventListener('message', connectedListener)
55+
sharedWorker.port.start()
56+
57+
try {
58+
await def.promise
59+
log(' SharedWorker connected')
60+
return true
61+
} catch {
62+
log('FAIL: SharedWorker failed to connect')
63+
if (window.opener)
64+
testComplete("FAIL: SharedWorker failed to connect")
65+
return false
66+
} finally {
67+
sharedWorker.port.removeEventListener('message', connectedListener)
68+
}
69+
}
70+
71+
function disonnectFromSharedWorker() {
72+
sharedWorker.port.removeEventListener('message', messageLogger)
73+
sharedWorker.port.close()
74+
sharedWorker = null
75+
}
76+
77+
async function tellSharedWorkerToShutdown() {
78+
if (!sharedWorker) { return }
79+
80+
log(' Telling the SharedWorker to shutdown')
81+
82+
const def = deferredWithTimeout(3000)
83+
84+
const shutdownListener = e => {
85+
if (e.data === 'shutdown') {
86+
def.resolve()
87+
}
88+
}
89+
90+
sharedWorker.port.addEventListener('message', shutdownListener)
91+
sharedWorker.port.postMessage('shutdown')
92+
93+
try {
94+
await def.promise
95+
log(' SharedWorker successfully shutdown')
96+
return true
97+
} catch {
98+
log('FAIL: SharedWorker timed out during shutdown')
99+
if (window.opener)
100+
testComplete("FAIL: SharedWorker timed out during shutdown")
101+
return false
102+
} finally {
103+
sharedWorker.port.removeEventListener('message', shutdownListener)
104+
disonnectFromSharedWorker()
105+
}
106+
}
107+
108+
function log(msg) {
109+
if (opener)
110+
opener.console.log("PAGE 2: " + msg);
111+
}
112+
113+
function deferredWithTimeout(amount) {
114+
let resolve, reject
115+
116+
const promise = new Promise((re, rej) => {
117+
resolve = re
118+
reject = rej
119+
setTimeout(() => rej(new Error('timeout')), amount)
120+
})
121+
122+
return {
123+
resolve,
124+
reject,
125+
promise
126+
}
127+
}
128+
129+
async function wait(amount) {
130+
return await deferredWithTimeout(amount).promise.catch(() => {})
131+
}
132+

‎Source/WebCore/workers/shared/context/SharedWorkerContextManager.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,9 @@ void SharedWorkerContextManager::stopSharedWorker(SharedWorkerIdentifier sharedW
6262
// a race towards its destruction.
6363
callOnMainThread([worker = WTFMove(worker)] { });
6464
});
65+
66+
if (auto* connection = SharedWorkerContextManager::singleton().connection())
67+
connection->sharedWorkerTerminated(sharedWorkerIdentifier);
6568
}
6669

6770
void SharedWorkerContextManager::suspendSharedWorker(SharedWorkerIdentifier sharedWorkerIdentifier)
@@ -108,18 +111,19 @@ void SharedWorkerContextManager::registerSharedWorkerThread(Ref<SharedWorkerThre
108111
proxy->thread().start([](const String& /*exceptionMessage*/) { });
109112
}
110113

111-
void SharedWorkerContextManager::Connection::postConnectEvent(SharedWorkerIdentifier sharedWorkerIdentifier, TransferredMessagePort&& transferredPort, String&& sourceOrigin)
114+
void SharedWorkerContextManager::Connection::postConnectEvent(SharedWorkerIdentifier sharedWorkerIdentifier, TransferredMessagePort&& transferredPort, String&& sourceOrigin, CompletionHandler<void(bool)>&& completionHandler)
112115
{
113116
ASSERT(isMainThread());
114117
auto* proxy = SharedWorkerContextManager::singleton().sharedWorker(sharedWorkerIdentifier);
115118
RELEASE_LOG(SharedWorker, "SharedWorkerContextManager::Connection::postConnectEvent: sharedWorkerIdentifier=%" PRIu64 ", proxy=%p", sharedWorkerIdentifier.toUInt64(), proxy);
116119
if (!proxy)
117-
return;
120+
return completionHandler(false);
118121

119122
proxy->thread().runLoop().postTask([transferredPort = WTFMove(transferredPort), sourceOrigin = WTFMove(sourceOrigin).isolatedCopy()] (auto& scriptExecutionContext) mutable {
120123
ASSERT(!isMainThread());
121124
downcast<SharedWorkerGlobalScope>(scriptExecutionContext).postConnectEvent(WTFMove(transferredPort), WTFMove(sourceOrigin));
122125
});
126+
completionHandler(true);
123127
}
124128

125129
void SharedWorkerContextManager::Connection::terminateSharedWorker(SharedWorkerIdentifier sharedWorkerIdentifier)

‎Source/WebCore/workers/shared/context/SharedWorkerContextManager.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,13 +50,14 @@ class SharedWorkerContextManager {
5050
virtual ~Connection() { }
5151
virtual void establishConnection(CompletionHandler<void()>&&) = 0;
5252
virtual void postExceptionToWorkerObject(SharedWorkerIdentifier, const String& errorMessage, int lineNumber, int columnNumber, const String& sourceURL) = 0;
53+
virtual void sharedWorkerTerminated(SharedWorkerIdentifier) = 0;
5354
bool isClosed() const { return m_isClosed; }
5455

5556
protected:
5657
void setAsClosed() { m_isClosed = true; }
5758

5859
// IPC message handlers.
59-
WEBCORE_EXPORT void postConnectEvent(SharedWorkerIdentifier, TransferredMessagePort&&, String&& sourceOrigin);
60+
WEBCORE_EXPORT void postConnectEvent(SharedWorkerIdentifier, TransferredMessagePort&&, String&& sourceOrigin, CompletionHandler<void(bool)>&&);
6061
WEBCORE_EXPORT void terminateSharedWorker(SharedWorkerIdentifier);
6162
WEBCORE_EXPORT void suspendSharedWorker(SharedWorkerIdentifier);
6263
WEBCORE_EXPORT void resumeSharedWorker(SharedWorkerIdentifier);

‎Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorkerServer.cpp‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,17 @@ void WebSharedWorkerServer::requestSharedWorker(WebCore::SharedWorkerKey&& share
7878
if (sharedWorker->isRunning()) {
7979
auto* contextConnection = sharedWorker->contextConnection();
8080
ASSERT(contextConnection);
81-
if (contextConnection)
82-
contextConnection->postConnectEvent(*sharedWorker, port);
81+
if (contextConnection) {
82+
contextConnection->postConnectEvent(*sharedWorker, port, [this, weakThis = WeakPtr { *this }, sharedWorkerKey, sharedWorkerObjectIdentifier, sharedWorkerIdentifier = sharedWorker->identifier(), port, workerOptions](bool success) mutable {
83+
if (success || !weakThis)
84+
return;
85+
// We failed to connect to the existing shared worker, likely because it just terminated.
86+
RELEASE_LOG_ERROR(SharedWorker, "WebSharedWorkerServer::requestSharedWorker: Failed to connect to existing shared worker %" PRIu64 ", will create a new one instead.", sharedWorkerIdentifier.toUInt64());
87+
if (auto it = m_sharedWorkers.find(sharedWorkerKey); it != m_sharedWorkers.end() && it->value->identifier() == sharedWorkerIdentifier)
88+
m_sharedWorkers.remove(it);
89+
requestSharedWorker(WTFMove(sharedWorkerKey), sharedWorkerObjectIdentifier, WTFMove(port), WTFMove(workerOptions));
90+
});
91+
}
8392
}
8493
return;
8594
}
@@ -282,6 +291,14 @@ void WebSharedWorkerServer::postExceptionToWorkerObject(WebCore::SharedWorkerIde
282291
});
283292
}
284293

294+
void WebSharedWorkerServer::sharedWorkerTerminated(WebCore::SharedWorkerIdentifier sharedWorkerIdentifier)
295+
{
296+
RELEASE_LOG_ERROR(SharedWorker, "WebSharedWorkerServer::sharedWorkerTerminated: sharedWorkerIdentifier=%" PRIu64, sharedWorkerIdentifier.toUInt64());
297+
m_sharedWorkers.removeIf([sharedWorkerIdentifier] (auto& iterator) {
298+
return iterator.value->identifier() == sharedWorkerIdentifier;
299+
});
300+
}
301+
285302
void WebSharedWorkerServer::terminateContextConnectionWhenPossible(const WebCore::RegistrableDomain& registrableDomain, WebCore::ProcessIdentifier processIdentifier)
286303
{
287304
auto* contextConnection = contextConnectionForRegistrableDomain(registrableDomain);

‎Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorkerServer.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ class WebSharedWorkerServer : public CanMakeWeakPtr<WebSharedWorkerServer> {
6767
void suspendForBackForwardCache(const WebCore::SharedWorkerKey&, WebCore::SharedWorkerObjectIdentifier);
6868
void resumeForBackForwardCache(const WebCore::SharedWorkerKey&, WebCore::SharedWorkerObjectIdentifier);
6969
void postExceptionToWorkerObject(WebCore::SharedWorkerIdentifier, const String& errorMessage, int lineNumber, int columnNumber, const String& sourceURL);
70+
void sharedWorkerTerminated(WebCore::SharedWorkerIdentifier);
7071

7172
void terminateContextConnectionWhenPossible(const WebCore::RegistrableDomain&, WebCore::ProcessIdentifier);
7273
void addContextConnection(WebSharedWorkerServerToContextConnection&);

‎Source/WebKit/NetworkProcess/SharedWorker/WebSharedWorkerServerToContextConnection.cpp‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,13 @@ void WebSharedWorkerServerToContextConnection::postExceptionToWorkerObject(WebCo
9797
m_server->postExceptionToWorkerObject(sharedWorkerIdentifier, errorMessage, lineNumber, columnNumber, sourceURL);
9898
}
9999

100+
void WebSharedWorkerServerToContextConnection::sharedWorkerTerminated(WebCore::SharedWorkerIdentifier sharedWorkerIdentifier)
101+
{
102+
CONTEXT_CONNECTION_RELEASE_LOG("sharedWorkerTerminated: sharedWorkerIdentifier=%" PRIu64, sharedWorkerIdentifier.toUInt64());
103+
if (m_server)
104+
m_server->sharedWorkerTerminated(sharedWorkerIdentifier);
105+
}
106+
100107
void WebSharedWorkerServerToContextConnection::launchSharedWorker(WebSharedWorker& sharedWorker)
101108
{
102109
CONTEXT_CONNECTION_RELEASE_LOG("launchSharedWorker: sharedWorkerIdentifier=%" PRIu64, sharedWorker.identifier().toUInt64());
@@ -120,7 +127,7 @@ void WebSharedWorkerServerToContextConnection::launchSharedWorker(WebSharedWorke
120127
}
121128
send(Messages::WebSharedWorkerContextManagerConnection::LaunchSharedWorker { sharedWorker.origin(), sharedWorker.identifier(), sharedWorker.workerOptions(), sharedWorker.fetchResult(), initializationData });
122129
sharedWorker.forEachSharedWorkerObject([&](auto, auto& port) {
123-
postConnectEvent(sharedWorker, port);
130+
postConnectEvent(sharedWorker, port, [](bool) { });
124131
});
125132
}
126133

@@ -134,10 +141,10 @@ void WebSharedWorkerServerToContextConnection::resumeSharedWorker(WebCore::Share
134141
send(Messages::WebSharedWorkerContextManagerConnection::ResumeSharedWorker { identifier });
135142
}
136143

137-
void WebSharedWorkerServerToContextConnection::postConnectEvent(const WebSharedWorker& sharedWorker, const WebCore::TransferredMessagePort& port)
144+
void WebSharedWorkerServerToContextConnection::postConnectEvent(const WebSharedWorker& sharedWorker, const WebCore::TransferredMessagePort& port, CompletionHandler<void(bool)>&& completionHandler)
138145
{
139146
CONTEXT_CONNECTION_RELEASE_LOG("postConnectEvent: sharedWorkerIdentifier=%" PRIu64, sharedWorker.identifier().toUInt64());
140-
send(Messages::WebSharedWorkerContextManagerConnection::PostConnectEvent { sharedWorker.identifier(), port, sharedWorker.origin().clientOrigin.toString() });
147+
sendWithAsyncReply(Messages::WebSharedWorkerContextManagerConnection::PostConnectEvent { sharedWorker.identifier(), port, sharedWorker.origin().clientOrigin.toString() }, WTFMove(completionHandler));
141148
}
142149

143150
void WebSharedWorkerServerToContextConnection::terminateSharedWorker(const WebSharedWorker& sharedWorker)

0 commit comments

Comments
 (0)