Test addition (243907@main): [ iOS ][ macOS ] imported/w3c/web-platfo…

…rm-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/blob-url-workers.window.html is a flaky failure

https://bugs.webkit.org/show_bug.cgi?id=244549
rdar://99341974

Reviewed by Geoffrey Garen.

The test was flaky because the Worker object was getting garbage collected too
aggressively and message event listeners on the Worker object wouldn't get
called as a result.

Basically, the worker can fire events as long as:
1. Its script is being loaded
2. Its script is running since the script can post messages at any point

This patch updates Worker::virtualHasPendingActivity() accordingly to address
the issue.

Our new behavior seems more inline with the specification [1] and Blink.

[1] https://html.spec.whatwg.org/#the-worker's-lifetime

* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/mac/TestExpectations:
* Source/WebCore/workers/Worker.cpp:
(WebCore::Worker::virtualHasPendingActivity const):
(WebCore::Worker::notifyFinished):
* Source/WebCore/workers/Worker.h:

Canonical link: https://commits.webkit.org/256496@main

LayoutTests/fast/workers/resources/dedicated-worker-lifecycle.js

@@ -30,7 +30,7 @@ function orphanAllWorkers(callback)
 {
     var i;
     for (i = 0; i < numberOfWorkers; i++) {
-        workers[i].onmessage = 0;
+        workers[i].terminate();
         workers[i] = 0;
     }
     workers = [ ];

LayoutTests/inspector/worker/worker-create-and-terminate-expected.txt

@@ -22,7 +22,3 @@ Worker - inspector/worker/resources/worker-3.js
 PASS: Worker.workerTerminated
 Worker - inspector/worker/resources/worker-1.js
 
--- Running test case: Worker.workerTerminated.GC
-PASS: Worker.workerTerminated
-No Workers
-

LayoutTests/inspector/worker/worker-create-and-terminate.html

@@ -19,10 +19,6 @@
     worker3.postMessage("close");
 }
 
-function terminateWorker1ViaCollection() {
-    worker1 = null;
-}
-
 function test()
 {
     let workers = [];
@@ -183,25 +179,6 @@
         }
     });
 
-    suite.addTestCase({
-        name: "Worker.workerTerminated.GC",
-        description: "Should receive a Worker.workerTerminated event when terminating a Worker via Garbage Collection.",
-        async test() {
-            let workerTerminatedPromise = waitForWorkerTerminatedEvent();
-            await ProtocolTest.evaluateInPage("terminateWorker1ViaCollection()");
-            await InspectorProtocol.awaitCommand({method: "Runtime.releaseObjectGroup", params: {objectGroup: "test"}});
-            await InspectorProtocol.awaitCommand({method: "Heap.gc", params: {}});
-            await workerTerminatedPromise;
-
-            ProtocolTest.pass("Worker.workerTerminated");
-            dumpWorkers();
-
-            await Promise.all([
-                checkInternalProperties(`worker2`, {name: "Worker 2", terminated: true}),
-            ]);
-        }
-    });
-
     suite.runTestCasesAndFinish();
 }
 </script>

LayoutTests/platform/ios/TestExpectations

@@ -3746,8 +3746,6 @@ webkit.org/b/244358 imported/w3c/web-platform-tests/html/cross-origin-opener-pol
 
 webkit.org/b/244501 [ Debug ] webgl/2.0.y/conformance2/state/gl-object-get-calls.html [ Pass Timeout ]
 
-webkit.org/b/244549 imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/blob-url-workers.window.html [ Pass Failure ]
-
 webkit.org/b/244619 editing/selection/ios/tap-to-set-selection-in-editable-web-view.html [ Pass Timeout ]
 
 webkit.org/b/244740 http/wpt/cross-origin-opener-policy/non-secure-to-secure-context-navigation.https.html [ Pass Failure ]

LayoutTests/platform/mac/TestExpectations

@@ -2346,8 +2346,6 @@ webkit.org/b/244012 imported/w3c/web-platform-tests/css/css-transforms/transform
 
 webkit.org/b/244014 [ Debug ] imported/w3c/web-platform-tests/html/semantics/document-metadata/the-style-element/style-load-after-mutate.html  [ Pass Failure ]
 
