Bug 198692 – [iOS] ResourceLoadStatistics state is not properly relayed to the NetworkProcess

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

[Brent Fulgham]

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.

[Brent Fulgham]

<rdar://problem/51538088>

[Brent Fulgham]

Created attachment 371666 [details]
Patch

[John Wilander]

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.

[Maciej Stachowiak]

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.

[Brent Fulgham]

Created attachment 371681 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 371682 [details]
Patch

[Brent Fulgham]

Comment on attachment 371682 [details]
Patch

Updated to address WPE build failure.

[John Wilander]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 371691 [details]
Patch

[Brent Fulgham]

(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.

[Brent Fulgham]

Created attachment 371697 [details]
Patch

[Maciej Stachowiak]

Comment on attachment 371697 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 371697 [details]
Patch

Clearing flags on attachment: 371697

Committed r246248: <https://trac.webkit.org/changeset/246248>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.