[WK2] WebPageProxy loadURL() won't work when called just after terminateProcess()
authorbenjamin@webkit.org <benjamin@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Apr 2013 23:29:13 +0000 (23:29 +0000)
committerbenjamin@webkit.org <benjamin@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Apr 2013 23:29:13 +0000 (23:29 +0000)
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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@148312 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
Source/WebKit2/ChangeLog
Source/WebKit2/UIProcess/WebPageProxy.cpp
Source/WebKit2/UIProcess/WebPageProxy.h
Source/WebKit2/UIProcess/WebProcessProxy.cpp
Source/WebKit2/UIProcess/WebProcessProxy.h
Tools/ChangeLog
Tools/TestWebKitAPI/GNUmakefile.am
Tools/TestWebKitAPI/PlatformEfl.cmake
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp [new file with mode: 0644]
Tools/TestWebKitAPI/Tests/WebKit2/MouseMoveAfterCrash.cpp
Tools/TestWebKitAPI/Tests/WebKit2/WebKit2.pro

index e13f12f..ae43e34 100644 (file)
@@ -1,3 +1,42 @@
+2013-04-12  Adenilson Cavalcanti  <cavalcantii@gmail.com>
+
+        [WK2] WebPageProxy loadURL() won't work when called just after terminateProcess()
+        https://bugs.webkit.org/show_bug.cgi?id=110743
+
+        Reviewed by Benjamin Poulain.
+
+        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):
+
 2013-04-12  Gavin Barraclough  <barraclough@apple.com>
 
         Add contentAnchor to WKView
index 8517340..342b0a9 100644 (file)
@@ -1567,7 +1567,8 @@ void WebPageProxy::terminateProcess()
     if (!m_isValid)
         return;
 
-    m_process->terminate();
+    m_process->requestTermination();
+    resetStateAfterProcessExited();
 }
 
 #if !USE(CF) || defined(BUILDING_QT__)
@@ -3798,8 +3799,17 @@ void WebPageProxy::processDidBecomeResponsive()
 
 void WebPageProxy::processDidCrash()
 {
-    ASSERT(m_pageClient);
+    ASSERT(m_isValid);
+
+    resetStateAfterProcessExited();
+
+    m_pageClient->processDidCrash();
+    m_loaderClient.processDidCrash(this);
+}
 
+void WebPageProxy::resetStateAfterProcessExited()
+{
+    ASSERT(m_pageClient);
     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
 
     m_isValid = false;
@@ -3886,16 +3896,11 @@ void WebPageProxy::processDidCrash()
 
     m_pendingLearnOrIgnoreWordMessageCount = 0;
 
-    m_pageClient->processDidCrash();
-    m_loaderClient.processDidCrash(this);
-
-    if (!m_isValid) {
-        // If the call out to the loader client didn't cause the web process to be relaunched, 
-        // we'll call setNeedsDisplay on the view so that we won't have the old contents showing.
-        // If the call did cause the web process to be relaunched, we'll keep the old page contents showing
-        // until the new web process has painted its contents.
-        setViewNeedsDisplay(IntRect(IntPoint(), viewSize()));
-    }
+    // If the call out to the loader client didn't cause the web process to be relaunched,
+    // we'll call setNeedsDisplay on the view so that we won't have the old contents showing.
+    // If the call did cause the web process to be relaunched, we'll keep the old page contents showing
+    // until the new web process has painted its contents.
+    setViewNeedsDisplay(IntRect(IntPoint(), viewSize()));
 
     // Can't expect DidReceiveEvent notifications from a crashed web process.
     m_keyEventQueue.clear();
index 2f09921..f8926f1 100644 (file)
@@ -777,6 +777,8 @@ public:
 private:
     WebPageProxy(PageClient*, PassRefPtr<WebProcessProxy>, WebPageGroup*, uint64_t pageID);
 
+    void resetStateAfterProcessExited();
+
     // CoreIPC::MessageReceiver
     virtual void didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&) OVERRIDE;
     virtual void didReceiveSyncMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&, OwnPtr<CoreIPC::MessageEncoder>&) OVERRIDE;
