ServiceWorkerJob can be destroyed in the wrong thread, as shown in https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r226473%20(4864)/com.apple.WebKit.WebContent.Development-67744-crash-log.txt Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000114eaaaa4 WTFCrash + 36 (Assertions.cpp:272) 1 com.apple.WebCore 0x0000000107f6feae WebCore::ServiceWorkerJob::~ServiceWorkerJob() + 110 (ServiceWorkerJob.cpp:51) 2 com.apple.WebCore 0x0000000107f6ff55 WebCore::ServiceWorkerJob::~ServiceWorkerJob() + 21 (ServiceWorkerJob.cpp:52) 3 com.apple.WebCore 0x0000000107f6ff79 WebCore::ServiceWorkerJob::~ServiceWorkerJob() + 25 (ServiceWorkerJob.cpp:50) 4 com.apple.WebCore 0x0000000107f6508f WTF::ThreadSafeRefCounted<WebCore::ServiceWorkerJob>::deref() const + 79 (ThreadSafeRefCounted.h:71) 5 com.apple.WebCore 0x0000000107f65035 void WTF::derefIfNotNull<WebCore::ServiceWorkerJob>(WebCore::ServiceWorkerJob*) + 53 (RefPtr.h:46) 6 com.apple.WebCore 0x0000000107f64ff9 WTF::RefPtr<WebCore::ServiceWorkerJob, WTF::DumbPtrTraits<WebCore::ServiceWorkerJob> >::~RefPtr() + 41 (RefPtr.h:70) 7 com.apple.WebCore 0x0000000107f5bcd5 WTF::RefPtr<WebCore::ServiceWorkerJob, WTF::DumbPtrTraits<WebCore::ServiceWorkerJob> >::~RefPtr() + 21 (RefPtr.h:70) 8 com.apple.WebCore 0x0000000107f486e9 WebCore::SWClientConnection::registrationJobResolvedInServer(WebCore::ServiceWorkerJobDataIdentifier const&, WebCore::ServiceWorkerRegistrationData&&, WebCore::ShouldNotifyWhenResolved) + 553 (SWClientConnection.cpp:110)
Created attachment 330677 [details] Patch
Created attachment 330694 [details] Patch for landing
Created attachment 330699 [details] Patch
<rdar://problem/36350422>
Comment on attachment 330699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=330699&action=review r=me with comments. > Source/WebCore/workers/service/SWClientConnection.cpp:48 > +void SWClientConnection::scheduleJob(ServiceWorkerJobIdentifier jobIdentifier, DocumentOrWorkerIdentifier contextIdentifier, const ServiceWorkerJobData& jobData) I do not believe you need the jobIdentifier parameters, you can get it from the jobData, no? > Source/WebCore/workers/service/SWClientConnection.cpp:115 > + failedFetchingScript(jobIdentifier, registrationKey, ResourceError { errorDomainWebKitInternal, 0, registrationKey.scope(), ASCIILiteral("Failed to fetch service worker script") }); Seems odd to use the scope here. The URL we tried to fetch is the scriptURL so we should use that URL in the error message. > Source/WebCore/workers/service/SWClientConnection.cpp:253 > auto jobs = WTFMove(m_scheduledJobs); These are no longer jobs but jobSources > Source/WebCore/workers/service/SWClientConnection.h:107 > + enum class ShouldTakeJob { No, Yes }; I find this shouldTakeJob a bit confusing. We do not really take the job, we remove a job source from the HashMap of job sources. Maybe this could be a enum class IsJobComplete { No, Yes }; and then we'd remove the source from the map if the job is complete? > Source/WebCore/workers/service/SWClientConnection.h:110 > + HashMap<ServiceWorkerJobIdentifier, DocumentOrWorkerIdentifier> m_scheduledJobs; m_scheduledJobSources? since it no longer contains jobs but source identifiers.
Comment on attachment 330699 [details] Patch Thanks for the review. View in context: https://bugs.webkit.org/attachment.cgi?id=330699&action=review >> Source/WebCore/workers/service/SWClientConnection.cpp:115 >> + failedFetchingScript(jobIdentifier, registrationKey, ResourceError { errorDomainWebKitInternal, 0, registrationKey.scope(), ASCIILiteral("Failed to fetch service worker script") }); > > Seems odd to use the scope here. The URL we tried to fetch is the scriptURL so we should use that URL in the error message. We no longer have the scriptURL information since we do not have any job/jobData anymore. We could store the job data or the script URL in the job map but that does not seem appealing just for this purpose. I will update the ResourceError by passing an empty URL and adding the scope URL in the error message.
Created attachment 330734 [details] Patch for landing
(In reply to youenn fablet from comment #7) > Created attachment 330734 [details] > Patch for landing Updated according review.
Comment on attachment 330734 [details] Patch for landing Clearing flags on attachment: 330734 Committed r226540: <https://trac.webkit.org/changeset/226540>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 181422
Patch was rolled out because apparently it is breaking basic browsing. My intuition is that it may be related to SQL SW persistency changes done in https://bugs.webkit.org/show_bug.cgi?id=181385. I will reland this one as is for now.
Created attachment 330808 [details] Patch for landing
Comment on attachment 330808 [details] Patch for landing Clearing flags on attachment: 330808 Committed r226626: <https://trac.webkit.org/changeset/226626>