Bug 176179

Summary: Add (entirely incorrect) fetching of ServiceWorker scripts
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, buildbot, commit-queue, dbates, jrburke, mkwst, ryanhaddad, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=174541
Attachments:
Description Flags
Patch
aestes: review+
Patch fo landing
none
New patch
none
Patch
none
Patch none

Description Brady Eidson 2017-08-31 13:04:29 PDT
Add (entirely incorrect) fetching of ServiceWorker scripts

After the basic infrastructure is in place, we'll make it proper in a followup.
Comment 1 Brady Eidson 2017-08-31 14:26:46 PDT
Created attachment 319522 [details]
Patch
Comment 2 Andy Estes 2017-08-31 14:41:01 PDT
Comment on attachment 319522 [details]
Patch

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

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:192
> +        LOG_ERROR("ServiceWorkerContainer::jobResolvedWithRegistration called but the containers ScriptExecutionContext is gone");

containers => container's

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:193
> +        m_swConnection->failedFetchingScript(job, { errorDomainWebKitInternal, 0, job.data().scriptURL, ASCIILiteral("Attempt to fetch service worker script with no script execution context") });

This error says "script execution context", but the one above says "ScriptExecutionContext". I'd be consistent.
Comment 3 Brady Eidson 2017-08-31 15:39:06 PDT
Created attachment 319533 [details]
Patch fo landing
Comment 4 WebKit Commit Bot 2017-08-31 17:12:56 PDT
Comment on attachment 319533 [details]
Patch fo landing

Clearing flags on attachment: 319533

Committed r221461: <http://trac.webkit.org/changeset/221461>
Comment 5 youenn fablet 2017-08-31 21:10:39 PDT
Comment on attachment 319533 [details]
Patch fo landing

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

> Source/WebCore/workers/service/ServiceWorkerJob.cpp:85
> +    m_fetchLoader = std::make_unique<FetchLoader>(*this, nullptr);

FetchLoader should not be used for the purpose of loading scripts.
It should be kept internal to fetch API and not to other kind of fetch except for raw resources.

WorkerScriptLoader is the closest to classic script loading and CachedModuleScriptLoader/CachedScriptFetcher the closest to module loading.

I am unclear how is used this data.
If the goal is to send it back to the storage process, it seems unnecessary to load the data on the worker thread.
We can probably grab in the worker thread the parameters necessary for the load, and then continue the networking in the main thread, like done by WorkerThreadableLoader.

> Source/WebCore/workers/service/ServiceWorkerJob.h:84
> +    std::optional<Ref<SharedBuffer>> m_scriptData;

