Bug 235710

Summary: redirectCount returns 0 when using a Service Worker
Product: WebKit Reporter: Daniel Schulz <daniel.schulz>
Component: Service WorkersAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Minor CC: achristensen, bfulgham, cdumez, cgarcia, ews-feeder, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Mac (Apple Silicon)   
OS: macOS 12   
Attachments:
Description Flags
All the files referenced in the bug report
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch for landing none

Description Daniel Schulz 2022-01-27 06:19:47 PST
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
Comment 1 Daniel Schulz 2022-01-30 23:25:32 PST
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
Comment 2 Radar WebKit Bug Importer 2022-02-03 06:20:16 PST
<rdar://problem/88432190>
Comment 3 youenn fablet 2022-03-11 02:21:33 PST
Created attachment 454462 [details]
Patch
Comment 4 youenn fablet 2022-03-11 06:44:02 PST
Created attachment 454480 [details]
Patch
Comment 5 youenn fablet 2022-03-11 08:35:16 PST
@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?
Comment 6 Carlos Garcia Campos 2022-03-14 02:41:59 PDT
(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.
Comment 7 Carlos Garcia Campos 2022-03-14 06:32:29 PDT
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.
Comment 8 Carlos Garcia Campos 2022-03-14 06:49:20 PDT
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.
Comment 9 Carlos Garcia Campos 2022-03-14 06:52:47 PDT
Created attachment 454586 [details]
Patch
Comment 10 youenn fablet 2022-03-14 07:19:56 PDT
(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).
Comment 11 youenn fablet 2022-03-14 09:43:12 PDT
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.
Comment 12 youenn fablet 2022-03-14 09:43:51 PDT
Created attachment 454599 [details]
Patch
Comment 13 Chris Dumez 2022-03-15 07:19:57 PDT
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.
Comment 14 youenn fablet 2022-03-16 03:22:40 PDT
Created attachment 454815 [details]
Patch for landing
Comment 15 EWS 2022-03-16 05:53:42 PDT
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].