index 6c648dd..0dc11d7 100644 (file)
@@ -626,4 +626,11 @@ void WebProcessProxy::pagePreferencesChanged(WebKit::WebPageProxy *page)
 #endif
 }
 
+void WebProcessProxy::requestTermination()
+{
+    ChildProcessProxy::terminate();
+    webConnection()->didClose();
+    disconnect();
+}
+
 } // namespace WebKit
index d335ce1..67ef4cf 100644 (file)
@@ -115,6 +115,8 @@ public:
     void updateProcessSuppressionState();
 #endif
 
+    void requestTermination();
+
 private:
     explicit WebProcessProxy(PassRefPtr<WebContext>);
 
index e036f27..8ca5ab8 100644 (file)
@@ -1,3 +1,31 @@
+2013-04-12  Adenilson Cavalcanti  <cavalcantii@gmail.com>
+
+        [WK2] WebPageProxy loadURL() won't work when called just after terminateProcess()
+        https://bugs.webkit.org/show_bug.cgi?id=110743
+
+        Reviewed by Benjamin Poulain.
+
+        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:
+
 2013-04-12  Ryosuke Niwa  <rniwa@webkit.org>
 
         [Mac] REGRESSION: Auto substitution strips new lines
index 344f60b..9b20e03 100644 (file)
@@ -155,6 +155,7 @@ Programs_TestWebKitAPI_TestWebKit2_SOURCES = \
        Tools/TestWebKitAPI/Tests/WebKit2/InjectedBundleInitializationUserDataCallbackWins.cpp \
        Tools/TestWebKitAPI/Tests/WebKit2/LoadAlternateHTMLStringWithNonDirectoryURL.cpp \
        Tools/TestWebKitAPI/Tests/WebKit2/LoadCanceledNoServerRedirectCallback.cpp \
+       Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp \
        Tools/TestWebKitAPI/Tests/WebKit2/MouseMoveAfterCrash.cpp \
        Tools/TestWebKitAPI/Tests/WebKit2/ReloadPageAfterCrash.cpp \
        Tools/TestWebKitAPI/Tests/WebKit2/ResizeWindowAfterCrash.cpp \
index 1b4e960..b48b004 100644 (file)
@@ -68,6 +68,7 @@ set(test_webkit2_api_BINARIES
     InjectedBundleInitializationUserDataCallbackWins
     LoadAlternateHTMLStringWithNonDirectoryURL
     LoadCanceledNoServerRedirectCallback
+    LoadPageOnCrash
     MouseMoveAfterCrash
     ReloadPageAfterCrash
     ResizeWindowAfterCrash
index 5f66b09..660868d 100644 (file)
@@ -91,6 +91,7 @@
                81B50193140F232300D9EB58 /* StringBuilder.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 81B50192140F232300D9EB58 /* StringBuilder.cpp */; };
                8A2C750E16CED9550024F352 /* ResizeWindowAfterCrash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8A2C750D16CED9550024F352 /* ResizeWindowAfterCrash.cpp */; };
                8A3AF93B16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8A3AF93A16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp */; };
