Bug 181381 – SWClientConnection should not keep references to service worker jobs

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

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)

[youenn fablet]

Created attachment 330677 [details]
Patch

[youenn fablet]

Created attachment 330694 [details]
Patch for landing

[youenn fablet]

Created attachment 330699 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/36350422>

[Chris Dumez]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 330734 [details]
Patch for landing

[youenn fablet]

(In reply to youenn fablet from comment #7)
> Created attachment 330734 [details]
> Patch for landing

Updated according review.

[WebKit Commit Bot]

Comment on attachment 330734 [details]
Patch for landing

Clearing flags on attachment: 330734

Committed r226540: <https://trac.webkit.org/changeset/226540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 181422

[youenn fablet]

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.

[youenn fablet]

Created attachment 330808 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 330808 [details]
Patch for landing

Clearing flags on attachment: 330808

Committed r226626: <https://trac.webkit.org/changeset/226626>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.