Bug 235710 – redirectCount returns 0 when using a Service Worker

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[Daniel Schulz]

Created attachment 450130 [details]
All the files referenced in the bug report

The Redirect Counter of the Performance API returns 0 after a redirect, when having a Service Worker installed.
I've hosted the code in a test environment and attached everything in a zip as well.
Tested in Safari 15.2 and 15.4TP on MacOS 12.1 on a MacBook Pro M1 2020.


Steps to Reproduce

1)
- Go to https://sw-lifecycle-test.app.baqend.com/v1/code/redirect
- this redirects to https://sw-lifecycle-test.app.baqend.com/redirectTest/redirect.html
- on redirected site, execute in console:
	performance.navigation.redirectCount // returns 1
- This is the expected behaviour

2)
- Go to https://sw-lifecycle-test.app.baqend.com/redirectTest/test.html
- this installs a SW and redirects to https://sw-lifecycle-test.app.baqend.com/redirectTest/redirect.html
- on redirected site, execute in console:
	performance.navigation.redirectCount // returns 0
- This is unexpected behaviour. It should return 1.

3)
Repeat step 1)
- performance.navigation.redirectCount // still returns 1

4)
Uninstall Service Worker, Repeat 1)
- performance.navigation.redirectCount // returns 0 again

[Daniel Schulz]

Amendment: My reported return values of step 3) and 4) are swapped. Sorry for the inconvenience. Is supposed to be:

3)
Repeat step 1)
- performance.navigation.redirectCount // still returns 0

4)
Uninstall Service Worker, Repeat 1)
- performance.navigation.redirectCount // returns 1 again

[Radar WebKit Bug Importer]

<rdar://problem/88432190>

[youenn fablet]

Created attachment 454462 [details]
Patch

[youenn fablet]

Created attachment 454480 [details]
Patch

[youenn fablet]

@kal, gtk is crashing with the following trace:
Thread 1 (Thread 0x7fc96cd06e80 (LWP 322757)):
#0  0x00007fc975f1136b in WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#1  0x00007fc975f113db in WebCore::ResourceRequestBase::~ResourceRequestBase() () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#2  0x00007fc9760ce0a6 in WebKit::NetworkLoad::~NetworkLoad() () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#3  0x00007fc9760ce379 in WebKit::NetworkLoad::~NetworkLoad() () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#4  0x00007fc9761a0997 in WebKit::ServiceWorkerNavigationPreloader::didComplete() () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#5  0x00007fc9760cdc8b in WebKit::NetworkLoad::willPerformHTTPRedirection(WebCore::ResourceResponse&&, WebCore::ResourceRequest&&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&) () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
#6  0x00007fc976215857 in WebKit::NetworkDataTaskSoup::continueHTTPRedirection() () at /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0

Any idea what might be the issue?

[Carlos Garcia Campos]

