Attachment 360180 Details for Bug 193747 – Patch

[patch] Patch

bug-193747-20190125155702.patch (text/plain), 27.81 KB, created by youenn fablet on 2019-01-25 15:57:03 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-01-25 15:57:03 PST

Size: 27.81 KB

patch

obsolete

>Subversion Revision: 240268
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c2e528dcd9957cef760dddb8a4e04c246836dfbd..9d19a376f3c365a2047e33b8423b7a3898ef587f 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,54 @@
>+2019-01-25  Youenn Fablet  <youenn@apple.com>
>+
>+        Refactor ServiceWorkerJob management by ServiceWorkerContainer to make it more memory safe
>+        https://bugs.webkit.org/show_bug.cgi?id=193747
>+        <rdar://problem/47498196>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Make ServiceWorkerJob be no longer ref counted.
>+        Instead its lifetime is fully controlled by ServiceWorkerContainer.
>+
>+        Make sure that a failing load will remove the job from the map.
>+        This allows to ensure that these jobs do not stay forever.  
>+
>+        Make sure that setPendingActivity/unsetPendingActivity is balanced by storing
>+        a pending activity in the job map next to the job.
>+
>+        When ServiceWorkerContainer is stopped, notify that all jobs are cancelled to NetworkProcess.
>+        This makes these jobs in NetworkProcess-side to not stay until the cooresponding WebProcess is gone.
>+
>+        Simplify ServiceWorkerJob promise rejection handling so that it is clear when promise is rejected and when it is not.
>+        Update type of exception to be SecurityError when load fails due to AccessControl.
>+
>+        Covered by existing tests.
>+
>+        * workers/service/ServiceWorkerContainer.cpp:
>+        (WebCore::ServiceWorkerContainer::addRegistration):
>+        (WebCore::ServiceWorkerContainer::removeRegistration):
>+        (WebCore::ServiceWorkerContainer::updateRegistration):
>+        (WebCore::ServiceWorkerContainer::scheduleJob):
>+        (WebCore::ServiceWorkerContainer::jobFailedWithException):
>+        (WebCore::ServiceWorkerContainer::jobResolvedWithRegistration):
>+        (WebCore::ServiceWorkerContainer::jobResolvedWithUnregistrationResult):
>+        (WebCore::ServiceWorkerContainer::jobFailedLoadingScript):
>+        (WebCore::ServiceWorkerContainer::jobDidFinish):
>+        (WebCore::ServiceWorkerContainer::stop):
>+        (WebCore::ServiceWorkerContainer::job):
>+        * workers/service/ServiceWorkerContainer.h:
>+        * workers/service/ServiceWorkerJob.cpp:
>+        (WebCore::ServiceWorkerJob::failedWithException):
>+        (WebCore::ServiceWorkerJob::resolvedWithRegistration):
>+        (WebCore::ServiceWorkerJob::resolvedWithUnregistrationResult):
>+        (WebCore::ServiceWorkerJob::startScriptFetch):
>+        (WebCore::ServiceWorkerJob::didReceiveResponse):
>+        (WebCore::ServiceWorkerJob::notifyFinished):
>+        (WebCore::ServiceWorkerJob::cancelPendingLoad):
>+        * workers/service/ServiceWorkerJob.h:
>+        (WebCore::ServiceWorkerJob::hasPromise const):
>+        (WebCore::ServiceWorkerJob::takePromise):
>+        * workers/service/ServiceWorkerJobClient.h:
>+
> 2019-01-22  Claudio Saavedra  <csaavedra@igalia.com>
> 
>         [GTK] Build fix for Ubuntu LTS 16.04
>diff --git a/Source/WebCore/workers/service/ServiceWorkerContainer.cpp b/Source/WebCore/workers/service/ServiceWorkerContainer.cpp
>index 7442dd45537f1f4f4aead62652e36a74609d990b..b37da475e2863737cc7b627156a91c33ce061ef1 100644
>--- a/Source/WebCore/workers/service/ServiceWorkerContainer.cpp
>+++ b/Source/WebCore/workers/service/ServiceWorkerContainer.cpp
>@@ -176,7 +176,7 @@ void ServiceWorkerContainer::addRegistration(const String& relativeScriptURL, co
>     jobData.type = ServiceWorkerJobType::Register;
>     jobData.registrationOptions = options;
> 
>-    scheduleJob(ServiceWorkerJob::create(*this, WTFMove(promise), WTFMove(jobData)));
>+    scheduleJob(std::make_unique<ServiceWorkerJob>(*this, WTFMove(promise), WTFMove(jobData)));
> }
> 
> void ServiceWorkerContainer::removeRegistration(const URL& scopeURL, Ref<DeferredPromise>&& promise)
>@@ -202,7 +202,7 @@ void ServiceWorkerContainer::removeRegistration(const URL& scopeURL, Ref<Deferre
> 
>     CONTAINER_RELEASE_LOG_IF_ALLOWED("removeRegistration: Unregistering service worker. Job ID: %" PRIu64, jobData.identifier().jobIdentifier.toUInt64());
> 
>-    scheduleJob(ServiceWorkerJob::create(*this, WTFMove(promise), WTFMove(jobData)));
>+    scheduleJob(std::make_unique<ServiceWorkerJob>(*this, WTFMove(promise), WTFMove(jobData)));
> }
> 
> void ServiceWorkerContainer::updateRegistration(const URL& scopeURL, const URL& scriptURL, WorkerType, RefPtr<DeferredPromise>&& promise)
>@@ -228,22 +228,22 @@ void ServiceWorkerContainer::updateRegistration(const URL& scopeURL, const URL&
> 
>     CONTAINER_RELEASE_LOG_IF_ALLOWED("removeRegistration: Updating service worker. Job ID: %" PRIu64, jobData.identifier().jobIdentifier.toUInt64());
> 
>-    scheduleJob(ServiceWorkerJob::create(*this, WTFMove(promise), WTFMove(jobData)));
>+    scheduleJob(std::make_unique<ServiceWorkerJob>(*this, WTFMove(promise), WTFMove(jobData)));
> }
> 
>-void ServiceWorkerContainer::scheduleJob(Ref<ServiceWorkerJob>&& job)
>+void ServiceWorkerContainer::scheduleJob(std::unique_ptr<ServiceWorkerJob>&& job)
> {
> #ifndef NDEBUG
>     ASSERT(m_creationThread.ptr() == &Thread::current());
> #endif
> 
>     ASSERT(m_swConnection);
>-
>-    setPendingActivity(*this);
>+    ASSERT(!isStopped());
> 
>     auto& jobData = job->data();
>-    auto result = m_jobMap.add(job->identifier(), WTFMove(job));
>-    ASSERT_UNUSED(result, result.isNewEntry);
>+    auto jobIdentifier = job->identifier();
>+    ASSERT(!m_jobMap.contains(jobIdentifier));
>+    m_jobMap.add(jobIdentifier, OngoingJob { WTFMove(job), makePendingActivity(*this) });
> 
>     callOnMainThread([connection = m_swConnection, contextIdentifier = this->contextIdentifier(), jobData = jobData.isolatedCopy()] {
>         connection->scheduleJob(contextIdentifier, jobData);
>@@ -385,20 +385,21 @@ void ServiceWorkerContainer::jobFailedWithException(ServiceWorkerJob& job, const
>     ASSERT(m_creationThread.ptr() == &Thread::current());
> #endif
> 
>-    ASSERT_WITH_MESSAGE(job.promise() || job.data().type == ServiceWorkerJobType::Update, "Only soft updates have no promise");
>+    ASSERT_WITH_MESSAGE(job.hasPromise() || job.data().type == ServiceWorkerJobType::Update, "Only soft updates have no promise");
> 
>     auto guard = WTF::makeScopeExit([this, &job] {
>-        jobDidFinish(job);
>+        destroyJob(job);
>     });
> 
>     CONTAINER_RELEASE_LOG_ERROR_IF_ALLOWED("jobFailedWithException: Job %" PRIu64 " failed with error %s", job.identifier().toUInt64(), exception.message().utf8().data());
> 
>-    if (!job.promise())
>+    auto promise = job.takePromise();
>+    if (!promise)
>         return;
> 
>     if (auto* context = scriptExecutionContext()) {
>-        context->postTask([job = makeRef(job), exception](ScriptExecutionContext&) {
>-            job->promise()->reject(exception);
>+        context->postTask([promise = WTFMove(promise), exception](auto&) mutable {
>+            promise->reject(exception);
>         });
>     }
> }
>@@ -418,10 +419,10 @@ void ServiceWorkerContainer::jobResolvedWithRegistration(ServiceWorkerJob& job,
> #ifndef NDEBUG
>     ASSERT(m_creationThread.ptr() == &Thread::current());
> #endif
>-    ASSERT_WITH_MESSAGE(job.promise() || job.data().type == ServiceWorkerJobType::Update, "Only soft updates have no promise");
>+    ASSERT_WITH_MESSAGE(job.hasPromise() || job.data().type == ServiceWorkerJobType::Update, "Only soft updates have no promise");
> 
>     auto guard = WTF::makeScopeExit([this, &job] {
>-        jobDidFinish(job);
>+        destroyJob(job);
>     });
> 
>     if (job.data().type == ServiceWorkerJobType::Register)
>@@ -446,13 +447,14 @@ void ServiceWorkerContainer::jobResolvedWithRegistration(ServiceWorkerJob& job,
>         return;
>     }
> 
>-    if (!job.promise()) {
>+    auto promise = job.takePromise();
>+    if (!promise) {
>         if (notifyWhenResolvedIfNeeded)
>             notifyWhenResolvedIfNeeded();
>         return;
>     }
> 
>-    scriptExecutionContext()->postTask([this, protectedThis = makeRef(*this), job = makeRef(job), data = WTFMove(data), notifyWhenResolvedIfNeeded = WTFMove(notifyWhenResolvedIfNeeded)](ScriptExecutionContext& context) mutable {
>+    scriptExecutionContext()->postTask([this, protectedThis = makeRef(*this), promise = WTFMove(promise), jobIdentifier = job.identifier(), data = WTFMove(data), notifyWhenResolvedIfNeeded = WTFMove(notifyWhenResolvedIfNeeded)](ScriptExecutionContext& context) mutable {
>         if (isStopped() || !context.sessionID().isValid()) {
>             if (notifyWhenResolvedIfNeeded)
>                 notifyWhenResolvedIfNeeded();
>@@ -461,15 +463,15 @@ void ServiceWorkerContainer::jobResolvedWithRegistration(ServiceWorkerJob& job,
> 
>         auto registration = ServiceWorkerRegistration::getOrCreate(context, *this, WTFMove(data));
> 
>-        CONTAINER_RELEASE_LOG_IF_ALLOWED("jobResolvedWithRegistration: Resolving promise for job %" PRIu64 ". Registration ID: %" PRIu64, job->identifier().toUInt64(), registration->identifier().toUInt64());
>+        CONTAINER_RELEASE_LOG_IF_ALLOWED("jobResolvedWithRegistration: Resolving promise for job %" PRIu64 ". Registration ID: %" PRIu64, jobIdentifier.toUInt64(), registration->identifier().toUInt64());
> 
>         if (notifyWhenResolvedIfNeeded) {
>-            job->promise()->whenSettled([notifyWhenResolvedIfNeeded = WTFMove(notifyWhenResolvedIfNeeded)] {
>+            promise->whenSettled([notifyWhenResolvedIfNeeded = WTFMove(notifyWhenResolvedIfNeeded)] {
>                 notifyWhenResolvedIfNeeded();
>             });
>         }
> 
>-        job->promise()->resolve<IDLInterface<ServiceWorkerRegistration>>(WTFMove(registration));
>+        promise->resolve<IDLInterface<ServiceWorkerRegistration>>(WTFMove(registration));
>     });
> }
> 
>@@ -479,10 +481,10 @@ void ServiceWorkerContainer::jobResolvedWithUnregistrationResult(ServiceWorkerJo
>     ASSERT(m_creationThread.ptr() == &Thread::current());
> #endif
> 
>-    ASSERT(job.promise());
>+    ASSERT(job.hasPromise());
> 
>     auto guard = WTF::makeScopeExit([this, &job] {
>-        jobDidFinish(job);
>+        destroyJob(job);
>     });
> 
>     CONTAINER_RELEASE_LOG_IF_ALLOWED("jobResolvedWithUnregistrationResult: Unregister job %" PRIu64 " finished. Success? %d", job.identifier().toUInt64(), unregistrationResult);
>@@ -493,8 +495,8 @@ void ServiceWorkerContainer::jobResolvedWithUnregistrationResult(ServiceWorkerJo
>         return;
>     }
> 
>-    context->postTask([job = makeRef(job), unregistrationResult](ScriptExecutionContext&) mutable {
>-        job->promise()->resolve<IDLBoolean>(unregistrationResult);
>+    context->postTask([promise = job.takePromise(), unregistrationResult](auto&) mutable {
>+        promise->resolve<IDLBoolean>(unregistrationResult);
>     });
> }
> 
>@@ -509,10 +511,8 @@ void ServiceWorkerContainer::startScriptFetchForJob(ServiceWorkerJob& job, Fetch
>     auto* context = scriptExecutionContext();
>     if (!context) {
>         LOG_ERROR("ServiceWorkerContainer::jobResolvedWithRegistration called but the container's ScriptExecutionContext is gone");
>-        callOnMainThread([connection = m_swConnection, jobIdentifier = job.identifier(), registrationKey = job.data().registrationKey().isolatedCopy(), scriptURL = job.data().scriptURL.isolatedCopy()] {
>-            connection->failedFetchingScript(jobIdentifier, registrationKey, { errorDomainWebKitInternal, 0, scriptURL, "Attempt to fetch service worker script with no ScriptExecutionContext"_s });
>-        });
>-        jobDidFinish(job);
>+        notifyFailedFetchingScript(job, { errorDomainWebKitInternal, 0, job.data().scriptURL, "Attempt to fetch service worker script with no ScriptExecutionContext"_s });
>+        destroyJob(job);
>         return;
>     }
> 
>@@ -532,33 +532,37 @@ void ServiceWorkerContainer::jobFinishedLoadingScript(ServiceWorkerJob& job, con
>     });
> }
> 
>-void ServiceWorkerContainer::jobFailedLoadingScript(ServiceWorkerJob& job, const ResourceError& error, Optional<Exception>&& exception)
>+void ServiceWorkerContainer::jobFailedLoadingScript(ServiceWorkerJob& job, const ResourceError& error, Exception&& exception)
> {
> #ifndef NDEBUG
>     ASSERT(m_creationThread.ptr() == &Thread::current());
> #endif
>-    ASSERT_WITH_MESSAGE(job.promise() || job.data().type == ServiceWorkerJobType::Update, "Only soft updates have no promise");
>+    ASSERT_WITH_MESSAGE(job.hasPromise() || job.data().type == ServiceWorkerJobType::Update, "Only soft updates have no promise");
> 
>     CONTAINER_RELEASE_LOG_ERROR_IF_ALLOWED("jobFinishedLoadingScript: Failed to fetch script for job %" PRIu64 ", error: %s", job.identifier().toUInt64(), error.localizedDescription().utf8().data());
> 
>-    if (exception && job.promise())
>-        job.promise()->reject(*exception);
>+    if (auto promise = job.takePromise())
>+        promise->reject(WTFMove(exception));
> 
>+    notifyFailedFetchingScript(job, error);
>+    destroyJob(job);
>+}
>+
>+void ServiceWorkerContainer::notifyFailedFetchingScript(ServiceWorkerJob& job, const ResourceError& error)
>+{
>     callOnMainThread([connection = m_swConnection, jobIdentifier = job.identifier(), registrationKey = job.data().registrationKey().isolatedCopy(), error = error.isolatedCopy()] {
>         connection->failedFetchingScript(jobIdentifier, registrationKey, error);
>     });
> }
> 
>-void ServiceWorkerContainer::jobDidFinish(ServiceWorkerJob& job)
>+void ServiceWorkerContainer::destroyJob(ServiceWorkerJob& job)
> {
> #ifndef NDEBUG
>     ASSERT(m_creationThread.ptr() == &Thread::current());
> #endif
> 
>-    auto taken = m_jobMap.take(job.identifier());
>-    ASSERT_UNUSED(taken, !taken || taken->ptr() == &job);
>-
>-    unsetPendingActivity(*this);
>+    ASSERT(m_jobMap.contains(job.identifier()));
>+    m_jobMap.remove(job.identifier());
> }
> 
> SWServerConnectionIdentifier ServiceWorkerContainer::connectionIdentifier()
>@@ -633,8 +637,11 @@ void ServiceWorkerContainer::stop()
>     removeAllEventListeners();
>     m_pendingPromises.clear();
>     m_readyPromise = nullptr;
>-    for (auto& job : m_jobMap.values())
>-        job->cancelPendingLoad();
>+    auto jobMap = WTFMove(m_jobMap);
>+    for (auto& ongoingJob : jobMap.values()) {
>+        notifyFailedFetchingScript(*ongoingJob.job.get(), ResourceError { errorDomainWebKitInternal, 0, ongoingJob.job->data().scriptURL, "Job cancelled"_s, ResourceError::Type::Cancellation });
>+        ongoingJob.job->cancelPendingLoad();
>+    }
> }
> 
> DocumentOrWorkerIdentifier ServiceWorkerContainer::contextIdentifier()
>@@ -649,6 +656,14 @@ DocumentOrWorkerIdentifier ServiceWorkerContainer::contextIdentifier()
>     return downcast<Document>(*scriptExecutionContext()).identifier();
> }
> 
>+ServiceWorkerJob* ServiceWorkerContainer::job(ServiceWorkerJobIdentifier identifier)
>+{
>+    auto iterator = m_jobMap.find(identifier);
>+    if (iterator == m_jobMap.end())
>+        return nullptr;
>+    return iterator->value.job.get();
>+}
>+
> bool ServiceWorkerContainer::isAlwaysOnLoggingAllowed() const
> {
>     auto* context = scriptExecutionContext();
>diff --git a/Source/WebCore/workers/service/ServiceWorkerContainer.h b/Source/WebCore/workers/service/ServiceWorkerContainer.h
>index fd8c07d22cf63492f8de58718ec302614a96a5c1..60818dd075ea5c51198e21fa0f9e10953acc8fda 100644
>--- a/Source/WebCore/workers/service/ServiceWorkerContainer.h
>+++ b/Source/WebCore/workers/service/ServiceWorkerContainer.h
>@@ -77,28 +77,26 @@ public:
>     void addRegistration(ServiceWorkerRegistration&);
>     void removeRegistration(ServiceWorkerRegistration&);
> 
>-    ServiceWorkerJob* job(ServiceWorkerJobIdentifier identifier) { return m_jobMap.get(identifier); }
>+    ServiceWorkerJob* job(ServiceWorkerJobIdentifier);
> 
>     void startMessages();
> 
>-    void ref() final { refEventTarget(); }
>-    void deref() final { derefEventTarget(); }
>-
>     bool isStopped() const { return m_isStopped; };
> 
>     bool isAlwaysOnLoggingAllowed() const;
> 
> private:
>-    void scheduleJob(Ref<ServiceWorkerJob>&&);
>+    void scheduleJob(std::unique_ptr<ServiceWorkerJob>&&);
> 
>     void jobFailedWithException(ServiceWorkerJob&, const Exception&) final;
>     void jobResolvedWithRegistration(ServiceWorkerJob&, ServiceWorkerRegistrationData&&, ShouldNotifyWhenResolved) final;
>     void jobResolvedWithUnregistrationResult(ServiceWorkerJob&, bool unregistrationResult) final;
>     void startScriptFetchForJob(ServiceWorkerJob&, FetchOptions::Cache) final;
>     void jobFinishedLoadingScript(ServiceWorkerJob&, const String& script, const ContentSecurityPolicyResponseHeaders&, const String& referrerPolicy) final;
>-    void jobFailedLoadingScript(ServiceWorkerJob&, const ResourceError&, Optional<Exception>&&) final;
>+    void jobFailedLoadingScript(ServiceWorkerJob&, const ResourceError&, Exception&&) final;
> 
>-    void jobDidFinish(ServiceWorkerJob&);
>+    void notifyFailedFetchingScript(ServiceWorkerJob&, const ResourceError&);
>+    void destroyJob(ServiceWorkerJob&);
> 
>     void didFinishGetRegistrationRequest(uint64_t requestIdentifier, Optional<ServiceWorkerRegistrationData>&&);
>     void didFinishGetRegistrationsRequest(uint64_t requestIdentifier, Vector<ServiceWorkerRegistrationData>&&);
>@@ -121,7 +119,12 @@ private:
>     NavigatorBase& m_navigator;
> 
>     RefPtr<SWClientConnection> m_swConnection;
>-    HashMap<ServiceWorkerJobIdentifier, Ref<ServiceWorkerJob>> m_jobMap;
>+
>+    struct OngoingJob {
>+        std::unique_ptr<ServiceWorkerJob> job;
>+        RefPtr<PendingActivity<ServiceWorkerContainer>> pendingActivity;
>+    };
>+    HashMap<ServiceWorkerJobIdentifier, OngoingJob> m_jobMap;
> 
>     bool m_isStopped { false };
>     HashMap<ServiceWorkerRegistrationIdentifier, ServiceWorkerRegistration*> m_registrations;
>diff --git a/Source/WebCore/workers/service/ServiceWorkerJob.cpp b/Source/WebCore/workers/service/ServiceWorkerJob.cpp
>index 71d1e5281c73556da5ef897b0a46896b14544a90..009b9877b66305a4da3104a05b14c62b77e0eef5 100644
>--- a/Source/WebCore/workers/service/ServiceWorkerJob.cpp
>+++ b/Source/WebCore/workers/service/ServiceWorkerJob.cpp
>@@ -57,7 +57,7 @@ void ServiceWorkerJob::failedWithException(const Exception& exception)
>     ASSERT(!m_completed);
> 
>     m_completed = true;
>-    m_client->jobFailedWithException(*this, exception);
>+    m_client.jobFailedWithException(*this, exception);
> }
> 
> void ServiceWorkerJob::resolvedWithRegistration(ServiceWorkerRegistrationData&& data, ShouldNotifyWhenResolved shouldNotifyWhenResolved)
>@@ -66,7 +66,7 @@ void ServiceWorkerJob::resolvedWithRegistration(ServiceWorkerRegistrationData&&
>     ASSERT(!m_completed);
> 
>     m_completed = true;
>-    m_client->jobResolvedWithRegistration(*this, WTFMove(data), shouldNotifyWhenResolved);
>+    m_client.jobResolvedWithRegistration(*this, WTFMove(data), shouldNotifyWhenResolved);
> }
> 
> void ServiceWorkerJob::resolvedWithUnregistrationResult(bool unregistrationResult)
>@@ -75,7 +75,7 @@ void ServiceWorkerJob::resolvedWithUnregistrationResult(bool unregistrationResul
>     ASSERT(!m_completed);
> 
>     m_completed = true;
>-    m_client->jobResolvedWithUnregistrationResult(*this, unregistrationResult);
>+    m_client.jobResolvedWithUnregistrationResult(*this, unregistrationResult);
> }
> 
> void ServiceWorkerJob::startScriptFetch(FetchOptions::Cache cachePolicy)
>@@ -83,7 +83,7 @@ void ServiceWorkerJob::startScriptFetch(FetchOptions::Cache cachePolicy)
>     ASSERT(m_creationThread.ptr() == &Thread::current());
>     ASSERT(!m_completed);
> 
>-    m_client->startScriptFetchForJob(*this, cachePolicy);
>+    m_client.startScriptFetchForJob(*this, cachePolicy);
> }
> 
> void ServiceWorkerJob::fetchScriptWithContext(ScriptExecutionContext& context, FetchOptions::Cache cachePolicy)
>@@ -114,13 +114,17 @@ void ServiceWorkerJob::didReceiveResponse(unsigned long, const ResourceResponse&
> 
>     // Extract a MIME type from the response's header list. If this MIME type (ignoring parameters) is not a JavaScript MIME type, then:
>     if (!MIMETypeRegistry::isSupportedJavaScriptMIMEType(response.mimeType())) {
>+        m_scriptLoader->cancel();
>+        m_scriptLoader = nullptr;
>+
>         // Invoke Reject Job Promise with job and "SecurityError" DOMException.
>         Exception exception { SecurityError, "MIME Type is not a JavaScript MIME type"_s };
>         // Asynchronously complete these steps with a network error.
>         ResourceError error { errorDomainWebKitInternal, 0, response.url(), "Unexpected MIME type"_s };
>-        m_client->jobFailedLoadingScript(*this, WTFMove(error), WTFMove(exception));
>-        m_scriptLoader = nullptr;
>+        m_client.jobFailedLoadingScript(*this, WTFMove(error), WTFMove(exception));
>+        return;
>     }
>+
>     String serviceWorkerAllowed = response.httpHeaderField(HTTPHeaderName::ServiceWorkerAllowed);
>     String maxScopeString;
>     if (serviceWorkerAllowed.isNull()) {
>@@ -131,13 +135,17 @@ void ServiceWorkerJob::didReceiveResponse(unsigned long, const ResourceResponse&
>         auto maxScope = URL(m_jobData.scriptURL, serviceWorkerAllowed);
>         maxScopeString = maxScope.path();
>     }
>+
>     String scopeString = m_jobData.scopeURL.path();
>-    if (!scopeString.startsWith(maxScopeString)) {
>-        Exception exception { SecurityError, "Scope URL should start with the given script URL"_s };
>-        ResourceError error { errorDomainWebKitInternal, 0, response.url(), "Scope URL should start with the given script URL"_s };
>-        m_client->jobFailedLoadingScript(*this, WTFMove(error), WTFMove(exception));
>-        m_scriptLoader = nullptr;
>-    }
>+    if (scopeString.startsWith(maxScopeString))
>+        return;
>+
>+    m_scriptLoader->cancel();
>+    m_scriptLoader = nullptr;
>+
>+    Exception exception { SecurityError, "Scope URL should start with the given script URL"_s };
>+    ResourceError error { errorDomainWebKitInternal, 0, response.url(), "Scope URL should start with the given script URL"_s };
>+    m_client.jobFailedLoadingScript(*this, WTFMove(error), WTFMove(exception));
> }
> 
> void ServiceWorkerJob::notifyFinished()
>@@ -145,15 +153,17 @@ void ServiceWorkerJob::notifyFinished()
>     ASSERT(m_creationThread.ptr() == &Thread::current());
>     ASSERT(m_scriptLoader);
>     
>-    if (!m_scriptLoader->failed())
>-        m_client->jobFinishedLoadingScript(*this, m_scriptLoader->script(), m_scriptLoader->contentSecurityPolicy(), m_scriptLoader->referrerPolicy());
>-    else {
>-        auto& error =  m_scriptLoader->error();
>-        ASSERT(!error.isNull());
>-        m_client->jobFailedLoadingScript(*this, error, WTF::nullopt);
>+    auto scriptLoader = WTFMove(m_scriptLoader);
>+
>+    if (!scriptLoader->failed()) {
>+        m_client.jobFinishedLoadingScript(*this, scriptLoader->script(), scriptLoader->contentSecurityPolicy(), scriptLoader->referrerPolicy());
>+        return;
>     }
> 
>-    m_scriptLoader = nullptr;
>+    auto& error = scriptLoader->error();
>+    ASSERT(!error.isNull());
>+
>+    m_client.jobFailedLoadingScript(*this, error, Exception { error.isAccessControl() ? SecurityError : TypeError, "Script load failed"_s });
> }
> 
> void ServiceWorkerJob::cancelPendingLoad()
>diff --git a/Source/WebCore/workers/service/ServiceWorkerJob.h b/Source/WebCore/workers/service/ServiceWorkerJob.h
>index 0abd21943e1f80369b196726fe5cb0db1ab6b5da..a2d976367e84e3946851770eaaa9bb2c08707a8e 100644
>--- a/Source/WebCore/workers/service/ServiceWorkerJob.h
>+++ b/Source/WebCore/workers/service/ServiceWorkerJob.h
>@@ -46,13 +46,9 @@ class ScriptExecutionContext;
> enum class ServiceWorkerJobType;
> struct ServiceWorkerRegistrationData;
> 
>-class ServiceWorkerJob : public ThreadSafeRefCounted<ServiceWorkerJob>, public WorkerScriptLoaderClient {
>+class ServiceWorkerJob : public WorkerScriptLoaderClient {
> public:
>-    static Ref<ServiceWorkerJob> create(ServiceWorkerJobClient& client, RefPtr<DeferredPromise>&& promise, ServiceWorkerJobData&& jobData)
>-    {
>-        return adoptRef(*new ServiceWorkerJob(client, WTFMove(promise), WTFMove(jobData)));
>-    }
>-
>+    ServiceWorkerJob(ServiceWorkerJobClient&, RefPtr<DeferredPromise>&&, ServiceWorkerJobData&&);
>     WEBCORE_EXPORT ~ServiceWorkerJob();
> 
>     void failedWithException(const Exception&);
>@@ -64,7 +60,8 @@ public:
>     Identifier identifier() const { return m_jobData.identifier().jobIdentifier; }
> 
>     const ServiceWorkerJobData& data() const { return m_jobData; }
>-    DeferredPromise* promise() { return m_promise.get(); }
>+    bool hasPromise() const { return !!m_promise; }
>+    RefPtr<DeferredPromise> takePromise() { return WTFMove(m_promise); }
> 
>     void fetchScriptWithContext(ScriptExecutionContext&, FetchOptions::Cache);
> 
>@@ -73,13 +70,11 @@ public:
>     void cancelPendingLoad();
> 
> private:
>-    ServiceWorkerJob(ServiceWorkerJobClient&, RefPtr<DeferredPromise>&&, ServiceWorkerJobData&&);
>-
>     // WorkerScriptLoaderClient
>     void didReceiveResponse(unsigned long identifier, const ResourceResponse&) final;
>     void notifyFinished() final;
> 
>-    Ref<ServiceWorkerJobClient> m_client;
>+    ServiceWorkerJobClient& m_client;
>     ServiceWorkerJobData m_jobData;
>     RefPtr<DeferredPromise> m_promise;
> 
>diff --git a/Source/WebCore/workers/service/ServiceWorkerJobClient.h b/Source/WebCore/workers/service/ServiceWorkerJobClient.h
>index 4d43314cf7981222b096b184b0586b61b7f2083b..6c4818ef8fbf4f1c08c92cbd534407ae0ae49056 100644
>--- a/Source/WebCore/workers/service/ServiceWorkerJobClient.h
>+++ b/Source/WebCore/workers/service/ServiceWorkerJobClient.h
>@@ -50,12 +50,9 @@ public:
>     virtual void jobResolvedWithUnregistrationResult(ServiceWorkerJob&, bool unregistrationResult) = 0;
>     virtual void startScriptFetchForJob(ServiceWorkerJob&, FetchOptions::Cache) = 0;
>     virtual void jobFinishedLoadingScript(ServiceWorkerJob&, const String& script, const ContentSecurityPolicyResponseHeaders&, const String& referrerPolicy) = 0;
>-    virtual void jobFailedLoadingScript(ServiceWorkerJob&, const ResourceError&, Optional<Exception>&&) = 0;
>+    virtual void jobFailedLoadingScript(ServiceWorkerJob&, const ResourceError&, Exception&&) = 0;
> 
>     virtual SWServerConnectionIdentifier connectionIdentifier() = 0;
>-
>-    virtual void ref() = 0;
>-    virtual void deref() = 0;
> };
> 
> } // namespace WebCore
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index 7c6f04ef7e1fabcd8d397f4472fa59c431b49a6a..c4e26f1e8477d7fff6b8b4cee8bbc7e50a4f5044 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-01-25  Youenn Fablet  <youenn@apple.com>
>+
>+        ServiceWorkerJob should notify its client in case its job is cancelled
>+        https://bugs.webkit.org/show_bug.cgi?id=193747
>+        <rdar://problem/47498196>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt:
>+
> 2019-01-22  Oriol Brufau  <obrufau@igalia.com>
> 
>         [css-logical] Implement flow-relative margin, padding and border shorthands
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt
>index 38d0678faf3141d4bcae51f35b36e5db74fda922..aa75ec93844700f757cdc99dd362106a4fec98a8 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/registration-security-error.https-expected.txt
>@@ -3,7 +3,7 @@ PASS Registering same scope as the script directory without the last slash
> PASS Registration scope outside the script directory 
> PASS Registering scope outside domain 
> PASS Registering script outside domain 
>-FAIL Registering redirected script assert_throws: Registration of redirected script should fail. function "function () { throw e }" threw object "TypeError: Script URL https://localhost:9443/service-workers/service-worker/resources/redirect.py?Redirect=%2Fresources%2Fregistration-worker.js fetch resulted in error: Not allowed to follow a redirection while loading https://localhost:9443/service-workers/service-worker/resources/redirect.py?Redirect=%2Fresources%2Fregistration-worker.js" that is not a DOMException SecurityError: property "code" is equal to undefined, expected 18
>+PASS Registering redirected script 
> PASS Scope including parent-reference and not under the script directory 
> PASS Script URL including consecutive slashes 
> PASS Script URL is same-origin filesystem: URL

Actions: View | Formatted Diff | Diff

Attachments on bug 193747: 359971 | 360146 | 360180 | 360360 | 360602