Bug 193297 – Activate the WebResourceLoadStatisticsStore in the NetworkProcess and deactivate it in the UIProcess.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

This patch activates the ResourceLoadStatistics code in the NetworkProcess, and turns it off in the UIProcess. It also updates test infrastructure to work with this change in architecture.

[Radar WebKit Bug Importer]

<rdar://problem/47158841>

[Brent Fulgham]

Created attachment 359996 [details]
Patch

[EWS Watchlist]

Comment on attachment 359996 [details]
Patch

Attachment 359996 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10870526

New failing tests:
http/tests/websocket/connection-refusal-in-frame-resource-load-statistics.html

[EWS Watchlist]

Created attachment 359999 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 359996 [details]
Patch

Attachment 359996 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10870604

New failing tests:
http/tests/websocket/connection-refusal-in-frame-resource-load-statistics.html

[EWS Watchlist]

Created attachment 360001 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Brent Fulgham]

Created attachment 360025 [details]
Patch

[Alex Christensen]

Comment on attachment 360025 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.h:77
> +    WEBCORE_EXPORT void setLogWebSocketLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&&);

WTF:: is not needed.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:506
> +    postTask([this, targetPrimaryDomain = targetPrimaryDomain.isolatedCopy(), mainFramePrimaryDomain = mainFramePrimaryDomain.isolatedCopy(), lastSeen, completionHandler = WTFMove(completionHandler)]() mutable {

Should we have a protectedThis or a weak pointer?

> Source/WebKit/WebProcess/WebProcess.cpp:400
> +    ResourceLoadObserver::shared().setLogSubresourceRedirectNotificationCallback([this] (PAL::SessionID sessionID, const String& sourcePrimaryDomain, const String& targetPrimaryDomain) {

'this' should probably be a weak pointer.  I can imagine some using after freeing from loads updating after a page has been torn down.

[Chris Dumez]

Comment on attachment 360025 [details]
Patch

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

>> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:506
>> +    postTask([this, targetPrimaryDomain = targetPrimaryDomain.isolatedCopy(), mainFramePrimaryDomain = mainFramePrimaryDomain.isolatedCopy(), lastSeen, completionHandler = WTFMove(completionHandler)]() mutable {
> 
> Should we have a protectedThis or a weak pointer?

No need, postTask() protects this for you.

>> Source/WebKit/WebProcess/WebProcess.cpp:400
>> +    ResourceLoadObserver::shared().setLogSubresourceRedirectNotificationCallback([this] (PAL::SessionID sessionID, const String& sourcePrimaryDomain, const String& targetPrimaryDomain) {
> 
> 'this' should probably be a weak pointer.  I can imagine some using after freeing from loads updating after a page has been torn down.

WebProcess is a singleton so I do not think this would be useful.

[Brent Fulgham]

Comment on attachment 360025 [details]
Patch

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

Thank you guys for reviewing this.

>> Source/WebCore/loader/ResourceLoadObserver.h:77
>> +    WEBCORE_EXPORT void setLogWebSocketLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&&);
> 
> WTF:: is not needed.

Fixed.

>>> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:506
>>> +    postTask([this, targetPrimaryDomain = targetPrimaryDomain.isolatedCopy(), mainFramePrimaryDomain = mainFramePrimaryDomain.isolatedCopy(), lastSeen, completionHandler = WTFMove(completionHandler)]() mutable {
>> 
>> Should we have a protectedThis or a weak pointer?
> 
> No need, postTask() protects this for you.

Yay!

[Brent Fulgham]

Committed r240446: <https://trac.webkit.org/changeset/240446>

[Truitt Savell]

Looks like https://trac.webkit.org/changeset/240446/webkit 

has caused 5 API failures. 

Log:
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK1%20%28Tests%29/builds/2406/steps/run-api-tests/logs/stdio

build:
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK1%20%28Tests%29/builds/2405

[Truitt Savell]

Reverted r240446 for reason:

Casued 5 API failures

Committed r240453: <https://trac.webkit.org/changeset/240453>

[Truitt Savell]

Rollout out in https://trac.webkit.org/changeset/240453/webkit

[Brent Fulgham]

These TestWebKitAPI tests fail because they don't do anything to ensure the required support processes are running (namely the NetworkProcess).

[Truitt Savell]

It also appears this test:
http/tests/resourceLoadStatistics/non-sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-localhost-to-ip-to-localhost.html

Became a constant timeout with r240446 on Mac WK2

History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fnon-sandboxed-nesting-iframe-with-sandboxed-iframe-redirect-localhost-to-ip-to-localhost.html

this was also resolved with the rollout:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/9046

[Brent Fulgham]

Created attachment 360085 [details]
Patch

[Chris Dumez]

Comment on attachment 360085 [details]
Patch

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

Are the API tests passing with this iteration or not yet?

> Source/WebCore/loader/ResourceLoadObserver.cpp:78
> +void ResourceLoadObserver::setLogWebSocketLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&& callback)

WTF:: is not needed.

> Source/WebCore/loader/ResourceLoadObserver.cpp:84
> +void ResourceLoadObserver::setLogSubresourceLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&& callback)

WTF:: is not needed.

> Source/WebCore/loader/ResourceLoadObserver.cpp:90
> +void ResourceLoadObserver::setLogSubresourceRedirectNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&)>&& callback)

WTF:: is not needed.

> Source/WebCore/loader/ResourceLoadObserver.h:77
> +    WEBCORE_EXPORT void setLogWebSocketLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&&);

WTF:: is not needed.

> Source/WebCore/loader/ResourceLoadObserver.h:78
> +    WEBCORE_EXPORT void setLogSubresourceLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&&);

WTF:: is not needed.

> Source/WebCore/loader/ResourceLoadObserver.h:79
> +    WEBCORE_EXPORT void setLogSubresourceRedirectNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&)>&&);

WTF:: is not needed.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:643
> +    auto& targetStatistics = ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);