+               8AA28C1A16D2FA7B002FF4DB /* LoadPageOnCrash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8AA28C1916D2FA7B002FF4DB /* LoadPageOnCrash.cpp */; };
                930AD402150698D00067970F /* lots-of-text.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 930AD401150698B30067970F /* lots-of-text.html */; };
                9318778915EEC57700A9CCE3 /* NewFirstVisuallyNonEmptyLayoutForImages.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 93AF4ECA1506F035007FD57E /* NewFirstVisuallyNonEmptyLayoutForImages.cpp */; };
                9361002914DC95A70061379D /* lots-of-iframes.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 9361002814DC957B0061379D /* lots-of-iframes.html */; };
                81B50192140F232300D9EB58 /* StringBuilder.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = StringBuilder.cpp; path = WTF/StringBuilder.cpp; sourceTree = "<group>"; };
                8A2C750D16CED9550024F352 /* ResizeWindowAfterCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ResizeWindowAfterCrash.cpp; sourceTree = "<group>"; };
                8A3AF93A16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ReloadPageAfterCrash.cpp; sourceTree = "<group>"; };
+               8AA28C1916D2FA7B002FF4DB /* LoadPageOnCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = LoadPageOnCrash.cpp; sourceTree = "<group>"; };
                8DD76FA10486AA7600D96B5E /* TestWebKitAPI */ = {isa = PBXFileReference; explicitFileType = "compiled.mach-o.executable"; includeInIndex = 0; path = TestWebKitAPI; sourceTree = BUILT_PRODUCTS_DIR; };
                930AD401150698B30067970F /* lots-of-text.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "lots-of-text.html"; sourceTree = "<group>"; };
                9361002814DC957B0061379D /* lots-of-iframes.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "lots-of-iframes.html"; sourceTree = "<group>"; };
                                52CB47401448FB9300873995 /* LoadAlternateHTMLStringWithNonDirectoryURL.cpp */,
                                33DC8910141953A300747EF7 /* LoadCanceledNoServerRedirectCallback.cpp */,
                                33DC89131419579F00747EF7 /* LoadCanceledNoServerRedirectCallback_Bundle.cpp */,
+                               8AA28C1916D2FA7B002FF4DB /* LoadPageOnCrash.cpp */,
                                33BE5AF4137B5A6C00705813 /* MouseMoveAfterCrash.cpp */,
                                33BE5AF8137B5AAE00705813 /* MouseMoveAfterCrash_Bundle.cpp */,
                                93F1DB3014DA20760024C362 /* NewFirstVisuallyNonEmptyLayout.cpp */,
                                26300B1816755CD90066886D /* ListHashSet.cpp in Sources */,
                                52CB47411448FB9300873995 /* LoadAlternateHTMLStringWithNonDirectoryURL.cpp in Sources */,
                                33DC8911141953A300747EF7 /* LoadCanceledNoServerRedirectCallback.cpp in Sources */,
+                               8AA28C1A16D2FA7B002FF4DB /* LoadPageOnCrash.cpp in Sources */,
                                B4039F9D15E6D8B3007255D6 /* MathExtras.cpp in Sources */,
                                CD5497B415857F0C00B5BC30 /* MediaTime.cpp in Sources */,
                                E1220DA0155B25480013E2FC /* MemoryCacheDisableWithinResourceLoadDelegate.mm in Sources */,
