Bug 176231

Summary: Move ServiceWorkerJob from FetchLoader to ThreadableLoader
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: REOPENED ---    
Severity: Normal CC: aestes, buildbot, cdumez, commit-queue, dbates, japhet, jlewis3, jrburke, olivier, ryanhaddad, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=174541
Attachments:
Description Flags
Patch
none
Patch none

Description Brady Eidson 2017-09-01 10:46:55 PDT
Move ServiceWorkerJob from FetchLoader to WorkerScriptLoader
Comment 1 Brady Eidson 2017-09-01 11:15:42 PDT
Nope, WorkerScriptLoader is also not currently acceptable.

For now, using ThreadableLoader.

Rename:
Move ServiceWorkerJob from FetchLoader to ThreadableLoader
Comment 2 youenn fablet 2017-09-01 11:25:43 PDT
(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.
Comment 3 Brady Eidson 2017-09-01 12:44:37 PDT
Created attachment 319632 [details]
Patch
Comment 4 Brady Eidson 2017-09-01 12:50:49 PDT
(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. :)
Comment 5 Brady Eidson 2017-09-01 12:52:01 PDT
(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.
Comment 6 youenn fablet 2017-09-01 13:15:03 PDT
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...
Comment 7 Brady Eidson 2017-09-01 13:20:32 PDT
Created attachment 319640 [details]
Patch
Comment 8 Brady Eidson 2017-09-01 14:04:02 PDT
Comment on attachment 319640 [details]
Patch

The iOS-sim failure is an infrastructure issue, not an issue with this patch
Comment 9 WebKit Commit Bot 2017-09-01 14:33:06 PDT
Comment on attachment 319640 [details]
Patch

Clearing flags on attachment: 319640

Committed r221495: <http://trac.webkit.org/changeset/221495>
Comment 10 WebKit Commit Bot 2017-09-01 14:33:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Matt Lewis 2017-09-05 15:25:21 PDT
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
Comment 12 Ryan Haddad 2017-09-05 15:29:35 PDT
(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
Comment 13 Ryan Haddad 2017-09-05 16:33:55 PDT
Reverted r221495 for reason:

This change introduced  assertion failures on El Capitan Debug WK2.

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