-webkit.org/b/244549 imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/dynamic-import/blob-url-workers.window.html [ Pass Failure ]
-
 # These tests are flaky due to "Blocked access to external URL" messages because we don't support WPT domains.
 imported/w3c/web-platform-tests/feature-policy/feature-policy-frame-policy-allowed-for-all.https.sub.html [ DumpJSConsoleLogInStdErr Failure Pass ]
 imported/w3c/web-platform-tests/feature-policy/feature-policy-frame-policy-allowed-for-self.https.sub.html [ DumpJSConsoleLogInStdErr Failure Pass ]

Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp

@@ -105,13 +105,6 @@ ExceptionOr<void> DedicatedWorkerGlobalScope::postMessage(JSC::JSGlobalObject& s
     return { };
 }
 
-ExceptionOr<void> DedicatedWorkerGlobalScope::importScripts(const FixedVector<String>& urls)
-{
-    auto result = Base::importScripts(urls);
-    thread().workerObjectProxy().reportPendingActivity(hasPendingActivity());
-    return result;
-}
-
 DedicatedWorkerThread& DedicatedWorkerGlobalScope::thread()
 {
     return static_cast<DedicatedWorkerThread&>(Base::thread());

Source/WebCore/workers/DedicatedWorkerGlobalScope.h

@@ -96,7 +96,6 @@ class DedicatedWorkerGlobalScope final : public WorkerGlobalScope {
 
     Type type() const final { return Type::DedicatedWorker; }
 
-    ExceptionOr<void> importScripts(const FixedVector<String>& urls) final;
     EventTargetInterface eventTargetInterface() const final;
 
     void prepareForDestruction() final;

Source/WebCore/workers/DedicatedWorkerThread.cpp

@@ -58,11 +58,4 @@ Ref<WorkerGlobalScope> DedicatedWorkerThread::createWorkerGlobalScope(const Work
     return scope;
 }
 
-void DedicatedWorkerThread::runEventLoop()
-{
-    // Notify the parent object of our current active state before calling the superclass to run the event loop.
-    m_workerObjectProxy.reportPendingActivity(globalScope()->hasPendingActivity());
-    WorkerThread::runEventLoop();
-}
-
 } // namespace WebCore

Source/WebCore/workers/DedicatedWorkerThread.h

@@ -52,7 +52,6 @@ class DedicatedWorkerThread : public WorkerThread {
 
 protected:
     Ref<WorkerGlobalScope> createWorkerGlobalScope(const WorkerParameters&, Ref<SecurityOrigin>&&, Ref<SecurityOrigin>&& topOrigin) override;
-    void runEventLoop() override;
 
 private:
     DedicatedWorkerThread(const WorkerParameters&, const ScriptBuffer& sourceCode, WorkerLoaderProxy&, WorkerDebuggerProxy&, WorkerObjectProxy&, WorkerThreadStartMode, const SecurityOrigin& topOrigin, IDBClient::IDBConnectionProxy*, SocketProvider*, JSC::RuntimeFlags);

Source/WebCore/workers/Worker.cpp

@@ -183,7 +183,7 @@ void Worker::resume()
 
 bool Worker::virtualHasPendingActivity() const
 {
-    return m_contextProxy.hasPendingActivity() || m_scriptLoader;
+    return m_scriptLoader || (m_didStartWorkerGlobalScope && !m_contextProxy.askedToTerminate());
 }
 
 void Worker::didReceiveResponse(ResourceLoaderIdentifier identifier, const ResourceResponse& response)
@@ -218,6 +218,7 @@ void Worker::notifyFinished()
     if (auto policy = parseReferrerPolicy(m_scriptLoader->referrerPolicy(), ReferrerPolicySource::HTTPHeader))
         referrerPolicy = *policy;
 
+    m_didStartWorkerGlobalScope = true;
     WorkerInitializationData initializationData {
 #if ENABLE(SERVICE_WORKER)
         m_scriptLoader->takeServiceWorkerData(),

Source/WebCore/workers/Worker.h

@@ -112,6 +112,7 @@ class Worker final : public AbstractWorker, public ActiveDOMObject, private Work
     JSC::RuntimeFlags m_runtimeFlags;
     Deque<RefPtr<Event>> m_pendingEvents;
     bool m_wasTerminated { false };
+    bool m_didStartWorkerGlobalScope { false };
     const ScriptExecutionContextIdentifier m_clientIdentifier;
 };
 

Source/WebCore/workers/WorkerGlobalScopeProxy.h

@@ -56,7 +56,7 @@ class WorkerGlobalScopeProxy {
     virtual void terminateWorkerGlobalScope() = 0;
     virtual void postMessageToWorkerGlobalScope(MessageWithMessagePorts&&) = 0;
     virtual void postTaskToWorkerGlobalScope(Function<void(ScriptExecutionContext&)>&&) = 0;
-    virtual bool hasPendingActivity() const = 0;
+    virtual bool askedToTerminate() const = 0;
     virtual void workerObjectDestroyed() = 0;
     virtual void notifyNetworkStateChange(bool isOnline) = 0;
 

Source/WebCore/workers/WorkerMessagingProxy.cpp

@@ -201,7 +201,6 @@ void WorkerMessagingProxy::postMessageToWorkerGlobalScope(MessageWithMessagePort
         m_userGestureForwarder = WTFMove(userGestureForwarder);
 
         context.dispatchEvent(MessageEvent::create(message.message.releaseNonNull(), { }, { }, std::nullopt, WTFMove(ports)));
-        context.thread().workerObjectProxy().confirmMessageFromWorkerObject(context.hasPendingActivity());
 
         // Because WorkerUserGestureForwarder is defined as DestructionThread::Main, releasing this Ref
         // on the Worker thread will cause the forwarder to be destroyed on the main thread.
@@ -218,7 +217,6 @@ void WorkerMessagingProxy::postTaskToWorkerGlobalScope(Function<void(ScriptExecu
         m_queuedEarlyTasks.append(makeUnique<ScriptExecutionContext::Task>(WTFMove(task)));
         return;
     }
-    ++m_unconfirmedMessageCount;
     m_workerThread->runLoop().postTask(WTFMove(task));
 }
 
@@ -326,10 +324,6 @@ void WorkerMessagingProxy::workerThreadCreated(DedicatedWorkerThread& workerThre
             m_workerThread->suspend();
         }
 
-        ASSERT(!m_unconfirmedMessageCount);
-        m_unconfirmedMessageCount = m_queuedEarlyTasks.size();
-        m_workerThreadHadPendingActivity = true; // Worker initialization means a pending activity.
-
         auto queuedEarlyTasks = WTFMove(m_queuedEarlyTasks);
         for (auto& task : queuedEarlyTasks)
             m_workerThread->runLoop().postTask(WTFMove(*task));
@@ -419,39 +413,4 @@ void WorkerMessagingProxy::terminateWorkerGlobalScope()
         m_scriptExecutionContext = nullptr;
 }
 
-void WorkerMessagingProxy::confirmMessageFromWorkerObject(bool hasPendingActivity)
-{
-    if (!m_scriptExecutionContext)
-        return;
-
-    m_scriptExecutionContext->postTask([this, hasPendingActivity] (ScriptExecutionContext&) {
-        reportPendingActivityInternal(true, hasPendingActivity);
-    });
-}
-
-void WorkerMessagingProxy::reportPendingActivity(bool hasPendingActivity)
-{
-    if (!m_scriptExecutionContext)
-        return;
-
-    m_scriptExecutionContext->postTask([this, hasPendingActivity] (ScriptExecutionContext&) {
-        reportPendingActivityInternal(false, hasPendingActivity);
-    });
-}
-
-void WorkerMessagingProxy::reportPendingActivityInternal(bool confirmingMessage, bool hasPendingActivity)
-{
-    if (confirmingMessage && !m_askedToTerminate) {
-        ASSERT(m_unconfirmedMessageCount);
-        --m_unconfirmedMessageCount;
-    }
-
-    m_workerThreadHadPendingActivity = hasPendingActivity;
-}
-
-bool WorkerMessagingProxy::hasPendingActivity() const
-{
-    return (m_unconfirmedMessageCount || m_workerThreadHadPendingActivity) && !m_askedToTerminate;
-}
-
 } // namespace WebCore

Source/WebCore/workers/WorkerMessagingProxy.h

@@ -51,7 +51,6 @@ class WorkerMessagingProxy final : public ThreadSafeRefCounted<WorkerMessagingPr
     void terminateWorkerGlobalScope() final;
     void postMessageToWorkerGlobalScope(MessageWithMessagePorts&&) final;
     void postTaskToWorkerGlobalScope(Function<void(ScriptExecutionContext&)>&&) final;
-    bool hasPendingActivity() const final;
     void workerObjectDestroyed() final;
     void notifyNetworkStateChange(bool isOnline) final;
     void suspendForBackForwardCache() final;
@@ -62,8 +61,6 @@ class WorkerMessagingProxy final : public ThreadSafeRefCounted<WorkerMessagingPr
     void postMessageToWorkerObject(MessageWithMessagePorts&&) final;
     void postTaskToWorkerObject(Function<void(Worker&)>&&) final;
     void postExceptionToWorkerObject(const String& errorMessage, int lineNumber, int columnNumber, const String& sourceURL) final;
-    void confirmMessageFromWorkerObject(bool hasPendingActivity) final;
-    void reportPendingActivity(bool hasPendingActivity) final;
     void workerGlobalScopeClosed() final;
     void workerGlobalScopeDestroyed() final;
 
@@ -84,11 +81,9 @@ class WorkerMessagingProxy final : public ThreadSafeRefCounted<WorkerMessagingPr
 
     void workerThreadCreated(DedicatedWorkerThread&);
 
-    // Only use this method on the worker object thread.
-    bool askedToTerminate() const { return m_askedToTerminate; }
+    bool askedToTerminate() const final { return m_askedToTerminate; }
 
     void workerGlobalScopeDestroyedInternal();
-    void reportPendingActivityInternal(bool confirmingMessage, bool hasPendingActivity);
     Worker* workerObject() const { return m_workerObject; }
 
     RefPtr<ScriptExecutionContext> m_scriptExecutionContext;
@@ -99,9 +94,6 @@ class WorkerMessagingProxy final : public ThreadSafeRefCounted<WorkerMessagingPr
     bool m_mayBeDestroyed { false };
     RefPtr<DedicatedWorkerThread> m_workerThread;
 
-    unsigned m_unconfirmedMessageCount { 0 }; // Unconfirmed messages from worker object to worker thread.
-    bool m_workerThreadHadPendingActivity { false }; // The latest confirmation from worker thread reported that it was still active.
-
     bool m_askedToSuspend { false };
     bool m_askedToTerminate { false };
 

Source/WebCore/workers/WorkerObjectProxy.h

@@ -44,10 +44,7 @@ class Worker;
 class WorkerObjectProxy : public WorkerReportingProxy {
 public:
     virtual void postMessageToWorkerObject(MessageWithMessagePorts&&) = 0;
-    virtual void postTaskToWorkerObject(Function<void(Worker&)>&&) { };
-
-    virtual void confirmMessageFromWorkerObject(bool hasPendingActivity) = 0;
-    virtual void reportPendingActivity(bool hasPendingActivity) = 0;
+    virtual void postTaskToWorkerObject(Function<void(Worker&)>&&) { }
 
     // No need to notify the parent page context when dedicated workers are closing.
     void workerGlobalScopeClosed() override { }

Source/WebCore/workers/service/context/ServiceWorkerThread.cpp

@@ -71,8 +71,6 @@ class DummyServiceWorkerThreadProxy : public WorkerObjectProxy {
     void postExceptionToWorkerObject(const String&, int, int, const String&) final { };
     void workerGlobalScopeDestroyed() final { };
     void postMessageToWorkerObject(MessageWithMessagePorts&&) final { };
-    void confirmMessageFromWorkerObject(bool) final { };
-    void reportPendingActivity(bool) final { };
 };
 
 // FIXME: Use a valid WorkerReportingProxy
@@ -145,7 +143,6 @@ static void fireMessageEvent(ServiceWorkerGlobalScope& scope, MessageWithMessage
     auto ports = MessagePort::entanglePorts(scope, WTFMove(message.transferredPorts));
     auto messageEvent = ExtendableMessageEvent::create(WTFMove(ports), WTFMove(message.message), SecurityOriginData::fromURL(sourceURL).toString(), { }, source);
     scope.dispatchEvent(messageEvent);
-    scope.thread().workerObjectProxy().confirmMessageFromWorkerObject(scope.hasPendingActivity());
     scope.updateExtendedEventsSet(messageEvent.ptr());
 }
 

Source/WebCore/workers/shared/context/SharedWorkerThreadProxy.h

@@ -67,8 +67,6 @@ class SharedWorkerThreadProxy final : public ThreadSafeRefCounted<SharedWorkerTh
     void postMessageToWorkerObject(MessageWithMessagePorts&&) final { }
     void workerGlobalScopeDestroyed() final { }
     void workerGlobalScopeClosed() final;
-    void confirmMessageFromWorkerObject(bool) final { }
-    void reportPendingActivity(bool) final { }
 
     // WorkerLoaderProxy.
     RefPtr<CacheStorageConnection> createCacheStorageConnection() final;