Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
JSON Parse error: Unrecognized token '<' on web.autocad.com
https://bugs.webkit.org/show_bug.cgi?id=243418
<rdar://96271134>

Reviewed by Geoffrey Garen.

There were two issues on web.autocad.com:
1. The service worker intercepts certain types of loads and the service worker is meant to respond
   to those loads with content that was previously sent to it via postMessage(). If, when a load
   request comes in, the service worker didn't previously receive the data for this load via
   postMessage(), it will do a setInterval(1) and keep checking if it received a postMessage for
   this load or not. In practice, we see that a postMessage() may not come for a long time (verified
   on Chrome too), leaving the load request hanging. Chrome seems to allow this but in WebKit, we
   have a timeout timer that fires after 70 seconds and kills the service worker due to
   "unresponsiveness".
2. Even though the loads are properly intercepted by the service worker the first time, it is no
   longer the case on reload. The reason for this is that those loads come from a dedicated worker
   and the second around, this dedicated worker script is fetched from the memory cache instead of
   going to the network process. As a result of this, the dedicated worker is not properly marked
   as controlled by the service worker and all future loads by this dedicated worker won't go through
   the service worker. This break autocad since they rely on those loads to be intercepted by the
   service worker to serve particular JSON content. By bypassing the service worker, the loads go
   to the network and don't get the JSON content they expect.

To address issue 1, I am disabling the timeout timer for subresource loads and keeping it only for
main resource navigations. This allows us to support the use case of autocad, while keeping a
responsiveness check for navigations.

To address issue 2, I am updating mustReloadFromServiceWorkerOptions() to return true for dedicated
worker loads when there is an active service worker. This causes us to bypass the memory cache in
this case, making sure the dedicated worker is marked as controlled by the service worker, even on
reload.

With those two fixes, I no longer see any `Unrecognized token '<'` errors on web.autocad.com.

* LayoutTests/http/tests/workers/service/interception-from-dedicated-worker-on-reload-expected.txt: Added.
* LayoutTests/http/tests/workers/service/interception-from-dedicated-worker-on-reload.html: Added.
* LayoutTests/http/tests/workers/service/resources/foo.txt: Added.
* LayoutTests/http/tests/workers/service/resources/interception-from-dedicated-worker-on-reload-cacheable-worker.py: Added.
* LayoutTests/http/tests/workers/service/resources/interception-from-dedicated-worker-on-reload-iframe.html: Added.
* LayoutTests/http/tests/workers/service/resources/interception-from-dedicated-worker-on-reload-sw.js: Added.
(event.event.request.url.indexOf):
* Source/WebCore/loader/cache/CachedResourceLoader.cpp:
(WebCore::mustReloadFromServiceWorkerOptions):
(WebCore::CachedResourceLoader::determineRevalidationPolicy const):
* Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:
(WebKit::ServiceWorkerFetchTask::ServiceWorkerFetchTask):

Canonical link: https://commits.webkit.org/253037@main
  • Loading branch information
cdumez committed Aug 2, 2022
1 parent ac691ca commit 6087b92
Show file tree
Hide file tree
Showing 8 changed files with 105 additions and 4 deletions.
@@ -0,0 +1,6 @@
Tests that loads from dedicated worker still get intercepted by the service worker after a reload.

PASS: Load from dedicated worker was properly intercepted by the service worker
* Reloading...
PASS: Load from dedicated worker was properly intercepted by the service worker

@@ -0,0 +1,48 @@
<!DOCTYPE html>
<html>
<body>
<script src="resources/sw-test-pre.js"></script>
<p>Tests that loads from dedicated worker still get intercepted by the service worker after a reload.</p>
<script>

const scopeURL = "/workers/service/resources/";

let didReload = false;
let iframe = null;

function createTestIframe()
{
if (iframe)
iframe.remove();

iframe = document.createElement("iframe");
iframe.src = "resources/interception-from-dedicated-worker-on-reload-iframe.html";
document.body.appendChild(iframe);
}

function receivedFetchContentFromDedicatedWorker(content)
{
if (content === "INTERCEPTED")
log("PASS: Load from dedicated worker was properly intercepted by the service worker");
else
log("FAIL: Load from dedicated worker was not intercepted by the service worker");

if (!didReload) {
didReload = true;
log("* Reloading...");
createTestIframe();
return;
}
finishSWTest();
}

async function test()
{
let registration = await registerAndWaitForActive("resources/interception-from-dedicated-worker-on-reload-sw.js", scopeURL);
createTestIframe();
}

test();
</script>
</body>
</html>
1 change: 1 addition & 0 deletions LayoutTests/http/tests/workers/service/resources/foo.txt
@@ -0,0 +1 @@
FOO
@@ -0,0 +1,19 @@
#!/usr/bin/env python3

import os
import sys

max_age = 12 * 31 * 24 * 60 * 60

sys.stdout.write(
f'Cache-Control: public, max-age={max_age}\r\n'
'Content-Type: text/javascript\r\n\r\n'
)

