Changeset 148312 in webkit


Ignore:
Timestamp:
Apr 12, 2013 4:29:13 PM (11 years ago)
Author:
benjamin@webkit.org
Message:

[WK2] WebPageProxy loadURL() won't work when called just after terminateProcess()
​https://bugs.webkit.org/show_bug.cgi?id=110743

Patch by Adenilson Cavalcanti <​cavalcantii@gmail.com> on 2013-04-12
Reviewed by Benjamin Poulain.

Source/WebKit2:

A call to loadURL() just after terminating WebProcess will fail thanks to
WebPageProxy being in an undefined state since it is in the middle of its own
cleanup after process termination.

To properly fix this, not only WebPageProxy cleanup should be made
at WebProcess termination request, but also WebProcessProxy needs
to only return to its caller after terminating the process and
closing connections. Otherwise, WebPageProxy can even be able to
detect that WebProcess is no longer running, but a call to respawn
the process will fail.

To fix these issues, this patch moves the cleanup code to a shared private function
that is used for both the cases i.e. user termination and real crash. WebProcess
shutdown is done using a new method that ensures that all cleanup was done before
returning.

A last change introduced in this patch is that for user requested termination,
clients are no longer notified of a crash (since it is not a crash).

  • UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::terminateProcess):
(WebKit::WebPageProxy::processDidCrash):
(WebKit):
(WebKit::WebPageProxy::resetStateAfterProcessExited):

  • UIProcess/WebPageProxy.h:

(WebPageProxy):

  • UIProcess/WebProcessProxy.cpp:

(WebKit::WebProcessProxy::userRequestedTerminate):
(WebKit):

  • UIProcess/WebProcessProxy.h:

(WebProcessProxy):

Tools:

Adding a new test file to check if loading a page just after WebProcess
has crashed (or was terminated) works. The test executes the
following steps (Load, Crash, Load), thus stressing WebProcess
reattach and process termination code path.

  • TestWebKitAPI/GNUmakefile.am:
  • TestWebKitAPI/PlatformEfl.cmake:
  • TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
  • TestWebKitAPI/Tests/WebKit2/MouseMoveAfterCrash.cpp:

(TestWebKitAPI::setPageLoaderClient):
(TestWebKitAPI::TEST):

  • TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp: Added.

(TestWebKitAPI):
(WebKit2CrashLoader):
(TestWebKitAPI::WebKit2CrashLoader::WebKit2CrashLoader):
(TestWebKitAPI::WebKit2CrashLoader::loadUrl):
(TestWebKitAPI::WebKit2CrashLoader::crashWebProcess):
(TestWebKitAPI::didFinishLoad):
(TestWebKitAPI::TEST):

  • TestWebKitAPI/Tests/WebKit2/WebKit2.pro:
