Attachment 187968 Details for Bug 109305 – Addressing reviewers comments: Changelog, comments, variable names, test name, xcode files ordering.

[patch] Addressing reviewers comments: Changelog, comments, variable names, test name, xcode files ordering.

0007-final.patch (text/plain), 13.45 KB, created by Adenilson Cavalcanti Silva on 2013-02-12 17:09:28 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Adenilson Cavalcanti Silva

Created: 2013-02-12 17:09:28 PST

Size: 13.45 KB

patch

obsolete

>From f5866337b18cabf24b6decec4cb2bf75cd575d14 Mon Sep 17 00:00:00 2001
>From: Adenilson Cavalcanti <cavalcantii@gmail.com>
>Date: Mon, 11 Feb 2013 15:45:50 -0400
>Subject: [PATCH] Following suggested approach, plus test enabled for Mac,
> EFL, GTK, addressed review issues, ensures that test runs
> twice, changes on CL, renamed the test, etc.
>
>Signed-off-by: Adenilson Cavalcanti <cavalcantii@gmail.com>
>---
> Source/WebKit2/ChangeLog                           |   18 +++++
> Source/WebKit2/UIProcess/WebPageProxy.cpp          |   10 +--
> Tools/ChangeLog                                    |   18 +++++
> Tools/TestWebKitAPI/GNUmakefile.am                 |    1 +
> Tools/TestWebKitAPI/PlatformEfl.cmake              |    1 +
> .../TestWebKitAPI.xcodeproj/project.pbxproj        |    7 +-
> .../Tests/WebKit2/ReloadPageAfterCrash.cpp         |   85 ++++++++++++++++++++
> 7 files changed, 133 insertions(+), 7 deletions(-)
> create mode 100644 Tools/TestWebKitAPI/Tests/WebKit2/ReloadPageAfterCrash.cpp
>
>diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
>index c0f408e..9e3697e 100644
>--- a/Source/WebKit2/ChangeLog
>+++ b/Source/WebKit2/ChangeLog
>@@ -1,3 +1,21 @@
>+2013-02-11  Adenilson Cavalcanti  <cavalcantii@gmail.com>
>+
>+        [WK2] Page reloading will crash UIProcess after WebProcess was killed
>+        https://bugs.webkit.org/show_bug.cgi?id=109305
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Re-initialize the pointer to a WebInspectorProxy object before calling
>+        initializeWebPage().
>+
>+        When the WebProcess crashes, WebPageProxy::processDidCrash() will
>+        set WebInspectorProxy pointer to null, which later is accessed by
>+        initializeWebPage(). This patch avoids a crash scenario where
>+        calls into a null pointer would be made.
>+
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::reattachToWebProcess):
>+
> 2013-02-12  Anders Carlsson  <andersca@apple.com>
> 
>         Introduce a WorkQueueMessageReceiver class as a replacement for QueueClient
>diff --git a/Source/WebKit2/UIProcess/WebPageProxy.cpp b/Source/WebKit2/UIProcess/WebPageProxy.cpp
>index 4ac3393..c583e51 100644
>--- a/Source/WebKit2/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit2/UIProcess/WebPageProxy.cpp
>@@ -396,17 +396,17 @@ void WebPageProxy::reattachToWebProcess()
>     m_process->addExistingWebPage(this, m_pageID);
>     m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID, this);
> 
>-    initializeWebPage();
>-
>-    m_pageClient->didRelaunchProcess();
>-    m_drawingArea->waitForBackingStoreUpdateOnNextPaint();
>-
> #if ENABLE(INSPECTOR)
>     m_inspector = WebInspectorProxy::create(this);
> #endif
> #if ENABLE(FULLSCREEN_API)
>     m_fullScreenManager = WebFullScreenManagerProxy::create(this);
> #endif
>+
>+    initializeWebPage();
>+
>+    m_pageClient->didRelaunchProcess();
>+    m_drawingArea->waitForBackingStoreUpdateOnNextPaint();
> }
> 
> void WebPageProxy::reattachToWebProcessWithItem(WebBackForwardListItem* item)
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index c24f249..38902fd 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,21 @@
>+2013-02-11  Adenilson Cavalcanti  <cavalcantii@gmail.com>
>+
>+        [WK2] Page reloading will crash UIProcess after WebProcess was killed
>+        https://bugs.webkit.org/show_bug.cgi?id=109305
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Adding a new test to simulate the case of WebProcess crash followed by a trying
>+        to load a new page.
>+
>+        * TestWebKitAPI/GNUmakefile.am:
>+        * TestWebKitAPI/PlatformEfl.cmake:
>+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
>+        * TestWebKitAPI/Tests/WebKit2/ReloadPageAfterCrash.cpp: Added.
>+        (TestWebKitAPI):
>+        (TestWebKitAPI::didFinishLoad):
>+        (TestWebKitAPI::TEST):
>+
> 2013-02-12  Jochen Eisinger  <jochen@chromium.org>
> 
>         [chromium] move text dump generation to TestRunner library
>diff --git a/Tools/TestWebKitAPI/GNUmakefile.am b/Tools/TestWebKitAPI/GNUmakefile.am
>index 69713a0..aa88133 100644
>--- a/Tools/TestWebKitAPI/GNUmakefile.am
>+++ b/Tools/TestWebKitAPI/GNUmakefile.am
>@@ -152,6 +152,7 @@ Programs_TestWebKitAPI_TestWebKit2_SOURCES = \
> 	Tools/TestWebKitAPI/Tests/WebKit2/LoadAlternateHTMLStringWithNonDirectoryURL.cpp \
> 	Tools/TestWebKitAPI/Tests/WebKit2/LoadCanceledNoServerRedirectCallback.cpp \
> 	Tools/TestWebKitAPI/Tests/WebKit2/MouseMoveAfterCrash.cpp \
>+	Tools/TestWebKitAPI/Tests/WebKit2/ReloadPageAfterCrash.cpp \
> 	Tools/TestWebKitAPI/Tests/WebKit2/NewFirstVisuallyNonEmptyLayout.cpp \
> 	Tools/TestWebKitAPI/Tests/WebKit2/NewFirstVisuallyNonEmptyLayoutFails.cpp \
> 	Tools/TestWebKitAPI/Tests/WebKit2/NewFirstVisuallyNonEmptyLayoutForImages.cpp \
>diff --git a/Tools/TestWebKitAPI/PlatformEfl.cmake b/Tools/TestWebKitAPI/PlatformEfl.cmake
>index cfd9327..d8f5a78 100644
>--- a/Tools/TestWebKitAPI/PlatformEfl.cmake
>+++ b/Tools/TestWebKitAPI/PlatformEfl.cmake
>@@ -69,6 +69,7 @@ set(test_webkit2_api_BINARIES
>     LoadAlternateHTMLStringWithNonDirectoryURL
>     LoadCanceledNoServerRedirectCallback
>     MouseMoveAfterCrash
>+    ReloadPageAfterCrash
>     NewFirstVisuallyNonEmptyLayout
>     NewFirstVisuallyNonEmptyLayoutFails
>     NewFirstVisuallyNonEmptyLayoutForImages
>diff --git a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>index a1cc291..86e01c1 100644
>--- a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>+++ b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>@@ -88,6 +88,7 @@
> 		76E182DD1547569100F1FADD /* WillSendSubmitEvent_Bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 76E182DC1547569100F1FADD /* WillSendSubmitEvent_Bundle.cpp */; };
> 		76E182DF154767E600F1FADD /* auto-submitting-form.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 76E182DE15475A8300F1FADD /* auto-submitting-form.html */; };
> 		81B50193140F232300D9EB58 /* StringBuilder.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 81B50192140F232300D9EB58 /* StringBuilder.cpp */; };
>+		8A3AF93B16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 8A3AF93A16C9ED2700D248C1 /* ReloadPageAfterCrash.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 */; };
>@@ -347,6 +348,7 @@
> 		76E182DC1547569100F1FADD /* WillSendSubmitEvent_Bundle.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WillSendSubmitEvent_Bundle.cpp; sourceTree = "<group>"; };
> 		76E182DE15475A8300F1FADD /* auto-submitting-form.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; path = "auto-submitting-form.html"; sourceTree = "<group>"; };
> 		81B50192140F232300D9EB58 /* StringBuilder.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = StringBuilder.cpp; path = WTF/StringBuilder.cpp; sourceTree = "<group>"; };
>+		8A3AF93A16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ReloadPageAfterCrash.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>"; };
>@@ -665,6 +667,7 @@
> 				C54237ED16B8955800E638FC /* PasteboardNotifications_Bundle.cpp */,
> 				333B9CE11277F23100FEFCE3 /* PreventEmptyUserAgent.cpp */,
> 				F6FDDDD214241AD4004F1729 /* PrivateBrowsingPushStateNoHistoryCallback.cpp */,
>+				8A3AF93A16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp */,
> 				C0BD669C131D3CF700E18F2A /* ResponsivenessTimerDoesntFireEarly.cpp */,
> 				C0BD669E131D3CFF00E18F2A /* ResponsivenessTimerDoesntFireEarly_Bundle.cpp */,
> 				C0ADBE8212FCA6AA00D2C129 /* RestoreSessionStateContainingFormData.cpp */,
>@@ -1017,6 +1020,7 @@
> 				26300B1816755CD90066886D /* ListHashSet.cpp in Sources */,
> 				52CB47411448FB9300873995 /* LoadAlternateHTMLStringWithNonDirectoryURL.cpp in Sources */,
> 				33DC8911141953A300747EF7 /* LoadCanceledNoServerRedirectCallback.cpp in Sources */,
>+				2E7765CD16C4D80A00BA2BB1 /* mainIOS.mm in Sources */,
> 				B4039F9D15E6D8B3007255D6 /* MathExtras.cpp in Sources */,
> 				CD5497B415857F0C00B5BC30 /* MediaTime.cpp in Sources */,
> 				E1220DA0155B25480013E2FC /* MemoryCacheDisableWithinResourceLoadDelegate.mm in Sources */,
>@@ -1040,6 +1044,7 @@
> 				333B9CE21277F23100FEFCE3 /* PreventEmptyUserAgent.cpp in Sources */,
> 				F6FDDDD314241AD4004F1729 /* PrivateBrowsingPushStateNoHistoryCallback.cpp in Sources */,
> 				0FC6C4CC141027E0005B7F0C /* RedBlackTree.cpp in Sources */,
>+				8A3AF93B16C9ED2700D248C1 /* ReloadPageAfterCrash.cpp in Sources */,
> 				3722C8691461E03E00C45D00 /* RenderedImageFromDOMRange.mm in Sources */,
> 				C0BD669D131D3CF700E18F2A /* ResponsivenessTimerDoesntFireEarly.cpp in Sources */,
> 				C0ADBE8312FCA6AA00D2C129 /* RestoreSessionStateContainingFormData.cpp in Sources */,
>@@ -1083,8 +1088,6 @@
> 				BC90995E12567BC100083756 /* WKString.cpp in Sources */,
> 				BC9099941256ACF100083756 /* WKStringJSString.cpp in Sources */,
> 				265AF55015D1E48A00B0CB4A /* WTFString.cpp in Sources */,
>-				2E7765CD16C4D80A00BA2BB1 /* mainIOS.mm in Sources */,
>-				2E7765CF16C4D81100BA2BB1 /* mainMac.mm in Sources */,
> 			);
> 			runOnlyForDeploymentPostprocessing = 0;
> 		};
>diff --git a/Tools/TestWebKitAPI/Tests/WebKit2/ReloadPageAfterCrash.cpp b/Tools/TestWebKitAPI/Tests/WebKit2/ReloadPageAfterCrash.cpp
>new file mode 100644
>index 0000000..7960dcb
>--- /dev/null
>+++ b/Tools/TestWebKitAPI/Tests/WebKit2/ReloadPageAfterCrash.cpp
>@@ -0,0 +1,85 @@
>+/*
>+ * 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 bool loadBeforeCrash = false;
>+static bool loadAfterCrash = false;
>+
>+static void didFinishLoad(WKPageRef page, WKFrameRef frame, WKTypeRef userData, const void* clientInfo)
>+{
>+    // First load before WebProcess was terminated.
>+    if (!loadBeforeCrash) {
>+        loadBeforeCrash = true;
>+        return;
>+    }
>+
>+    // Next load after WebProcess was terminated (hopefully
>+    // it will be correctly re-spawned).
>+    EXPECT_EQ(static_cast<uint32_t>(kWKFrameLoadStateFinished), WKFrameGetFrameLoadState(frame));
>+    EXPECT_FALSE(loadAfterCrash);
>+
>+    // Set it, otherwise the loop will not end.
>+    loadAfterCrash = true;
>+}
>+
>+static void didCrash(WKPageRef page, const void*)
>+{
>+    // Test if first load actually worked.
>+    EXPECT_TRUE(loadBeforeCrash);
>+
>+    // Reload should re-spawn webprocess.
>+    WKPageReload(page);
>+}
>+
>+TEST(WebKit2, ReloadPageAfterCrash)
>+{
>+    WKRetainPtr<WKContextRef> context(AdoptWK, WKContextCreate());
>+    PlatformWebView webView(context.get());
>+
>+    WKPageLoaderClient loaderClient;
>+    memset(&loaderClient, 0, sizeof(loaderClient));
>+    loaderClient.didFinishLoadForFrame = didFinishLoad;
>+    loaderClient.processDidCrash = didCrash;
>+    WKPageSetPageLoaderClient(webView.page(), &loaderClient);
>+
>+    WKRetainPtr<WKURLRef> url = adoptWK(WKURLCreateWithUTF8CString("about:blank"));
>+    // Load a blank page and next kills WebProcess.
>+    WKPageLoadURL(webView.page(), url.get());
>+    Util::run(&loadBeforeCrash);
>+    WKPageTerminate(webView.page());
>+
>+    // Let's try load a page and see what happens.
>+    WKPageLoadURL(webView.page(), url.get());
>+    Util::run(&loadAfterCrash);
>+}
>+
>+} // namespace TestWebKitAPI
>-- 
>1.7.10.4
>

Flags:

benjamin: review+
benjamin: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 109305: 187328 | 187331 | 187438 | 187617 | 187630 | 187686 | 187718 | 187759 | 187767 | 187768 | 187968 | 187975