Bug 209410 – WebKitTestRunner should enable ResourceLoadStatistics also for non-Cocoa ports

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Fujii Hironori]

[WinCairo][WebKitTestRunner] Failing ASSERT(m_resourceLoadStatistics) in NetworkSession::setThirdPartyCookieBlockingMode

WinCairo debug builds always failing to start WebKitTestRunner.

> ASSERTION FAILED: m_resourceLoadStatistics
> ..\..\Source\WebKit\NetworkProcess/NetworkSession.cpp(238) : setThirdPartyCookieBlockingMode
> 1   00007FFB58071429 WTFCrash
> 2   00007FFB29931F70 WTFCrashWithInfo
> 3   00007FFB2D7AC65F WebKit::NetworkSession::setThirdPartyCookieBlockingMode
> 4   00007FFB2D70C368 WebKit::NetworkProcess::setShouldBlockThirdPartyCookiesForTesting
> 5   00007FFB2D385104 IPC::callMemberFunctionImpl<WebKit::NetworkProcess,void (WebKit::NetworkProcess::*)(PAL::SessionID, WebCore::ThirdPartyCookieBlockingMode, WTF::CompletionHandler<void ()> &&),void (),std::tuple<PAL::SessionID,WebCore::ThirdPartyCookieBlockingMode>,0,1>
> 6   00007FFB2D383444 IPC::callMemberFunction<WebKit::NetworkProcess,void (WebKit::NetworkProcess::*)(PAL::SessionID, WebCore::ThirdPartyCookieBlockingMode, WTF::CompletionHandler<void ()> &&),void (),std::tuple<PAL::SessionID,WebCore::ThirdPartyCookieBlockingMode>,std::integer_sequence<unsigned long long,0,1> >
> 7   00007FFB2D31DB53 IPC::handleMessageAsync<Messages::NetworkProcess::SetShouldBlockThirdPartyCookiesForTesting,WebKit::NetworkProcess,void (WebKit::NetworkProcess::*)(PAL::SessionID, WebCore::ThirdPartyCookieBlockingMode, WTF::CompletionHandler<void ()> &&)>
> 8   00007FFB2D311F61 WebKit::NetworkProcess::didReceiveNetworkProcessMessage
> 9   00007FFB2D70525C WebKit::NetworkProcess::didReceiveMessage
> 10  00007FFB2D936E56 IPC::Connection::dispatchMessage
> 11  00007FFB2D937547 IPC::Connection::dispatchMessage
> 12  00007FFB2D937BEB IPC::Connection::dispatchOneIncomingMessage
> 13  00007FFB2D93AB2F IPC::Connection::enqueueIncomingMessage::<unnamed-tag>::operator()
> 14  00007FFB2D93AA6C WTF::Detail::CallableWrapper<`lambda at ..\..\Source\WebKit\Platform\IPC\Connection.cpp:981:30',void>::call
> 15  00007FFB580850A0 WTF::Function<void ()>::operator()
> 16  00007FFB580E1B36 WTF::RunLoop::performWork
> 17  00007FFB581ACC97 WTF::RunLoop::wndProc
> 18  00007FFB581ACBB9 WTF::RunLoop::RunLoopWndProc
> 19  00007FFB87F65C0D CallWindowProcW
> 20  00007FFB87F65602 DispatchMessageW
> 21  00007FFB581ACE93 WTF::RunLoop::run
> 22  00007FFB2D1C931B WebKit::AuxiliaryProcessMain<WebKit::NetworkProcess,WebKit::NetworkProcessMainCurl>
> 23  00007FFB2D1C90AB WebKit::NetworkProcessMain
> 24  00007FF6FDDD101C main
> 25  00007FF6FDDD123C __scrt_common_main_seh
> 26  00007FFB87EA7BD4 BaseThreadInitThunk
> 27  00007FFB884CCED1 RtlUserThreadStart

[Fujii Hironori]

Here is the callstack of UI process to send SetShouldBlockThirdPartyCookiesForTesting to Network process.

> WebKit2.dll!WebKit::NetworkProcessProxy::setShouldBlockThirdPartyCookiesForTesting(PAL::SessionID sessionID={...}, WebCore::ThirdPartyCookieBlockingMode blockingMode=OnlyAccordingToPerDomainPolicy, WTF::CompletionHandler<void ()> && completionHandler={...}) Line 1156	C++
> WebKit2.dll!WebKit::WebsiteDataStore::setResourceLoadStatisticsShouldBlockThirdPartyCookiesForTesting(bool enabled=false, bool onlyOnSitesWithoutUserInteraction=false, WTF::CompletionHandler<void ()> && completionHandler={...}) Line 1846	C++
> WebKit2.dll!WKWebsiteDataStoreStatisticsResetToConsistentState(const OpaqueWKWebsiteDataStore * dataStoreRef=0x00000269129822e0, void * context=0x00000082018fe058, void(*)(void *) completionHandler=0x00007ffb4edbe440) Line 597	C++
> WebKitTestRunnerLib.dll!WTR::TestController::statisticsResetToConsistentState() Line 3711	C++
> WebKitTestRunnerLib.dll!WTR::TestController::resetStateToConsistentValues(const WTR::TestOptions & options={...}, WTR::TestController::ResetStage resetStage=BeforeTest) Line 1131	C++
> WebKitTestRunnerLib.dll!WTR::TestController::ensureViewSupportsOptionsForTest(const WTR::TestInvocation & test={...}) Line 822	C++
> WebKitTestRunnerLib.dll!WTR::TestController::configureViewForTest(const WTR::TestInvocation & test={...}) Line 1561	C++
> WebKitTestRunnerLib.dll!WTR::TestInvocation::invoke() Line 161	C++
> WebKitTestRunnerLib.dll!WTR::TestController::runTest(const char * inputLine=0x00000082018fe750) Line 1780	C++
> WebKitTestRunnerLib.dll!WTR::TestController::runTestingServerLoop() Line 1826	C++
> WebKitTestRunnerLib.dll!WTR::TestController::run() Line 1834	C++
> WebKitTestRunnerLib.dll!WTR::TestController::TestController(int argc=2, const char * * argv=0x000002691292e8c0) Line 168	C++
> WebKitTestRunnerLib.dll!dllLauncherEntryPoint(int argc=2, const char * * argv=0x000002691292e8c0) Line 34	C++
> WebKitTestRunner.exe!main(int argc=2, const char * * argv=0x000002691292e8c0) Line 230	C++
> [External Code]

[Fujii Hironori]

It seems that Cocoa WebKitTestRunner always enables ResourceLoadStatistics.

https://trac.webkit.org/browser/webkit/trunk/Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm?rev=258562#L75

>    [globalWebViewConfiguration.websiteDataStore _setResourceLoadStatisticsEnabled:YES];

[Fujii Hironori]

Created attachment 394244 [details]
WIP patch

[Fujii Hironori]

Created attachment 394295 [details]
Patch

[John Wilander]

Comment on attachment 394295 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        [WinCairo][WebKitTestRunner] Failing ASSERT(m_resourceLoadStatistics) in NetworkSession::setThirdPartyCookieBlockingMode

I don't think WinCairo and WebKitTestRunner are good prefixes on a patch that changes WebKit code across platforms. Also the "Failing ASSERT …" description does not convey what's being changed. Please rephrase.

> Tools/WebKitTestRunner/TestController.cpp:3115
> +    WKWebsiteDataStoreSetResourceLoadStatisticsEnabled(WKContextGetWebsiteDataStore(context), true);

I remember someone making changes here recently. Or was it a specific platform?

[Fujii Hironori]

Comment on attachment 394295 [details]
Patch

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

Thank you for the review.

>> Source/WebKit/ChangeLog:3
>> +        [WinCairo][WebKitTestRunner] Failing ASSERT(m_resourceLoadStatistics) in NetworkSession::setThirdPartyCookieBlockingMode
> 
> I don't think WinCairo and WebKitTestRunner are good prefixes on a patch that changes WebKit code across platforms. Also the "Failing ASSERT …" description does not convey what's being changed. Please rephrase.

Agreed. Will fix.

>> Tools/WebKitTestRunner/TestController.cpp:3115
>> +    WKWebsiteDataStoreSetResourceLoadStatisticsEnabled(WKContextGetWebsiteDataStore(context), true);
> 
> I remember someone making changes here recently. Or was it a specific platform?

I can't find it. which bug?

[John Wilander]

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 394295 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394295&action=review
> 
> Thank you for the review.
> 
> >> Source/WebKit/ChangeLog:3
> >> +        [WinCairo][WebKitTestRunner] Failing ASSERT(m_resourceLoadStatistics) in NetworkSession::setThirdPartyCookieBlockingMode
> > 
> > I don't think WinCairo and WebKitTestRunner are good prefixes on a patch that changes WebKit code across platforms. Also the "Failing ASSERT …" description does not convey what's being changed. Please rephrase.
> 
> Agreed. Will fix.
> 
> >> Tools/WebKitTestRunner/TestController.cpp:3115
> >> +    WKWebsiteDataStoreSetResourceLoadStatisticsEnabled(WKContextGetWebsiteDataStore(context), true);
> > 
> > I remember someone making changes here recently. Or was it a specific platform?
> 
> I can't find it. which bug?

Found it: https://bugs.webkit.org/show_bug.cgi?id=208506

[Fujii Hironori]

Created attachment 394410 [details]
Patch

[youenn fablet]

Comment on attachment 394410 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:3115
> +    WKWebsiteDataStoreSetResourceLoadStatisticsEnabled(WKContextGetWebsiteDataStore(context), true);

This probably makes the call to _setResourceLoadStatisticsEnabled:YES in TestControlerCocoa.mm unnecessary.

[Fujii Hironori]

Comment on attachment 394410 [details]
Patch

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

Thank you for the review.

>> Tools/WebKitTestRunner/TestController.cpp:3115
>> +    WKWebsiteDataStoreSetResourceLoadStatisticsEnabled(WKContextGetWebsiteDataStore(context), true);
> 
> This probably makes the call to _setResourceLoadStatisticsEnabled:YES in TestControlerCocoa.mm unnecessary.

No. Cocoa ports don't use this code. https://trac.webkit.org/browser/webkit/trunk/Tools/WebKitTestRunner/TestController.cpp?rev=258562#L3098

[Fujii Hironori]

Committed r259076: <https://trac.webkit.org/changeset/259076>

[Radar WebKit Bug Importer]

<rdar://problem/60935096>