diff --git a/Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp b/Tools/TestWebKitAPI/Tests/WebKit2/LoadPageOnCrash.cpp
new file mode 100644 (file)
index 0000000..f3d1830
--- /dev/null
@@ -0,0 +1,102 @@
+/*
+ * Copyright (C) 2013 Adenilson Cavalcanti <cavalcantii@gmail.com>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "PlatformUtilities.h"
+#include "PlatformWebView.h"
+#include "Test.h"
+#include <WebKit2/WKRetainPtr.h>
+
+namespace TestWebKitAPI {
+
+static void didFinishLoad(WKPageRef, WKFrameRef, WKTypeRef, const void*);
+
+class WebKit2CrashLoader {
+public:
+    WebKit2CrashLoader()
+        : context(AdoptWK, WKContextCreate())
+        , webView(context.get())
+        , url(adoptWK(WKURLCreateWithUTF8CString("about:blank")))
+        , firstSuccessfulLoad(false)
+        , secondSuccessfulLoad(false)
+    {
+        memset(&loaderClient, 0, sizeof(loaderClient));
+        loaderClient.clientInfo = this;
+        loaderClient.didFinishLoadForFrame = didFinishLoad;
+        WKPageSetPageLoaderClient(webView.page(), &loaderClient);
+    }
+
+    void loadUrl()
+    {
+        WKPageLoadURL(webView.page(), url.get());
+    }
+
+    void terminateWebProcess()
+    {
+        WKPageTerminate(webView.page());
+    }
+
+    WKRetainPtr<WKContextRef> context;
+    WKPageLoaderClient loaderClient;
+    PlatformWebView webView;
+    WKRetainPtr<WKURLRef> url;
+
+    bool firstSuccessfulLoad;
+    bool secondSuccessfulLoad;
+};
+
+// We are going to have 2 load events intertwined by a simulated crash
+// (i.e. Load -> Crash -> Load).
+void didFinishLoad(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void* clientInfo)
+{
+    WebKit2CrashLoader* testHelper = const_cast<WebKit2CrashLoader*>(static_cast<const WebKit2CrashLoader*>(clientInfo));
+
+    // First load worked, let's crash WebProcess.
+    if (!testHelper->firstSuccessfulLoad) {
+        testHelper->firstSuccessfulLoad = true;
+        return;
+    }
+
+    // Second load worked, we are done.
+    EXPECT_TRUE(testHelper->firstSuccessfulLoad);
+    if (!testHelper->secondSuccessfulLoad) {
+        testHelper->secondSuccessfulLoad = true;
+        return;
+    }
+}
+
+// This test will load a blank page and next kill WebProcess, the expected
+// result is that a call to page load should spawn a new WebProcess.
+TEST(WebKit2, LoadPageAfterCrash)
+{
+    WebKit2CrashLoader helper;
+    helper.loadUrl();
+    Util::run(&helper.firstSuccessfulLoad);
+    helper.terminateWebProcess();
+    helper.loadUrl();
+    Util::run(&helper.secondSuccessfulLoad);
+}
+
+} // namespace TestWebKitAPI
index 9b47051..4bf5527 100644 (file)
@@ -37,11 +37,6 @@ static void didFinishLoadForFrame(WKPageRef, WKFrameRef, WKTypeRef, const void*)
     didFinishLoad = true;
 }
 
-static void processDidCrash(WKPageRef page, const void*)
-{
-    WKPageReload(page);
-}
-
 static void setPageLoaderClient(WKPageRef page)
 {
     WKPageLoaderClient loaderClient;
@@ -49,7 +44,6 @@ static void setPageLoaderClient(WKPageRef page)
     loaderClient.version = 0;
     loaderClient.clientInfo = 0;
     loaderClient.didFinishLoadForFrame = didFinishLoadForFrame;
-    loaderClient.processDidCrash = processDidCrash;
 
     WKPageSetPageLoaderClient(page, &loaderClient);
 }
@@ -79,11 +73,11 @@ TEST(WebKit2, MouseMoveAfterCrash)
     webView.simulateMouseMove(10, 10);
     webView.simulateMouseMove(20, 20);
 
-    // After moving the mouse (while the web process was hung on the Pause message), kill the web process. It is restarted in
-    // processDidCrash by reloading the page.
+    // 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.
     WKPageTerminate(webView.page());
+    WKPageReload(webView.page());
 
-    // Wait until we load the page a second time (via reloading the page in processDidCrash).
+    // Wait until we load the page a second time.
     Util::run(&didFinishLoad);
 
     EXPECT_JS_FALSE(webView.page(), "didMoveMouse()");
index 9452a5a..1d06379 100644 (file)
@@ -19,6 +19,7 @@ SOURCES += \
     InjectedBundleInitializationUserDataCallbackWins.cpp \
     LoadAlternateHTMLStringWithNonDirectoryURL.cpp \
     LoadCanceledNoServerRedirectCallback.cpp \
+    LoadPageOnCrash.cpp \
     MouseMoveAfterCrash.cpp \
     PageLoadBasic.cpp \
     PageLoadDidChangeLocationWithinPageForFrame.cpp \