ASSERT(!RunLoop::isMain());

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:649
> +void ResourceLoadStatisticsMemoryStore::logSubresourceLoading(const String& targetPrimaryDomain, const String& mainFramePrimaryDomain, WallTime lastSeen)

This method seems entirely identical to the one above, no?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:651
> +    auto& targetStatistics = ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);

ASSERT(!RunLoop::isMain());

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:659
> +    auto& redirectingOriginStatistics = ensureResourceStatisticsForPrimaryDomain(sourcePrimaryDomain);

ASSERT(!RunLoop::isMain());

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:882
> +    if (auto websiteDataStore = websiteDataStoreFromSessionID(sessionID))

auto* ?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:61
> +    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);

auto

Same comment for all new variables below.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:62
> +    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);

auto

Also, since you're not customizing the configuration in any way, I do not think you need to create one and pass it here. You could just call:
auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);

Same comments below.

[Brent Fulgham]

(In reply to Chris Dumez from comment #18)
> Comment on attachment 360085 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360085&action=review
> 
> Are the API tests passing with this iteration or not yet?

I'm having trouble getting the two tests "WebsiteDataStoreCustomPathsWith{out}Prewarming" to work (see my e-mail).

All other tests pass now.

[Brent Fulgham]

Comment on attachment 360085 [details]
Patch

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

Thanks for reviewing!

>> Source/WebCore/loader/ResourceLoadObserver.cpp:78
>> +void ResourceLoadObserver::setLogWebSocketLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&& callback)
> 
> WTF:: is not needed.

Fixed.

>> Source/WebCore/loader/ResourceLoadObserver.cpp:84
>> +void ResourceLoadObserver::setLogSubresourceLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&& callback)
> 
> WTF:: is not needed.

Fixed.

>> Source/WebCore/loader/ResourceLoadObserver.cpp:90
>> +void ResourceLoadObserver::setLogSubresourceRedirectNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&)>&& callback)
> 
> WTF:: is not needed.

