Bug 228095 – [curl][soup] NetworkSession::m_keptAliveLoads leaks NetworkResourceLoader

• Component: WebKit2
• Status: REOPENED

[Fujii Hironori]

[WinCairo] ASSERT(!m_networkLoad) fails in ~NetworkResourceLoader while shutting down WebKitNetworkProcess.exe

1. Start WinCairo WK2 MiniBrowser (Debug builds)
2. Visit some web sites
3. Close the MiniBrowser
4. The assertion fails

Callstack:

> WTF.dll!WTFCrash() Line 321	C++
> WebKit2.dll!WTFCrashWithInfo(int __formal, const char * __formal, const char * __formal, int __formal) Line 698	C++
> WebKit2.dll!WebKit::NetworkResourceLoader::() Line 137	C++
> WebKit2.dll!WebKit::NetworkResourceLoader::`scalar deleting destructor'(unsigned int)	C++
> WebKit2.dll!std::default_delete<WebKit::NetworkResourceLoader>::operator()(WebKit::NetworkResourceLoader * _Ptr) Line 3120	C++
> WebKit2.dll!WTF::RefCounted<WebKit::NetworkResourceLoader,std::default_delete<WebKit::NetworkResourceLoader>>::deref() Line 191	C++
> WebKit2.dll!WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>::~Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>() Line 62	C++
> WebKit2.dll!WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>::`scalar deleting destructor'(unsigned int)	C++
> WebKit2.dll!WTF::HashTable<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>,WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>,WTF::IdentityExtractor,WTF::DefaultHash<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTraits<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTraits<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>>::deallocateTable(WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>> * table) Line 1228	C++
> WebKit2.dll!WTF::HashTable<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>,WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>,WTF::IdentityExtractor,WTF::DefaultHash<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTraits<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTraits<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>>::~HashTable<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>,WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>,WTF::IdentityExtractor,WTF::DefaultHash<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTraits<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTraits<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>>() Line 417	C++
> WebKit2.dll!WTF::HashSet<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>,WTF::DefaultHash<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTraits<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTableTraits>::~HashSet<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>,WTF::DefaultHash<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTraits<WTF::Ref<WebKit::NetworkResourceLoader,WTF::RawPtrTraits<WebKit::NetworkResourceLoader>>>,WTF::HashTableTraits>()	C++
> WebKit2.dll!WebKit::NetworkSession::~NetworkSession() Line 140	C++
> WebKit2.dll!WebKit::NetworkSessionCurl::~NetworkSessionCurl() Line 59	C++
> WebKit2.dll!WebKit::NetworkSessionCurl::`scalar deleting destructor'(unsigned int)	C++
> WebKit2.dll!std::default_delete<WebKit::NetworkSession>::operator()(WebKit::NetworkSession * _Ptr) Line 3120	C++
> WebKit2.dll!std::unique_ptr<WebKit::NetworkSession,std::default_delete<WebKit::NetworkSession>>::~unique_ptr<WebKit::NetworkSession,std::default_delete<WebKit::NetworkSession>>() Line 3232	C++
> WebKit2.dll!WebKit::NetworkProcess::destroySession(PAL::SessionID sessionID) Line 547	C++
> WebKit2.dll!WebKit::NetworkProcessMainCurl::platformFinalize() Line 39	C++
> WebKit2.dll!WebKit::AuxiliaryProcessMainBase<WebKit::NetworkProcess,0>::run(int argc, char * * argv) Line 73	C++
> WebKit2.dll!WebKit::AuxiliaryProcessMain<WebKit::NetworkProcessMainCurl>(int argc, char * * argv) Line 97	C++
> WebKit2.dll!WebKit::NetworkProcessMain(int argc, char * * argv) Line 45	C++
> WebKitNetworkProcess.exe!main(int argc, char * * argv) Line 35	C++
> [Inline Frame] WebKitNetworkProcess.exe!invoke_main() Line 78	C++
> WebKitNetworkProcess.exe!__scrt_common_main_seh() Line 288	C++
> kernel32.dll!00007ffd45127034()	Unknown
> ntdll.dll!00007ffd46f42651()	Unknown

[Fujii Hironori]

> void NetworkProcess::destroySession(PAL::SessionID sessionID)
> {
>     ASSERT(RunLoop::isMain());
> #if !USE(SOUP) && !USE(CURL)
>     // cURL and Soup based ports destroy the default session right before the process exits to avoid leaking
>     // network resources like the cookies database.
>     ASSERT(sessionID != PAL::SessionID::defaultSessionID());
> #endif
>     ....

How should NetworkSession::m_keptAliveLoads be cleaned up?

[Fujii Hironori]

Can Cocoa ports fail this assertion? They don't destroy the default network session. How about non-default session? Can m_keptAliveLoads be non-empty for the non-default sessions?

[Radar WebKit Bug Importer]

<rdar://problem/81393898>

[Fujii Hironori]

I don't know how to reproduce this assertion failure 100%. But,
the following steps can often reproduce it.

1. Start WinCairo WK2 MiniBrowser (Debug builds)
2. Go to https://bugs.webkit.org/show_bug.cgi?id=228095
3. Go to https://www.yomiuri.co.jp/
4. Go back
5. Close the MiniBrowser
6. The assertion fails

https://www.yahoo.co.jp/ also can reproduce the assertion failure.

I can't reproduce the assertion failure with http/wpt/fetch/fetch-in-pagehide.html test.

[Fujii Hironori]

Created attachment 434737 [details]
WIP patch

If the Fetch keepalive option is specified, and the page is navigated,
the NetworkResourceLoader is transferred to the NetworkSession by NetworkConnectionToWebProcess::transferKeptAliveLoad.

NetworkResourceLoader::didReceiveResponse replies PolicyAction::Ignore for the such kept alive loads.
https://github.com/WebKit/WebKit/blob/66ab53513519a418038feff18f73c4f3f5b6b9be/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp#L674

NetworkDataTaskCurl should call NetworkLoad::didCompleteWithError for the kept alive load in such case.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #2)
> Can Cocoa ports fail this assertion? They don't destroy the default network
> session. How about non-default session? Can m_keptAliveLoads be non-empty
> for the non-default sessions?

Yes, Mac port also fails the assertion.

WinCairo has two problems, comment#5 problem and the same problem with Mac.
I'm going to use this bug for curl port problem (comment#5).

Filed a new bug ticket for common problem.
Bug 228853 – ASSERTION FAILED: !m_networkLoad in NetworkResourceLoader::~NetworkResourceLoader()

[Fujii Hironori]

The assertion failure can't be reproduced after r280740 fixed Bug 228853.
However, the WIP patch (attachment#434737 [details]) is still valid because curl port leaks kept alive NetworkResourceLoaders.

[Fujii Hironori]

Created attachment 435658 [details]
Patch

[Fujii Hironori]

Created attachment 435659 [details]
Patch

[Fujii Hironori]

Created attachment 435662 [details]
Patch

[Fujii Hironori]

Created attachment 435667 [details]
Patch

[Fujii Hironori]

Comment on attachment 435667 [details]
Patch

Clearing flags on attachment: 435667

Committed r281158 (240608@main): <https://commits.webkit.org/240608@main>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

The test is failing in EWS:

https://ews-build.s3-us-west-2.amazonaws.com/macOS-Catalina-Release-WK2-Tests-EWS/r435724-13625/results.html

[Fujii Hironori]

Committed r281177 (240623@main): <https://commits.webkit.org/240623@main>

It looks like http/tests/fetch/keepalive-fetch-2.html is still failing on Windows and (https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Ffetch%2Fkeepalive-fetch-2.html)and Windows-EWS.

[Fujii Hironori]

Oh, sorry. I'm going to mark it as flaky and try to fix it in the following ticket.
  Bug 229247 – [Win] http/tests/fetch/keepalive-fetch-2.html is randomly failing

[Yury Semikhatsky]

(In reply to Fujii Hironori from comment #12)
> Comment on attachment 435667 [details]
> Patch
> 
> Clearing flags on attachment: 435667
> 
> Committed r281158 (240608@main): <https://commits.webkit.org/240608@main>

It looks like this change regressed inspector and console, previously on evaluating 


`fetch('http://localhost/')` in the context of about:blank page there would be a message:

"Origin null is not allowed by Access-Control-Allow-Origin."

which is now missing. Also the network failure is reported as just cancelled in this case:

Inspector protocol message before:
◀ RECV {"method":"Target.dispatchMessageFromTarget","params":{"targetId":"page-7","message":"{\"method\":\"Network.loadingFailed\",\"params\":{\"requestId\":\"8.1\",\"timestamp\":0.1725471019744873,\"errorText\":\"Origin null is not allowed by Access-Control-Allow-Origin.\",\"canceled\":false}}"},"browserContextId":"8000000000000002","pageProxyId":"6"}


Inspector protocol message after the change:
 ◀ RECV {"method":"Target.dispatchMessageFromTarget","params":{"targetId":"page-7","message":"{\"method\":\"Network.loadingFailed\",\"params\":{\"requestId\":\"8.1\",\"timestamp\":0.17682290077209473,\"errorText\":\"\",\"canceled\":true}}"},"browserContextId":"8000000000000002","pageProxyId":"6"}


This playwright test demonstrates the issue (no console message anymore): https://github.com/microsoft/playwright/blob/de85d8bb83d0e36c994af938d7274853a462ccc9/tests/page/page-event-console.spec.ts#L95-L103

[Fujii Hironori]

(In reply to Yury Semikhatsky from comment #18)

Thank you very much for the bug report.
Filed: Bug 229515 – [curl] REGRESSION(r281158): fetch('http://localhost/') from about:blank doesn't emit the console error message

[Fujii Hironori]

r281614 reverted r281158. Reopened.

[Fujii Hironori]

This is not curl specific issue.
As far as I tested, NetworkSession::removeKeptAliveLoad isn't called in GTK port for http/tests/fetch/keepalive-fetch-2.html.

[Fujii Hironori]

However, Mac port doesn't leak.
It calls NetworkSession::removeKeptAliveLoad.
Here is the callstack of that.

#0	0x0000000115096b1c in WebKit::NetworkSession::removeKeptAliveLoad(WebKit::NetworkResourceLoader&) at /Volumes/Data/webkit/gb/Source/WebKit/NetworkProcess/NetworkSession.cpp:389
#1	0x0000000114fc2721 in WebKit::NetworkProcess::removeKeptAliveLoad(WebKit::NetworkResourceLoader&) at /Volumes/Data/webkit/gb/Source/WebKit/NetworkProcess/NetworkProcess.cpp:2672
#2	0x0000000114f4879e in WebKit::NetworkConnectionToWebProcess::didCleanupResourceLoader(WebKit::NetworkResourceLoader&) at /Volumes/Data/webkit/gb/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:173
#3	0x0000000114fcf680 in WebKit::NetworkResourceLoader::cleanup(WebKit::NetworkResourceLoader::LoadResult) at /Volumes/Data/webkit/gb/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:475
#4	0x0000000114fcebe1 in WebKit::NetworkResourceLoader::didFailLoading(WebCore::ResourceError const&) at /Volumes/Data/webkit/gb/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:827
#5	0x0000000114f5904b in WebKit::NetworkLoad::didCompleteWithError(WebCore::ResourceError const&, WebCore::NetworkLoadMetrics const&) at /Volumes/Data/webkit/gb/Source/WebKit/NetworkProcess/NetworkLoad.cpp:255
#6	0x0000000114cf0375 in WebKit::NetworkDataTaskCocoa::didCompleteWithError(WebCore::ResourceError const&, WebCore::NetworkLoadMetrics const&) at /Volumes/Data/webkit/gb/Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:449
#7	0x0000000114cf6fb5 in -[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:] at /Volumes/Data/webkit/gb/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:794
#8	0x00007fff248c67e3 in ___lldb_unnamed_symbol9274$$CFNetwork ()
#9	0x0000000108444032 in _dispatch_call_block_and_release ()
#10	0x0000000108445264 in _dispatch_client_callout ()
#11	0x000000010845533b in _dispatch_main_queue_callback_4CF ()
#12	0x00007fff204b25a8 in __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ ()
#13	0x00007fff204747a2 in __CFRunLoopRun ()
#14	0x00007fff2047361c in CFRunLoopRunSpecific ()
#15	0x00007fff21203607 in -[NSRunLoop(NSRunLoop) runMode:beforeDate:] ()
#16	0x00007fff212914d1 in -[NSRunLoop(NSRunLoop) run] ()
#17	0x00007fff200cb38d in _xpc_objc_main ()
#18	0x00007fff200cacd3 in xpc_main ()
#19	0x00000001150cb055 in WebKit::XPCServiceMain(int, char const**) at /Volumes/Data/webkit/gb/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:243
#20	0x00000001168fb63b in WKXPCServiceMain at /Volumes/Data/webkit/gb/Source/WebKit/Shared/API/Cocoa/WKMain.mm:33
#21	0x000000010826cea2 in main at /Volumes/Data/webkit/gb/Source/WebKit/Shared/EntryPointUtilities/Cocoa/AuxiliaryProcessMain.cpp:30
#22	0x00007fff20397f3d in start ()
#23	0x00007fff20397f3d in start ()

[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:] calls didCompleteWithError.