| Summary: | Activate the WebResourceLoadStatisticsStore in the NetworkProcess and deactivate it in the UIProcess. | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||
| Component: | WebKit Misc. | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | achristensen, bfulgham, cdumez, dbates, ews-watchlist, japhet, jlewis3, rniwa, ryanhaddad, sroberts, tsavell, webkit-bug-importer, wilander | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=193922 | ||||||||||||||||
| Bug Depends on: | 193261 | ||||||||||||||||
| Bug Blocks: | 193199, 193303 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Brent Fulgham
2019-01-09 14:22:04 PST
Created attachment 359996 [details]
Patch
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 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
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 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
Created attachment 360025 [details]
Patch
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. 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. 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! Committed r240446: <https://trac.webkit.org/changeset/240446> 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 Reverted r240446 for reason: Casued 5 API failures Committed r240453: <https://trac.webkit.org/changeset/240453> Rollout out in https://trac.webkit.org/changeset/240453/webkit These TestWebKitAPI tests fail because they don't do anything to ensure the required support processes are running (namely the NetworkProcess). 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 Created attachment 360085 [details]
Patch
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. (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. 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. Created attachment 360127 [details]
Patch
(In reply to Chris Dumez from comment #18) > > Are the API tests passing with this iteration or not yet? API tests now pass. Committed r240498: <https://trac.webkit.org/changeset/240498> 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. Committed r240509: <https://trac.webkit.org/changeset/240509> 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&) (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 |