Changeset 239333 in webkit


Ignore:
Timestamp:
Dec 18, 2018 7:26:50 AM (5 years ago)
Author:
Chris Dumez
Message:

Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
​https://bugs.webkit.org/show_bug.cgi?id=192772

Reviewed by Antti Koivisto.

Source/WebKit:

With r239182, if the page in the previous process would fail to enter PageCache, we would destroy
the corresponding SuspendedPageProxy, which would potentially terminate the process. This would
regress performance when trying to navigate back in history to that page. This would also regress
performance when link-navigating to the same domain as we would have previously reused the suspended
page's process for such navigation.

Address the issue by keeping the SuspendedPageProxy alive even if the WebPage fails to suspend.
When trying to reuse a SuspendedPageProxy, if the page failed to suspend, reuse its process but
not the suspended page itself.

  • UIProcess/SuspendedPageProxy.cpp:

(WebKit::SuspendedPageProxy::~SuspendedPageProxy):
(WebKit::SuspendedPageProxy::waitUntilReadyToUnsuspend):
(WebKit::SuspendedPageProxy::unsuspend):
(WebKit::SuspendedPageProxy::didSuspend):
(WebKit::SuspendedPageProxy::didFailToSuspend):
(WebKit::SuspendedPageProxy::loggingString const):

  • UIProcess/SuspendedPageProxy.h:
  • UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::swapToWebProcess):

  • UIProcess/WebProcessPool.cpp:

(WebKit::WebProcessPool::processForNavigationInternal):

Tools:

