Bug 195747 - [PSON] Make sure the WebProcessCache is leveraged when relaunching a process after termination
Summary: [PSON] Make sure the WebProcessCache is leveraged when relaunching a process ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 195758
  Show dependency treegraph
 
Reported: 2019-03-14 08:57 PDT by Chris Dumez
Modified: 2019-03-15 14:07 PDT (History)
8 users (show)

See Also:


Attachments
Patch (18.20 KB, patch)
2019-03-14 09:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (18.18 KB, patch)
2019-03-14 11:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>