Bug 176179

Created attachment 319522 [details] Patch

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.

Created attachment 319533 [details] Patch fo landing

Comment on attachment 319533 [details] Patch fo landing Clearing flags on attachment: 319533 Committed r221461: <http://trac.webkit.org/changeset/221461>

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

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

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

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.

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

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.

Yup, going with WorkerScriptLoader I think.

Reverted r221461 for reason: The LayoutTest added with this change crashes under GuardMalloc. Committed r221699: <http://trac.webkit.org/changeset/221699>

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

Created attachment 322863 [details] New patch Try again, new loader like Youenn suggested.

Created attachment 322885 [details] Patch

Created attachment 322886 [details] Patch

(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 on attachment 322886 [details] Patch Clearing flags on attachment: 322886 Committed r222980: <http://trac.webkit.org/changeset/222980>

All reviewed patches have been landed. Closing bug.

<rdar://problem/34856978>