Add API test coverage.

  • TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r239323 r239333  
     12018-12-18  Chris Dumez  <cdumez@apple.com>
     2
     3        Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
     4        https://bugs.webkit.org/show_bug.cgi?id=192772
     5
     6        Reviewed by Antti Koivisto.
     7
     8        With r239182, if the page in the previous process would fail to enter PageCache, we would destroy
     9        the corresponding SuspendedPageProxy, which would potentially terminate the process. This would
     10        regress performance when trying to navigate back in history to that page. This would also regress
     11        performance when link-navigating to the same domain as we would have previously reused the suspended
     12        page's process for such navigation.
     13
     14        Address the issue by keeping the SuspendedPageProxy alive even if the WebPage fails to suspend.
     15        When trying to reuse a SuspendedPageProxy, if the page failed to suspend, reuse its process but
     16        not the suspended page itself.
     17
     18        * UIProcess/SuspendedPageProxy.cpp:
     19        (WebKit::SuspendedPageProxy::~SuspendedPageProxy):
     20        (WebKit::SuspendedPageProxy::waitUntilReadyToUnsuspend):
     21        (WebKit::SuspendedPageProxy::unsuspend):
     22        (WebKit::SuspendedPageProxy::didSuspend):
     23        (WebKit::SuspendedPageProxy::didFailToSuspend):
     24        (WebKit::SuspendedPageProxy::loggingString const):
     25        * UIProcess/SuspendedPageProxy.h:
     26        * UIProcess/WebPageProxy.cpp:
     27        (WebKit::WebPageProxy::swapToWebProcess):
     28        * UIProcess/WebProcessPool.cpp:
     29        (WebKit::WebProcessPool::processForNavigationInternal):
     30
    1312018-12-17  Jiewen Tan  <jiewen_tan@apple.com>
    232

  • trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp

    r239182 r239333  
    9595        m_readyToUnsuspendHandler(nullptr);
    9696
    97     if (!m_isSuspended)
     97    if (m_suspensionState == SuspensionState::Resumed)
    9898        return;
    9999
    … …  
    115115        m_readyToUnsuspendHandler(nullptr);
    116116
    117     if (!m_finishedSuspending)
     117    switch (m_suspensionState) {
     118    case SuspensionState::Suspending:
    118119        m_readyToUnsuspendHandler = WTFMove(completionHandler);
    119     else
     120        break;
     121    case SuspensionState::FailedToSuspend:
     122    case SuspensionState::Suspended:
    120123        completionHandler(this);
     124        break;
     125    case SuspensionState::Resumed:
     126        ASSERT_NOT_REACHED();
     127        completionHandler(nullptr);
     128        break;
     129    }
    121130}
    122131
    123132void SuspendedPageProxy::unsuspend()
    124133{
    125     ASSERT(m_isSuspended);
    126     ASSERT(m_finishedSuspending);
     134    ASSERT(m_suspensionState == SuspensionState::Suspended);
    127135
    128     m_isSuspended = false;
     136    m_suspensionState = SuspensionState::Resumed;
    129137    m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
    130138    m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
    … …  
    135143    LOG(ProcessSwapping, "SuspendedPageProxy %s from process %i finished transition to suspended", loggingString(), m_process->processIdentifier());
    136144
    137     m_finishedSuspending = true;
     145    ASSERT(m_suspensionState == SuspensionState::Suspending);
     146    m_suspensionState = SuspensionState::Suspended;
    138147
    139148#if PLATFORM(IOS_FAMILY)
    … …  
    147156void SuspendedPageProxy::didFailToSuspend()
    148157{
    149     // We are unusable due to failure to suspend so remove ourselves from the WebProcessPool.
    150     auto protectedThis = m_process->processPool().takeSuspendedPage(*this);
     158    ASSERT(m_suspensionState == SuspensionState::Suspending);
     159    m_suspensionState = SuspensionState::FailedToSuspend;
    151160
    152161    if (m_readyToUnsuspendHandler)
    153         m_readyToUnsuspendHandler(nullptr);
     162        m_readyToUnsuspendHandler(this);
    154163}
    155164
    … …  
    181190const char* SuspendedPageProxy::loggingString() const
    182191{
    183     return debugString("(", String::format("%p", this), " page ID ", String::number(m_page.pageID()), ", m_finishedSuspending ", String::number(m_finishedSuspending), ")");
     192    return debugString("(", String::format("%p", this), " page ID ", String::number(m_page.pageID()), ", m_suspensionState ", String::number(static_cast<unsigned>(m_suspensionState)), ")");
    184193}
    185194#endif

  • trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h

    r239182 r239333  
    4848    const String& registrableDomain() const { return m_registrableDomain; }
    4949
     50    bool failedToSuspend() const { return m_suspensionState == SuspensionState::FailedToSuspend; }
     51
    5052    void waitUntilReadyToUnsuspend(CompletionHandler<void(SuspendedPageProxy*)>&&);
    5153    void unsuspend();
    … …  
    6870    String m_registrableDomain;
    6971
    70     bool m_isSuspended { true };
    71 
    72     bool m_finishedSuspending { false };
     72    enum class SuspensionState : uint8_t { Suspending, FailedToSuspend, Suspended, Resumed };
     73    SuspensionState m_suspensionState { SuspensionState::Suspending };
    7374    CompletionHandler<void(SuspendedPageProxy*)> m_readyToUnsuspendHandler;
    7475#if PLATFORM(IOS_FAMILY)

  • trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

    r239305 r239333  
    781781    // already exists and already has a main frame.
    782782    if (destinationSuspendedPage) {
    783         ASSERT(!m_mainFrame);
    784         ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
    785         destinationSuspendedPage->unsuspend();
    786         m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
    787         m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
     783        if (!destinationSuspendedPage->failedToSuspend()) {
     784            ASSERT(!m_mainFrame);
     785            ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
     786            destinationSuspendedPage->unsuspend();
     787            m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
     788            m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
     789        } else {
     790            // We failed to suspend the page so destroy it now and merely reuse its WebContent process.
     791            destinationSuspendedPage = nullptr;
     792        }
    788793    }
    789794

  • trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

    r239277 r239333  
    21782178            return suspendedPage->waitUntilReadyToUnsuspend([createNewProcess = WTFMove(createNewProcess), completionHandler = WTFMove(completionHandler)](SuspendedPageProxy* suspendedPage) mutable {
    21792179                if (!suspendedPage)
    2180                     return completionHandler(createNewProcess(), nullptr, "Using new process because target back/forward item's process failed to suspend"_s);
     2180                    return completionHandler(createNewProcess(), nullptr, "Using new process because target back/forward item's suspended page is not reusable"_s);
    21812181                Ref<WebProcessProxy> process = suspendedPage->process();
    21822182                completionHandler(WTFMove(process), suspendedPage, "Using target back/forward item's process"_s);

  • trunk/Tools/ChangeLog

    r239329 r239333  
     12018-12-18  Chris Dumez  <cdumez@apple.com>
     2
     3        Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
     4        https://bugs.webkit.org/show_bug.cgi?id=192772
     5
     6        Reviewed by Antti Koivisto.
     7
     8        Add API test coverage.
     9
     10        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
     11
    1122018-12-18  Philippe Normand  <pnormand@igalia.com>
    213

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

    r239223 r239333  
    17931793}
    17941794
     1795static const char* failsToEnterPageCacheTestBytes = R"PSONRESOURCE(
     1796<body>
     1797<script>
     1798// Pages with dedicated workers do not go into page cache.
     1799var myWorker = new Worker('worker.js');
     1800</script>
     1801</body>
     1802)PSONRESOURCE";
     1803
     1804TEST(ProcessSwap, ReuseSuspendedProcessEvenIfPageCacheFails)
     1805{
     1806    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
     1807    [processPoolConfiguration setProcessSwapsOnNavigation:YES];
     1808    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
     1809
     1810    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
     1811    [webViewConfiguration setProcessPool:processPool.get()];
     1812    auto handler = adoptNS([[PSONScheme alloc] init]);
     1813    [handler addMappingFromURLString:@"pson://www.webkit.org/main1.html" toData:failsToEnterPageCacheTestBytes];
     1814    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
     1815
     1816    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
     1817    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
     1818    [webView setNavigationDelegate:delegate.get()];
     1819
     1820    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
     1821    [webView loadRequest:request];
     1822
     1823    TestWebKitAPI::Util::run(&done);
     1824    done = false;
     1825
     1826    auto webkitPID = [webView _webProcessIdentifier];
     1827
     1828    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main1.html"]];
     1829    [webView loadRequest:request];
     1830
     1831    TestWebKitAPI::Util::run(&done);
     1832    done = false;
     1833
     1834    auto applePID = [webView _webProcessIdentifier];
     1835
     1836    EXPECT_NE(webkitPID, applePID);
     1837
     1838    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
     1839    [webView loadRequest:request];
     1840
     1841    TestWebKitAPI::Util::run(&done);
     1842    done = false;
     1843
     1844    // We should have gone back to the webkit.org process for this load since we reuse SuspendedPages' process when possible.
     1845    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
     1846
     1847    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main2.html"]];
     1848    [webView loadRequest:request];
     1849
     1850    TestWebKitAPI::Util::run(&done);
     1851    done = false;
     1852
     1853    // We should have gone back to the apple.com process for this load since we reuse SuspendedPages' process when possible.
     1854    EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
     1855}
     1856
     1857TEST(ProcessSwap, ReuseSuspendedProcessOnBackEvenIfPageCacheFails)
     1858{
     1859    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
     1860    [processPoolConfiguration setProcessSwapsOnNavigation:YES];
     1861    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
     1862
     1863    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
     1864    [webViewConfiguration setProcessPool:processPool.get()];
     1865    auto handler = adoptNS([[PSONScheme alloc] init]);
     1866    [handler addMappingFromURLString:@"pson://www.webkit.org/main1.html" toData:failsToEnterPageCacheTestBytes];
     1867    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
     1868
     1869    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
     1870    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
     1871    [webView setNavigationDelegate:delegate.get()];
     1872
     1873    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main1.html"]];
     1874    [webView loadRequest:request];
     1875
     1876    TestWebKitAPI::Util::run(&done);
     1877    done = false;
     1878
     1879    auto webkitPID = [webView _webProcessIdentifier];
     1880
     1881    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main1.html"]];
     1882    [webView loadRequest:request];
     1883
     1884    TestWebKitAPI::Util::run(&done);
     1885    done = false;
     1886
     1887    auto applePID = [webView _webProcessIdentifier];
     1888
     1889    EXPECT_NE(webkitPID, applePID);
     1890
     1891    [webView goBack];
     1892
     1893    TestWebKitAPI::Util::run(&done);
     1894    done = false;
     1895
     1896    EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
     1897}
     1898
    17951899static const char* mainFramesOnlyMainFrame = R"PSONRESOURCE(
    17961900<script>
Note: See TracChangeset for help on using the changeset viewer.