Bug 176231 – Move ServiceWorkerJob from FetchLoader to ThreadableLoader

• Component: WebCore Misc.
• Status: REOPENED

[Brady Eidson]

Move ServiceWorkerJob from FetchLoader to WorkerScriptLoader

[Brady Eidson]

Nope, WorkerScriptLoader is also not currently acceptable.

For now, using ThreadableLoader.

Rename:
Move ServiceWorkerJob from FetchLoader to ThreadableLoader

[youenn fablet]

(In reply to Brady Eidson from comment #1)
> Nope, WorkerScriptLoader is also not currently acceptable.

What is the issue with WorkerScriptLoader?

> For now, using ThreadableLoader.
> 
> Rename:
> Move ServiceWorkerJob from FetchLoader to ThreadableLoader

That will not really help.
Might be better to postpone this change until there is a good plan.

[Brady Eidson]

Created attachment 319632 [details]
Patch

[Brady Eidson]

(In reply to youenn fablet from comment #2)
> (In reply to Brady Eidson from comment #1)
> > Nope, WorkerScriptLoader is also not currently acceptable.
> 
> What is the issue with WorkerScriptLoader?
> 
> > For now, using ThreadableLoader.
> > 
> > Rename:
> > Move ServiceWorkerJob from FetchLoader to ThreadableLoader
> 
> That will not really help.
> Might be better to postpone this change until there is a good plan.

It gets us closer.

It's definitely not right, but it's "less wrong"

I definitely want to remove notions of using FetchLoader before I move on. :)

[Brady Eidson]

(In reply to youenn fablet from comment #2)
> (In reply to Brady Eidson from comment #1)
> > Nope, WorkerScriptLoader is also not currently acceptable.
> 
> What is the issue with WorkerScriptLoader?

You can only feed it a URL and can't get the data as it streams in.

Additionally, it seems to be designed to be used *from Workers* but whatever we settle on needs to be used from both Documents and Workers.

[youenn fablet]

Comment on attachment 319632 [details]
Patch

r=me.
There is no need to create a FetchRequest object here so this is better like this for now.

Since FetchLoader.h/FetchLoaderClient.h are no longer used in WebKit2, they could be reverted to not being "Private" headers.

View in context: https://bugs.webkit.org/attachment.cgi?id=319632&action=review

> Source/WebCore/loader/ThreadableLoader.h:46
> +enum PreflightPolicy {

Would be nice as enum class, but hey...

[Brady Eidson]

Created attachment 319640 [details]
Patch

[Brady Eidson]

Comment on attachment 319640 [details]
Patch

The iOS-sim failure is an infrastructure issue, not an issue with this patch

[WebKit Commit Bot]

Comment on attachment 319640 [details]
Patch

Clearing flags on attachment: 319640

Committed r221495: <http://trac.webkit.org/changeset/221495>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

After this patch landed there were multiple Layout test crashing on El Capitan WK2:
  imported/w3c/web-platform-tests/FileAPI/idlharness.html
  imported/w3c/web-platform-tests/fetch/api/policies/referrer-no-referrer-worker.html
  imported/w3c/web-platform-tests/fetch/api/policies/referrer-origin-when-cross-origin-worker.html
  imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/cache-storage-match.https.html
  imported/w3c/web-platform-tests/streams/piping/close-propagation-backward.sharedworker.html
  imported/w3c/web-platform-tests/streams/readable-streams/bad-underlying-sources.dedicatedworker.html

https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29/builds/2841
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221495%20(2841)/results.html

Crash:
https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221495%20(2841)/imported/w3c/web-platform-tests/fetch/api/policies/referrer-no-referrer-worker-crash-log.txt

[Ryan Haddad]

(In reply to Matt Lewis from comment #11)
> After this patch landed there were multiple Layout test crashing on El
> Capitan WK2:
>   imported/w3c/web-platform-tests/FileAPI/idlharness.html
>  
> imported/w3c/web-platform-tests/fetch/api/policies/referrer-no-referrer-
> worker.html
>  
> imported/w3c/web-platform-tests/fetch/api/policies/referrer-origin-when-
> cross-origin-worker.html
>  
> imported/w3c/web-platform-tests/service-workers/cache-storage/serviceworker/
> cache-storage-match.https.html
>  
> imported/w3c/web-platform-tests/streams/piping/close-propagation-backward.
> sharedworker.html
>  
> imported/w3c/web-platform-tests/streams/readable-streams/bad-underlying-
> sources.dedicatedworker.html
> 
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20WK2%20%28Tests%29/builds/2841
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221495%20(2841)/results.html
> 
> Crash:
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221495%20(2841)/imported/w3c/
> web-platform-tests/fetch/api/policies/referrer-no-referrer-worker-crash-log.
> txt
Specifically, this is an assertion failure:
ASSERTION FAILED: m_loader
/Volumes/Data/slave/elcapitan-debug/build/Source/WebCore/workers/service/ServiceWorkerJob.cpp(136) : virtual void WebCore::ServiceWorkerJob::didFail(const WebCore::ResourceError &)
1   0x1149ff8c0 WTFCrash

[Ryan Haddad]

Reverted r221495 for reason:

This change introduced  assertion failures on El Capitan Debug WK2.

Committed r221646: <http://trac.webkit.org/changeset/221646>