Why optional and not RefPtr<SharedBuffer>?
Comment 6 Brady Eidson 2017-08-31 22:00:02 PDT
(In reply to youenn fablet from comment #5)
> Comment on attachment 319533 [details]
> Patch fo landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319533&action=review
> 
> > Source/WebCore/workers/service/ServiceWorkerJob.cpp:85
> > +    m_fetchLoader = std::make_unique<FetchLoader>(*this, nullptr);
> 
> FetchLoader should not be used for the purpose of loading scripts.
> It should be kept internal to fetch API and not to other kind of fetch
> except for raw resources.

The ServiceWorker spec says "perform a fetch" the same way the Fetch API does.

AFAIK, FetchLoader is our implementation of this.

> WorkerScriptLoader is the closest to classic script loading and
> CachedModuleScriptLoader/CachedScriptFetcher the closest to module loading.

AFAIK these are WebCore internal loading concepts and do not implement the semantics of "Fetch"

Is it not true that "Fetch" has special semantics over "load a resource the old fashioned way"?

> I am unclear how is used this data.
> If the goal is to send it back to the storage process, it seems unnecessary
> to load the data on the worker thread.

It is sent back to the storage process, but it is not loaded any given worker thread.

> We can probably grab in the worker thread the parameters necessary for the
> load, and then continue the networking in the main thread, like done by
> WorkerThreadableLoader.

Nothing in this patch performs a load on a worker thread - It's all in the main thread.

(Though it definitely is set up for future loading from a ServiceWorker, in addition to a main thread Document context)

> > Source/WebCore/workers/service/ServiceWorkerJob.h:84
> > +    std::optional<Ref<SharedBuffer>> m_scriptData;
> 
> Why optional and not RefPtr<SharedBuffer>?

Yah, leftover artifact from a period of development of this patch.
Comment 7 Brady Eidson 2017-08-31 22:04:13 PDT
> WorkerScriptLoader is the closest to classic script loading and
> CachedModuleScriptLoader/CachedScriptFetcher the closest to module loading.

CachedFoo* are insufficient as this code *will* need to be executable from a worker thread in the future.

But, anyways, tl;dr - If "perform the fetch" doesn't have special semantics that rely on FetchLoader, I'll gladly use something else.

I was under the impression "perform the fetch" has specific meanings that point me towards FetchLoader, but if not that's fine.
Comment 8 youenn fablet 2017-09-01 08:40:22 PDT
Fetch algorithm is taking into account the nature of the load,. This impacts for instance the request destination.

FetchLoader is only for raw resources, which is good for things like fetch API/XHR event source... but is not good for images, fonts or scripts.

Looking at the spec, SW is fetching a classic worker script or a module worker script tree, depending on the worker type.
That should ultimately end up in calling CachedResourceLoader::requestScript.

Or we could punt on future WorkerScriptLoader refactoring so that WorkerScriptLoader would be responsible to implement these classic/module worker script fetching.
Currently, it is not doing this correctly but this could probably be improved.
Comment 9 youenn fablet 2017-09-01 08:41:08 PDT
See for instance https://fetch.spec.whatwg.org/#concept-request-destination
Comment 10 youenn fablet 2017-09-01 08:44:25 PDT
Looking at ScriptModuleLoader::fetch, CachedModuleScriptLoader is responsible to implement https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script.

Given the current API of these loaders and SW implementation, it does not seem to align well though.
Comment 11 Brady Eidson 2017-09-01 10:46:32 PDT
Yup, going with WorkerScriptLoader I think.
Comment 12 Ryan Haddad 2017-09-06 13:40:07 PDT
Reverted r221461 for reason:

The LayoutTest added with this change crashes under GuardMalloc.

Committed r221699: <http://trac.webkit.org/changeset/221699>
Comment 13 Ryan Haddad 2017-09-06 13:41:23 PDT
(In reply to Ryan Haddad from comment #12)
> Reverted r221461 for reason:
> 
> The LayoutTest added with this change crashes under GuardMalloc.
> 
> Committed r221699: <http://trac.webkit.org/changeset/221699>
<rdar://problem/34209643>
Comment 14 Brady Eidson 2017-10-05 11:10:05 PDT
Created attachment 322863 [details]
New patch

Try again, new loader like Youenn suggested.
Comment 15 Brady Eidson 2017-10-05 12:56:23 PDT
Created attachment 322885 [details]
Patch
Comment 16 Brady Eidson 2017-10-05 13:01:21 PDT
Created attachment 322886 [details]
Patch
Comment 17 Brady Eidson 2017-10-05 14:09:58 PDT
(In reply to Ryan Haddad from comment #12)
> Reverted r221461 for reason:
> 
> The LayoutTest added with this change crashes under GuardMalloc.
> 
> Committed r221699: <http://trac.webkit.org/changeset/221699>

The new patch running through EWS does not crash under GM based on my `run-webkit-tests -g` command.
Comment 18 WebKit Commit Bot 2017-10-06 09:20:08 PDT
Comment on attachment 322886 [details]
Patch

Clearing flags on attachment: 322886

Committed r222980: <http://trac.webkit.org/changeset/222980>
Comment 19 WebKit Commit Bot 2017-10-06 09:20:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2017-10-06 09:20:40 PDT
<rdar://problem/34856978>