Location:
trunk
Files:
1 added
11 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit2/ChangeLog

    r148311 r148312  
     12013-04-12  Adenilson Cavalcanti  <cavalcantii@gmail.com>
     2
     3        [WK2] WebPageProxy loadURL() won't work when called just after terminateProcess()
     4        https://bugs.webkit.org/show_bug.cgi?id=110743
     5
     6        Reviewed by Benjamin Poulain.
     7
     8        A call to loadURL() just after terminating WebProcess will fail thanks to
     9        WebPageProxy being in an undefined state since it is in the middle of its own
     10        cleanup after process termination.
     11
     12        To properly fix this, not only WebPageProxy cleanup should be made
     13        at WebProcess termination request, but also WebProcessProxy needs
     14        to only return to its caller after terminating the process and
     15        closing connections. Otherwise, WebPageProxy can even be able to
     16        detect that WebProcess is no longer running, but a call to respawn
     17        the process will fail.
     18
     19        To fix these issues, this patch moves the cleanup code to a shared private function
     20        that is used for both the cases i.e. user termination and real crash. WebProcess
     21        shutdown is done using a new method that ensures that all cleanup was done before
     22        returning.
     23
     24        A last change introduced in this patch is that for user requested termination,
     25        clients are no longer notified of a crash (since it is not a crash).
     26
     27        * UIProcess/WebPageProxy.cpp:
     28        (WebKit::WebPageProxy::terminateProcess):
     29        (WebKit::WebPageProxy::processDidCrash):
     30        (WebKit):
     31        (WebKit::WebPageProxy::resetStateAfterProcessExited):
     32        * UIProcess/WebPageProxy.h:
     33        (WebPageProxy):
     34        * UIProcess/WebProcessProxy.cpp:
     35        (WebKit::WebProcessProxy::userRequestedTerminate):
     36        (WebKit):
     37        * UIProcess/WebProcessProxy.h:
     38        (WebProcessProxy):
     39
    1402013-04-12  Gavin Barraclough  <barraclough@apple.com>
    241

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

    r145845 r148312  
    15681568        return;
    15691569
    1570     m_process->terminate();
     1570    m_process->requestTermination();
     1571    resetStateAfterProcessExited();
    15711572}
    15721573
    … …  
    37993800void WebPageProxy::processDidCrash()
    38003801{
     3802    ASSERT(m_isValid);
     3803
     3804    resetStateAfterProcessExited();
     3805
     3806    m_pageClient->processDidCrash();
     3807    m_loaderClient.processDidCrash(this);
     3808}
     3809
     3810void WebPageProxy::resetStateAfterProcessExited()
     3811{
    38013812    ASSERT(m_pageClient);
    3802 
    38033813    m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
    38043814
    … …  
    38873897    m_pendingLearnOrIgnoreWordMessageCount = 0;
    38883898
    3889     m_pageClient->processDidCrash();
    3890     m_loaderClient.processDidCrash(this);
    3891 
    3892     if (!m_isValid) {
    3893         // If the call out to the loader client didn't cause the web process to be relaunched,
    3894         // we'll call setNeedsDisplay on the view so that we won't have the old contents showing.
    3895         // If the call did cause the web process to be relaunched, we'll keep the old page contents showing
    3896         // until the new web process has painted its contents.
    3897         setViewNeedsDisplay(IntRect(IntPoint(), viewSize()));
    3898     }
     3899    // If the call out to the loader client didn't cause the web process to be relaunched,
     3900    // we'll call setNeedsDisplay on the view so that we won't have the old contents showing.
     3901    // If the call did cause the web process to be relaunched, we'll keep the old page contents showing
     3902    // until the new web process has painted its contents.
     3903    setViewNeedsDisplay(IntRect(IntPoint(), viewSize()));
    38993904
    39003905    // Can't expect DidReceiveEvent notifications from a crashed web process.

  • trunk/Source/WebKit2/UIProcess/WebPageProxy.h

    r148278 r148312  
    778778    WebPageProxy(PageClient*, PassRefPtr<WebProcessProxy>, WebPageGroup*, uint64_t pageID);
    779779
     780    void resetStateAfterProcessExited();
     781
    780782    // CoreIPC::MessageReceiver
    781783    virtual void didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&) OVERRIDE;

  • trunk/Source/WebKit2/UIProcess/WebProcessProxy.cpp

    r147489 r148312  
    627627}
    628628
     629void WebProcessProxy::requestTermination()
     630{
     631    ChildProcessProxy::terminate();
     632    webConnection()->didClose();
     633    disconnect();
     634}
     635
    629636} // namespace WebKit

  • trunk/Source/WebKit2/UIProcess/WebProcessProxy.h

    r142630 r148312  
    116116#endif
    117117
     118    void requestTermination();
     119
    118120private:
    119121    explicit WebProcessProxy(PassRefPtr<WebContext>);

  • trunk/Tools/ChangeLog

    r148306 r148312  
     12013-04-12  Adenilson Cavalcanti  <cavalcantii@gmail.com>
     2
     3        [WK2] WebPageProxy loadURL() won't work when called just after terminateProcess()
     4        https://bugs.webkit.org/show_bug.cgi?id=110743
     5
     6        Reviewed by Benjamin Poulain.
     7
     8        Adding a new test file to check if loading a page just after WebProcess
     9        has crashed (or was terminated) works. The test executes the
     10        following steps (Load, Crash, Load), thus stressing WebProcess
     11        reattach and process termination code path.
     12
     13        * TestWebKitAPI/GNUmakefile.am:
     14        * TestWebKitAPI/PlatformEfl.cmake:
     15        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
     16        * TestWebKitAPI/Tests/WebKit2/MouseMoveAfterCrash.cpp:
     17        (TestWebKitAPI::setPageLoaderClient):
     18        (TestWebKitAPI::TEST):
     19        * TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp: Added.
     20        (TestWebKitAPI):
     21        (WebKit2CrashLoader):
     22        (TestWebKitAPI::WebKit2CrashLoader::WebKit2CrashLoader):
     23        (TestWebKitAPI::WebKit2CrashLoader::loadUrl):
     24        (TestWebKitAPI::WebKit2CrashLoader::crashWebProcess):
     25        (TestWebKitAPI::didFinishLoad):
     26        (TestWebKitAPI::TEST):
     27        * TestWebKitAPI/Tests/WebKit2/WebKit2.pro:
     28
    1292013-04-12  Ryosuke Niwa  <rniwa@webkit.org>
    230

  • trunk/Tools/TestWebKitAPI/GNUmakefile.am

    r145552 r148312  
    156156        Tools/TestWebKitAPI/Tests/WebKit2/LoadAlternateHTMLStringWithNonDirectoryURL.cpp \
    157157        Tools/TestWebKitAPI/Tests/WebKit2/LoadCanceledNoServerRedirectCallback.cpp \
     158        Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp \
    158159        Tools/TestWebKitAPI/Tests/WebKit2/MouseMoveAfterCrash.cpp \
    159160        Tools/TestWebKitAPI/Tests/WebKit2/ReloadPageAfterCrash.cpp \

  • trunk/Tools/TestWebKitAPI/PlatformEfl.cmake

    r148025 r148312  
    6969    LoadAlternateHTMLStringWithNonDirectoryURL
    7070    LoadCanceledNoServerRedirectCallback
     71    LoadPageOnCrash
    7172    MouseMoveAfterCrash
    7273    ReloadPageAfterCrash

  • trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

    r144552 r148312  
    9292                8A2C750E16CED9550024F352 /* ResizeWindowAfterCrash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8A2C750D16CED9550024F352 /* ResizeWindowAfterCrash.cpp */; };
    9393                8A3AF93B16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8A3AF93A16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp */; };
     94                8AA28C1A16D2FA7B002FF4DB /* LoadPageOnCrash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8AA28C1916D2FA7B002FF4DB /* LoadPageOnCrash.cpp */; };
    9495                930AD402150698D00067970F /* lots-of-text.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 930AD401150698B30067970F /* lots-of-text.html */; };
    9596                9318778915EEC57700A9CCE3 /* NewFirstVisuallyNonEmptyLayoutForImages.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 93AF4ECA1506F035007FD57E /* NewFirstVisuallyNonEmptyLayoutForImages.cpp */; };
    … …  
    360361                8A2C750D16CED9550024F352 /* ResizeWindowAfterCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ResizeWindowAfterCrash.cpp; sourceTree = "<group>"; };
    361362                8A3AF93A16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ReloadPageAfterCrash.cpp; sourceTree = "<group>"; };
     363                8AA28C1916D2FA7B002FF4DB /* LoadPageOnCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = LoadPageOnCrash.cpp; sourceTree = "<group>"; };
    362364                8DD76FA10486AA7600D96B5E /* TestWebKitAPI */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.executable"; includeInIndex = 0; path = TestWebKitAPI; sourceTree = BUILT_PRODUCTS_DIR; };
    363365                930AD401150698B30067970F /* lots-of-text.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "lots-of-text.html"; sourceTree = "<group>"; };
    … …  
    663665                                33DC8910141953A300747EF7 /* LoadCanceledNoServerRedirectCallback.cpp */,
    664666                                33DC89131419579F00747EF7 /* LoadCanceledNoServerRedirectCallback_Bundle.cpp */,
     667                                8AA28C1916D2FA7B002FF4DB /* LoadPageOnCrash.cpp */,
    665668                                33BE5AF4137B5A6C00705813 /* MouseMoveAfterCrash.cpp */,
    666669                                33BE5AF8137B5AAE00705813 /* MouseMoveAfterCrash_Bundle.cpp */,
    … …  
    10421045                                52CB47411448FB9300873995 /* LoadAlternateHTMLStringWithNonDirectoryURL.cpp in Sources */,
    10431046                                33DC8911141953A300747EF7 /* LoadCanceledNoServerRedirectCallback.cpp in Sources */,
     1047                                8AA28C1A16D2FA7B002FF4DB /* LoadPageOnCrash.cpp in Sources */,
    10441048                                B4039F9D15E6D8B3007255D6 /* MathExtras.cpp in Sources */,
    10451049                                CD5497B415857F0C00B5BC30 /* MediaTime.cpp in Sources */,

  • trunk/Tools/TestWebKitAPI/Tests/WebKit2/MouseMoveAfterCrash.cpp

    r119257 r148312  
    3838}
    3939
    40 static void processDidCrash(WKPageRef page, const void*)
    41 {
    42     WKPageReload(page);
    43 }
    44 
    4540static void setPageLoaderClient(WKPageRef page)
    4641{
    … …  
    5045    loaderClient.clientInfo = 0;
    5146    loaderClient.didFinishLoadForFrame = didFinishLoadForFrame;
    52     loaderClient.processDidCrash = processDidCrash;
    5347
    5448    WKPageSetPageLoaderClient(page, &loaderClient);
    … …  
    8074    webView.simulateMouseMove(20, 20);
    8175
    82     // After moving the mouse (while the web process was hung on the Pause message), kill the web process. It is restarted in
    83     // processDidCrash by reloading the page.
     76    // After moving the mouse (while the web process was hung on the Pause message), kill the web process. It is restarted by reloading the page.
    8477    WKPageTerminate(webView.page());
     78    WKPageReload(webView.page());
    8579
    86     // Wait until we load the page a second time (via reloading the page in processDidCrash).
     80    // Wait until we load the page a second time.
    8781    Util::run(&didFinishLoad);
    8882

  • trunk/Tools/TestWebKitAPI/Tests/WebKit2/WebKit2.pro

    r143221 r148312  
    2020    LoadAlternateHTMLStringWithNonDirectoryURL.cpp \
    2121    LoadCanceledNoServerRedirectCallback.cpp \
     22    LoadPageOnCrash.cpp \
    2223    MouseMoveAfterCrash.cpp \
    2324    PageLoadBasic.cpp \
Note: See TracChangeset for help on using the changeset viewer.