sys.stdout.write(
'fetch("foo.txt").then((response) => {\n'
' response.text().then((responseText) => {\n'
' postMessage(responseText);\n'
' });\n'
'});\n'
)
@@ -0,0 +1,11 @@
<!DOCTYPE html>
<html>
<body>
<script>
let worker = new Worker("interception-from-dedicated-worker-on-reload-cacheable-worker.py");
worker.onmessage = (message) => {
top.receivedFetchContentFromDedicatedWorker(message.data);
};
</script>
</body>
</html>
@@ -0,0 +1,6 @@
self.addEventListener("fetch", (event) => {
if (event.request.url.indexOf("foo.txt") !== -1) {
event.respondWith(new Response("INTERCEPTED", { status: 200, statusText: "Hello from service worker", headers: [["Hello", "World"]] }));
return;
}
});
11 changes: 9 additions & 2 deletions Source/WebCore/loader/cache/CachedResourceLoader.cpp
Expand Up @@ -1188,8 +1188,15 @@ static void logResourceRevalidationDecision(CachedResource::RevalidationDecision
}

#if ENABLE(SERVICE_WORKER)
static inline bool mustReloadFromServiceWorkerOptions(const ResourceLoaderOptions& options, const ResourceLoaderOptions& cachedOptions)
static inline bool mustReloadFromServiceWorkerOptions(Document* document, const ResourceLoaderOptions& options, const ResourceLoaderOptions& cachedOptions)
{
// When Loading a worker script and there is an active service worker, it is important to bypass the memory cache for the fetching of the script. If we
// don't then the worker won't be controlled by the service worker and the loads from the worker won't be intercepted by the service worker.

This comment has been minimized.

Copy link
@youennf

youennf Aug 17, 2022

Contributor

What we do in DocumentLoader is to call matchRegistration explicitly when getting a resource from memory cache.
We could and probably should have done this in WorkerScriptLoader as well.

if ((cachedOptions.destination == FetchOptions::Destination::Worker || cachedOptions.destination == FetchOptions::Destination::Sharedworker)
&& options.serviceWorkersMode != ServiceWorkersMode::None && document && document->activeServiceWorker()) {
return true;
}

// FIXME: We should validate/specify this behavior.
if (options.serviceWorkerRegistrationIdentifier != cachedOptions.serviceWorkerRegistrationIdentifier)
return true;
Expand All @@ -1215,7 +1222,7 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
return Reload;

#if ENABLE(SERVICE_WORKER)
if (mustReloadFromServiceWorkerOptions(cachedResourceRequest.options(), existingResource->options())) {
if (mustReloadFromServiceWorkerOptions(document(), cachedResourceRequest.options(), existingResource->options())) {
LOG(ResourceLoading, "CachedResourceLoader::determineRevalidationPolicy reloading because selected service worker may differ");
return Reload;
}
Expand Down
Expand Up @@ -85,13 +85,16 @@ ServiceWorkerFetchTask::ServiceWorkerFetchTask(WebSWServerConnection& swServerCo
, m_serverConnectionIdentifier(serverConnectionIdentifier)
, m_serviceWorkerIdentifier(serviceWorkerIdentifier)
, m_currentRequest(WTFMove(request))
, m_timeoutTimer(makeUnique<Timer>(*this, &ServiceWorkerFetchTask::timeoutTimerFired))
, m_serviceWorkerRegistrationIdentifier(registration.identifier())
, m_shouldSoftUpdate(registration.shouldSoftUpdate(loader.parameters().options))
{
SWFETCH_RELEASE_LOG("ServiceWorkerFetchTask: (serverConnectionIdentifier=%" PRIu64 ", serviceWorkerRegistrationIdentifier=%" PRIu64 ", serviceWorkerIdentifier=%" PRIu64 ", %d)", m_serverConnectionIdentifier.toUInt64(), m_serviceWorkerRegistrationIdentifier.toUInt64(), m_serviceWorkerIdentifier.toUInt64(), isWorkerReady);

m_timeoutTimer->startOneShot(loader.connectionToWebProcess().networkProcess().serviceWorkerFetchTimeout());
// We only do the timeout logic for main document navigations because it is not Web-compatible to do so for subresources.
if (loader.parameters().request.requester() == WebCore::ResourceRequest::Requester::Main) {
m_timeoutTimer = makeUnique<Timer>(*this, &ServiceWorkerFetchTask::timeoutTimerFired);
m_timeoutTimer->startOneShot(loader.connectionToWebProcess().networkProcess().serviceWorkerFetchTimeout());
}

bool shouldDoNavigationPreload = session && isNavigationRequest(loader.parameters().options.destination) && loader.originalRequest().httpMethod() == "GET"_s;
if (shouldDoNavigationPreload && (!isWorkerReady || registration.navigationPreloadState().enabled)) {
Expand Down

0 comments on commit 6087b92

Please sign in to comment.