Fixed.

>> Source/WebCore/loader/ResourceLoadObserver.h:77
>> +    WEBCORE_EXPORT void setLogWebSocketLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&&);
> 
> WTF:: is not needed.

Fixed.

>> Source/WebCore/loader/ResourceLoadObserver.h:78
>> +    WEBCORE_EXPORT void setLogSubresourceLoadingNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&, WallTime)>&&);
> 
> WTF:: is not needed.

Fixed.

>> Source/WebCore/loader/ResourceLoadObserver.h:79
>> +    WEBCORE_EXPORT void setLogSubresourceRedirectNotificationCallback(WTF::Function<void(PAL::SessionID, const String&, const String&)>&&);
> 
> WTF:: is not needed.

Fixed.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:649
>> +void ResourceLoadStatisticsMemoryStore::logSubresourceLoading(const String& targetPrimaryDomain, const String& mainFramePrimaryDomain, WallTime lastSeen)
> 
> This method seems entirely identical to the one above, no?

Yes -- good point. I'll have the higher-level WebSocket calls use the "logSubresourceLoading" method on the memory store, since the web socket load is just a special case of sub resource loading.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:651
>> +    auto& targetStatistics = ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);
> 
> ASSERT(!RunLoop::isMain());

Fixed.

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:659
>> +    auto& redirectingOriginStatistics = ensureResourceStatisticsForPrimaryDomain(sourcePrimaryDomain);
> 
> ASSERT(!RunLoop::isMain());

Fixed.

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:882
>> +    if (auto websiteDataStore = websiteDataStoreFromSessionID(sessionID))
> 
> auto* ?

Sure. Changed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:61
>> +    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
> 
> auto
> 
> Same comment for all new variables below.

Fixed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ResourceLoadStatistics.mm:62
>> +    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
> 
> auto
> 
> Also, since you're not customizing the configuration in any way, I do not think you need to create one and pass it here. You could just call:
> auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
> 
> Same comments below.

Fixed.

[Brent Fulgham]

Created attachment 360127 [details]
Patch

[Brent Fulgham]

(In reply to Chris Dumez from comment #18)
> 
> Are the API tests passing with this iteration or not yet?

API tests now pass.

[Brent Fulgham]

Committed r240498: <https://trac.webkit.org/changeset/240498>

[Truitt Savell]

https://trac.webkit.org/changeset/240498/webkit caused 3 API failures:

Build:
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK1%20%28Tests%29/builds/2441

Log:
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK1%20%28Tests%29/builds/2441/steps/run-api-tests/logs/stdio

Timeout

    TestWebKitAPI.ResourceLoadStatistics.ChildProcessesNotLaunched
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.

    TestWebKitAPI.ResourceLoadStatistics.GrandfatherCallback
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.

    TestWebKitAPI.ResourceLoadStatistics.ShouldNotGrandfatherOnStartup
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.

[Brent Fulgham]

Committed r240509: <https://trac.webkit.org/changeset/240509>

[Shawn Roberts]

The following test is a flaky crash on MacOS Debug

http/tests/resourceLoadStatistics/prune-statistics.html

Flakiness Dashboard: 

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fprune-statistics.html

Probable cause: 

Can reproduce crash starting with Debug build 240498 to current 240715 with the following command: 

run-webkit-tests http/tests/resourceLoadStatistics/prune-statistics.html --iterations 500 -f --debug --exit-after-n-failures=1 --no-retry-failures

Crash log from High Sierra: (Mojave has same information)

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r240710%20(6440)/http/tests/resourceLoadStatistics/prune-statistics-crash-log.txt

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010d3aeb10 WTFCrash + 16 (Assertions.cpp:255)
1   com.apple.JavaScriptCore      	0x000000010d3aeb29 WTFCrashWithSecurityImplication + 9
2   com.apple.WebKit              	0x0000000105a2a262 WTF::RefCountedBase::ref() const + 66 (RefCounted.h:43)

Assertion failure: 

ASSERTION FAILED: !m_deletionHasBegun
/Volumes/Data/slave/highsierra-debug/build/WebKitBuild/Debug/usr/local/include/wtf/RefCounted.h(43) : void WTF::RefCountedBase::ref() const
1   0x10d3aeb09 WTFCrash
2   0x10d3aeb29 WTFCrashWithSecurityImplication
3   0x105a2a262 WTF::RefCountedBase::ref() const
4   0x105d516e7 WTF::Ref<WebKit::NetworkProcess::deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul>&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String> > const&)>&&)::CallbackAggregator, WTF::DumbPtrTraits<WebKit::NetworkProcess::deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul>&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String> > const&)>&&)::CallbackAggregator> >::Ref(WebKit::NetworkProcess::deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(PAL::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul>&&, bool, WTF::CompletionHandler<void (WTF::HashSet<WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String> > const&)>&&)::CallbackAggregator&)

