Bug 195747

Summary: [PSON] Make sure the WebProcessCache is leveraged when relaunching a process after termination
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, commit-queue, ggaren, koivisto, sroberts, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 195758    
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2019-03-14 08:57:21 PDT 
Make sure the WebProcessCache is leverage when relaunching a process after termination (e.g. crash).
Comment 1 Chris Dumez 2019-03-14 09:27:14 PDT 
Created attachment 364662 [details]
Patch
Comment 2 Geoffrey Garen 2019-03-14 10:55:13 PDT 
Comment on attachment 364662 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/WebPageProxy.cpp:737
> +void WebPageProxy::reattachToWebProcess(const RegistrableDomain& registrableDomain)

At some point we should probably rename this family of functions to downplay the idea that there used to be a web process. Maybe this should be "attachToWebProcess" or "launchWebProcess".

> Source/WebKit/UIProcess/WebProcessPool.cpp:1145
> +WebProcessProxy& WebProcessPool::getNewWebProcessForNavigation(WebsiteDataStore& websiteDataStore, WebPageProxy* page, const RegistrableDomain& registrableDomain)

I would call this "webProcessForNavigation".

We usually use the "get" prefix for out parameters.

Also, we might return an old web process that's in cache.
Comment 3 Chris Dumez 2019-03-14 11:00:44 PDT 
Comment on attachment 364662 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:1145
>> +WebProcessProxy& WebProcessPool::getNewWebProcessForNavigation(WebsiteDataStore& websiteDataStore, WebPageProxy* page, const RegistrableDomain& registrableDomain)
> 
> I would call this "webProcessForNavigation".
> 
> We usually use the "get" prefix for out parameters.
> 
> Also, we might return an old web process that's in cache.

The issue is that there is the pre-existing WebProcessPool::processForNavigation() which is called on navigation for PSON. WebProcessPool::processForNavigation() then calls this method when it wants to swap. So while I agree we need a better name, I think "webProcessForNavigation" would not work.
Comment 4 Geoffrey Garen 2019-03-14 11:05:37 PDT 
Comment on attachment 364662 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebProcessPool.cpp:1145
>>> +WebProcessProxy& WebProcessPool::getNewWebProcessForNavigation(WebsiteDataStore& websiteDataStore, WebPageProxy* page, const RegistrableDomain& registrableDomain)
>> 
>> I would call this "webProcessForNavigation".
>> 
>> We usually use the "get" prefix for out parameters.
>> 
>> Also, we might return an old web process that's in cache.
> 
> The issue is that there is the pre-existing WebProcessPool::processForNavigation() which is called on navigation for PSON. WebProcessPool::processForNavigation() then calls this method when it wants to swap. So while I agree we need a better name, I think "webProcessForNavigation" would not work.

Maybe just call this webProcess, or webProcessForDomain?
Comment 5 Chris Dumez 2019-03-14 11:46:22 PDT 
Created attachment 364669 [details]
Patch
Comment 6 Chris Dumez 2019-03-14 12:11:56 PDT 
Comment on attachment 364669 [details]
Patch

Clearing flags on attachment: 364669

Committed r242952: <https://trac.webkit.org/changeset/242952>
Comment 7 Chris Dumez 2019-03-14 12:11:58 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-03-14 12:13:18 PDT 
<rdar://problem/48896266>
Comment 9 Shawn Roberts 2019-03-15 11:27:39 PDT 
Rolled out in https://trac.webkit.org/changeset/243003/webkit

Causing API failures on iOS Simulator

TestWebKitAPI.ProcessSwap.UseWebProcessCacheAfterTermination
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        
        /Volumes/Data/slave/ios-simulator-12-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:3238
        Expected equality of these values:
          1U
            Which is: 1
          [processPool _webProcessCountIgnoringPrewarmed]
            Which is: 0
        
        
        /Volumes/Data/slave/ios-simulator-12-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:3251
        Expected equality of these values:
          2U
            Which is: 2
          [processPool _webProcessCountIgnoringPrewarmed]
            Which is: 1
        
        
        /Volumes/Data/slave/ios-simulator-12-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:3269
        Expected equality of these values:
          webkitPID
            Which is: 4276
          [webView _webProcessIdentifier]
            Which is: 4279
Comment 10 Chris Dumez 2019-03-15 14:07:34 PDT 
Committed r243009: <https://trac.webkit.org/changeset/243009>