When I moved the ResourceLoadStatistics logic to the NetworkProcess, I did not realize that the iOS process startup does not have a WebsiteDataStore attached until much later in the launch cycle. Because of this, we do not pass the ITP state to the NetworkProcess at launch (as part of its configuration parameters), and when we attempt to message the state to the NetworkProcess during startup we do so at a moment when we have not associated the WebProcessPool with the WebsiteDataStore object that knows about ITP status. Furthermore, I failed to notice that the method to activate ITP only messages the WebContent processes, not the NetworkProcess. This had not been needed before I moved the logic to the NetworkProcess, and was an oversight. This patch updates WebProcessPool::setResourceLoadStatisticsEnabled so that the process pool tells the NetworkProcess when the ITP state changes.
<rdar://problem/51538088>
Created attachment 371666 [details] Patch
Comment on attachment 371666 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371666&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:1491 > + sendToNetworkingProcess(Messages::WebProcess::SetResourceLoadStatisticsEnabled(enabled)); This message type should be for the NetworkProcess, not the WebProcess.
Comment on attachment 371666 [details] Patch This really needs a test, since it is a fix for a real regression and one that wen't long unnoticed. r- for lack of test case.
Created attachment 371681 [details] Patch
Comment on attachment 371681 [details] Patch Updated to use proper message heading. I'll tackle the test separately since it will be a larger patch.
Created attachment 371682 [details] Patch
Comment on attachment 371682 [details] Patch Updated to address WPE build failure.
Comment on attachment 371682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371682&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:1492 > + sendToNetworkingProcess(Messages::NetworkProcess::SetResourceLoadStatisticsEnabled(enabled)); I’m looking at this on my phone so I don’t have the larger picture. Do we need to ensure that there is a network process here? I know we had some cases where that was a requirement.
Comment on attachment 371682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371682&action=review >> Source/WebKit/UIProcess/WebProcessPool.cpp:1492 >> + sendToNetworkingProcess(Messages::NetworkProcess::SetResourceLoadStatisticsEnabled(enabled)); > > I’m looking at this on my phone so I don’t have the larger picture. Do we need to ensure that there is a network process here? I know we had some cases where that was a requirement. That's probably a good idea. It looks like we do something like that when dealing with Ephemeral sessions, so this probably has the same requirement.
(In reply to Maciej Stachowiak from comment #4) > Comment on attachment 371666 [details] > Patch > > This really needs a test, since it is a fix for a real regression and one > that wen't long unnoticed. r- for lack of test case. I started doing this, but the patch grew much larger. I would rather handle the test case as a distinct patch so that it's clear what's going on. See Bug 198694 for that work.
Created attachment 371691 [details] Patch
(In reply to Brent Fulgham from comment #10) > Comment on attachment 371682 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371682&action=review > > >> Source/WebKit/UIProcess/WebProcessPool.cpp:1492 > >> + sendToNetworkingProcess(Messages::NetworkProcess::SetResourceLoadStatisticsEnabled(enabled)); > > > > I’m looking at this on my phone so I don’t have the larger picture. Do we need to ensure that there is a network process here? I know we had some cases where that was a requirement. > > That's probably a good idea. It looks like we do something like that when > dealing with Ephemeral sessions, so this probably has the same requirement. Actually it's not a good idea because it breaks the API tests involving a lack of a process pool (TestWebKitAPI.WebKit.WKHTTPCookieStoreWithoutProcessPoolWithPrewarming and the without prewarming case). So, we don't want to force a network process launch if it doesn't already exist -- we just want to make sure we do send it the message.
Created attachment 371697 [details] Patch
Comment on attachment 371697 [details] Patch Looks good.
Comment on attachment 371697 [details] Patch Clearing flags on attachment: 371697 Committed r246248: <https://trac.webkit.org/changeset/246248>
All reviewed patches have been landed. Closing bug.