[Chris Dumez]

(In reply to Shawn Roberts from comment #26)
> The following test is a flaky crash on MacOS Debug
> 
> http/tests/resourceLoadStatistics/prune-statistics.html
> 
> Flakiness Dashboard: 
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fprune-
> statistics.html
> 
> Probable cause: 
> 
> Can reproduce crash starting with Debug build 240498 to current 240715 with
> the following command: 
> 
> run-webkit-tests http/tests/resourceLoadStatistics/prune-statistics.html
> --iterations 500 -f --debug --exit-after-n-failures=1 --no-retry-failures
> 
> Crash log from High Sierra: (Mojave has same information)
> 
> https://build.webkit.org/results/
> Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r240710%20(6440)/http/tests/
> resourceLoadStatistics/prune-statistics-crash-log.txt
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore      	0x000000010d3aeb10 WTFCrash + 16
> (Assertions.cpp:255)
> 1   com.apple.JavaScriptCore      	0x000000010d3aeb29
> WTFCrashWithSecurityImplication + 9
> 2   com.apple.WebKit              	0x0000000105a2a262
> WTF::RefCountedBase::ref() const + 66 (RefCounted.h:43)
> 
> Assertion failure: 
> 
> ASSERTION FAILED: !m_deletionHasBegun
> /Volumes/Data/slave/highsierra-debug/build/WebKitBuild/Debug/usr/local/
> include/wtf/RefCounted.h(43) : void WTF::RefCountedBase::ref() const
> 1   0x10d3aeb09 WTFCrash
> 2   0x10d3aeb29 WTFCrashWithSecurityImplication
> 3   0x105a2a262 WTF::RefCountedBase::ref() const
> 4   0x105d516e7
> WTF::Ref<WebKit::NetworkProcess::
> deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(PA
> L::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>,
> WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul>&&, bool,
> WTF::CompletionHandler<void (WTF::HashSet<WTF::String, WTF::StringHash,
> WTF::HashTraits<WTF::String> > const&)>&&)::CallbackAggregator,
> WTF::DumbPtrTraits<WebKit::NetworkProcess::
> deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(PA
> L::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>,
> WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul>&&, bool,
> WTF::CompletionHandler<void (WTF::HashSet<WTF::String, WTF::StringHash,
> WTF::HashTraits<WTF::String> > const&)>&&)::CallbackAggregator>
> >::Ref(WebKit::NetworkProcess::
> deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(PA
> L::SessionID, WTF::OptionSet<WebKit::WebsiteDataType>,
> WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul>&&, bool,
> WTF::CompletionHandler<void (WTF::HashSet<WTF::String, WTF::StringHash,
> WTF::HashTraits<WTF::String> > const&)>&&)::CallbackAggregator&)

I think I know why. Please see my comment at https://bugs.webkit.org/show_bug.cgi?id=193556#c32