(In reply to youenn fablet from comment #5)
> @kal, gtk is crashing with the following trace:
> Thread 1 (Thread 0x7fc96cd06e80 (LWP 322757)):
> #0  0x00007fc975f1136b in WTF::Vector<WTF::String, 0ul,
> WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::~Vector() () at
> /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
> #1  0x00007fc975f113db in
> WebCore::ResourceRequestBase::~ResourceRequestBase() () at
> /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
> #2  0x00007fc9760ce0a6 in WebKit::NetworkLoad::~NetworkLoad() () at
> /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
> #3  0x00007fc9760ce379 in WebKit::NetworkLoad::~NetworkLoad() () at
> /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
> #4  0x00007fc9761a0997 in
> WebKit::ServiceWorkerNavigationPreloader::didComplete() () at
> /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
> #5  0x00007fc9760cdc8b in
> WebKit::NetworkLoad::willPerformHTTPRedirection(WebCore::ResourceResponse&&,
> WebCore::ResourceRequest&&, WTF::CompletionHandler<void
> (WebCore::ResourceRequest&&)>&&) () at
> /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
> #6  0x00007fc976215857 in
> WebKit::NetworkDataTaskSoup::continueHTTPRedirection() () at
> /app/webkit/WebKitBuild/Release/lib/libwebkit2gtk-4.1.so.0
> 
> Any idea what might be the issue?

Nope, let me debug it.

[Carlos Garcia Campos]

Making this a security bug, because the problem I found is an UAF that I think it's in current code, but I'm not sure it can happen without this patch, so feel free to make it public again if there's not a security problem.

[Carlos Garcia Campos]

The problem is that NetworkLoad destructor is called twice because ServiceWorkerNavigationPreloader is used after being freed. This is the sequence:

1.- ServiceWorkerFetchTask::loadResponseFromPreloader: starts the preloader load
2.- ServiceWorkerNavigationPreloader::loadFromNetwork: creates a NetworkLoad and starts loading. Redirection happens.
3.- ServiceWorkerNavigationPreloader::willSendRedirectedRequest: didReceiveResponse is called
 3.1.- NetworkResourceLoader processes the response and finishes the load calling NetworkResourceLoader::didFinishWithRedirectResponse
 3.2.- ServiceWorkerFetchTask::~ServiceWorkerFetchTask: calls cancelPreloadIfNecessary and sets m_preloader = nullptr;
 3.3.- ServiceWorkerNavigationPreloader::cancel: this cancels the NetworkLoad
 3.4.- ServiceWorkerNavigationPreloader::~ServiceWorkerNavigationPreloader: this destructor destroys the NetworkLoad.
4.- ServiceWorkerNavigationPreloader::willSendRedirectedRequest: continues and calls didComplete(), but this is already destroyed at this point.
4.- ServiceWorkerNavigationPreloader::didComplete: makes m_networkLoad = nullptr, calling the NetworkLoad destructor again.

So, we need to make WeakPtr before calling didReceiveResponse() in 3. and check it before calling didComplete(). I'll update the patch to confirm it fixes in the crashes in EWS too.

[Carlos Garcia Campos]

Created attachment 454586 [details]
Patch

[youenn fablet]

(In reply to Carlos Garcia Campos from comment #8)
> The problem is that NetworkLoad destructor is called twice because
> ServiceWorkerNavigationPreloader is used after being freed. This is the
> sequence:
> 
> 1.- ServiceWorkerFetchTask::loadResponseFromPreloader: starts the preloader
> load
> 2.- ServiceWorkerNavigationPreloader::loadFromNetwork: creates a NetworkLoad
> and starts loading. Redirection happens.
> 3.- ServiceWorkerNavigationPreloader::willSendRedirectedRequest:
> didReceiveResponse is called
>  3.1.- NetworkResourceLoader processes the response and finishes the load
> calling NetworkResourceLoader::didFinishWithRedirectResponse
>  3.2.- ServiceWorkerFetchTask::~ServiceWorkerFetchTask: calls
> cancelPreloadIfNecessary and sets m_preloader = nullptr;
>  3.3.- ServiceWorkerNavigationPreloader::cancel: this cancels the NetworkLoad
>  3.4.- ServiceWorkerNavigationPreloader::~ServiceWorkerNavigationPreloader:
> this destructor destroys the NetworkLoad.
> 4.- ServiceWorkerNavigationPreloader::willSendRedirectedRequest: continues
> and calls didComplete(), but this is already destroyed at this point.
> 4.- ServiceWorkerNavigationPreloader::didComplete: makes m_networkLoad =
> nullptr, calling the NetworkLoad destructor again.
> 
> So, we need to make WeakPtr before calling didReceiveResponse() in 3. and
> check it before calling didComplete(). I'll update the patch to confirm it
> fixes in the crashes in EWS too.

Good catch!
I am not sure the bug can happen in the current code path since all we are doing before the patch when receiving a redirect is to send it to WebProcess.
After the patch, we are going through NetworkResourceLoader which can either send it to WebProcess (no problem) or cancel the load (leading tho this problem).

[youenn fablet]

Comment on attachment 454586 [details]
Patch

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

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerNavigationPreloader.cpp:160
> +        return;

It might be better to call didComplete inside the didReceiveResponse lambda which would catch the weak pointer.
Will upload this variant.

[youenn fablet]

Created attachment 454599 [details]
Patch

[Chris Dumez]

Comment on attachment 454599 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:247
> +    enum class IsFromServiceWorker { No, Yes };

: bool

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:249
> +    std::optional<WebCore::NetworkLoadMetrics> computeResponseMetrics(const WebCore::ResourceResponse&);

Ideally this function would be marked as const.

[youenn fablet]

Created attachment 454815 [details]
Patch for landing

[EWS]

Committed r291340 (248479@main): <https://commits.webkit.org/248479@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454815 [details].