Bug 176179 – Add (entirely incorrect) fetching of ServiceWorker scripts

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Add (entirely incorrect) fetching of ServiceWorker scripts

After the basic infrastructure is in place, we'll make it proper in a followup.

[Brady Eidson]

Created attachment 319522 [details]
Patch

[Andy Estes]

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.

[Brady Eidson]

Created attachment 319533 [details]
Patch fo landing

[WebKit Commit Bot]

Comment on attachment 319533 [details]
Patch fo landing

Clearing flags on attachment: 319533

Committed r221461: <http://trac.webkit.org/changeset/221461>

[youenn fablet]

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>?

[Brady Eidson]

(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.

[Brady Eidson]

> 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.

[youenn fablet]

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.

[youenn fablet]

See for instance https://fetch.spec.whatwg.org/#concept-request-destination

[youenn fablet]

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.

[Brady Eidson]

Yup, going with WorkerScriptLoader I think.

[Ryan Haddad]

Reverted r221461 for reason:

The LayoutTest added with this change crashes under GuardMalloc.

Committed r221699: <http://trac.webkit.org/changeset/221699>

[Ryan Haddad]

(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>

[Brady Eidson]

Created attachment 322863 [details]
New patch

Try again, new loader like Youenn suggested.

[Brady Eidson]

Created attachment 322885 [details]
Patch

[Brady Eidson]

Created attachment 322886 [details]
Patch

[Brady Eidson]

(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.

[WebKit Commit Bot]

Comment on attachment 322886 [details]
Patch

Clearing flags on attachment: 322886

Committed r222980: <http://trac.webkit.org/changeset/222980>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34856978>