WebKit Bugzilla
Attachment 360360 Details for
Bug 193747
: Refactor ServiceWorkerJob management by ServiceWorkerContainer to make it more memory safe
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-193747-20190128111940.patch (text/plain), 29.10 KB, created by
youenn fablet
on 2019-01-28 11:19:45 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
youenn fablet
Created:
2019-01-28 11:19:45 PST
Size:
29.10 KB
patch
obsolete
>Subversion Revision: 240583 >diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog >index 0fa3a3b552bee2ff26367ad7866b47aa40564c40..24288667657fc50175607b36134e51b8e165bf37 100644 >--- a/Source/WebCore/ChangeLog >+++ b/Source/WebCore/ChangeLog >@@ -1,3 +1,59 @@ >+2019-01-28 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 ServiceWorkerContainer job map. >+ This allows to ensure that these jobs do not stay forever. >+ Before the patch, the jobs map was never cleared, which is creating a ref cycle whenever a job is not succesfull. >+ >+ Before the patch, unsetPendingActivity was only called for successful jobs finishing. >+ In case of failing loads, ServiceWorkerContainer would leak. >+ 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 corresponding 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: >+ * workers/service/server/SWServerJobQueue.cpp: >+ (WebCore::SWServerJobQueue::scriptFetchFinished): >+ > 2019-01-28 Zalan Bujtas <zalan@apple.com> > > [LFC][MarginCollapsing][Quirks] Quirk margin values get propagated through margin collapsing >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..81b811c075297b00064d642feb44d9a464c72db3 100644 >--- a/Source/WebCore/workers/service/ServiceWorkerJob.h >+++ b/Source/WebCore/workers/service/ServiceWorkerJob.h >@@ -27,6 +27,7 @@ > > #if ENABLE(SERVICE_WORKER) > >+#include "JSDOMPromiseDeferred.h" > #include "ResourceResponse.h" > #include "ServiceWorkerJobClient.h" > #include "ServiceWorkerJobData.h" >@@ -46,13 +47,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 +61,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 +71,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/Source/WebCore/workers/service/server/SWServerJobQueue.cpp b/Source/WebCore/workers/service/server/SWServerJobQueue.cpp >index 9ec1ba686583ec8c18ff617f61436124ef585071..39d7d3ebefd999ed470c305acd759ed3451f87c3 100644 >--- a/Source/WebCore/workers/service/server/SWServerJobQueue.cpp >+++ b/Source/WebCore/workers/service/server/SWServerJobQueue.cpp >@@ -64,7 +64,8 @@ void SWServerJobQueue::scriptFetchFinished(SWServer::Connection& connection, con > auto& job = firstJob(); > > auto* registration = m_server.getRegistration(m_registrationKey); >- ASSERT(registration); >+ if (!registration) >+ return; > > auto* newestWorker = registration->getNewestWorker(); > >diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog >index ec057412ce368bd74036ff1436c6a5caf449c7af..f9e451b41fe02c4f40b2e9218ea952362a612454 100644 >--- a/LayoutTests/imported/w3c/ChangeLog >+++ b/LayoutTests/imported/w3c/ChangeLog >@@ -1,3 +1,13 @@ >+2019-01-28 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-24 Charles Vazac <cvazac@akamai.com> > > Implement PerformanceObserver.supportedEntryTypes >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
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=360360">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 193747
:
359971
|
360146
|
360180
|
360360
|
360602