Attachment 361735 Details for Bug 194522 – Patch

[patch] Patch

bug-194522-20190211170832.patch (text/plain), 14.81 KB, created by Alex Christensen on 2019-02-11 17:08:33 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Christensen

Created: 2019-02-11 17:08:33 PST

Size: 14.81 KB

patch

obsolete

>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 241283)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,24 @@
>+2019-02-11  Alex Christensen  <achristensen@webkit.org>
>+
>+        WebPage::close needs to remove all message receivers associated with that WebPage, not WebPage::~WebPage
>+        https://bugs.webkit.org/show_bug.cgi?id=194522
>+        <rdar://problem/47789393>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The InjectedBundle SPI can retain the WebPage or wrapping objects (WKWebProcessPlugInBrowserContextController/WKBundlePageRef).
>+        This can make it so WebPage::close is called before WebPage::~WebPage, and if the SuspendedPageProxy is reused for a subsequent
>+        navigation to the same domain, the WebProcess is reused with a different WebPage instance with the same PageID, which causes problems
>+        when another WebPage registers message handlers and then the previous WebPage is destroyed, which removes both message handlers.
>+
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::~WebPage):
>+        (WebKit::WebPage::close):
>+        (WebKit::WebPage::mainFrameDidLayout):
>+        * WebProcess/WebPage/WebPage.h:
>+        * WebProcess/WebProcess.h:
>+        (WebKit::WebProcess::eventDispatcher):
>+
> 2019-02-11  Adrian Perez de Castro  <aperez@igalia.com>
> 
>         [GTK][WPE] Add content extensions support in WKTR and unskip layout tests
>Index: Source/WebKit/WebProcess/WebProcess.h
>===================================================================
>--- Source/WebKit/WebProcess/WebProcess.h	(revision 241275)
>+++ Source/WebKit/WebProcess/WebProcess.h	(working copy)
>@@ -165,7 +165,7 @@ public:
>     PluginProcessConnectionManager& pluginProcessConnectionManager();
> #endif
> 
>-    EventDispatcher& eventDispatcher() { return *m_eventDispatcher; }
>+    EventDispatcher& eventDispatcher() { return m_eventDispatcher.get(); }
> 
>     NetworkProcessConnection& ensureNetworkProcessConnection();
>     void networkProcessConnectionClosed(NetworkProcessConnection*);
>@@ -402,7 +402,7 @@ private:
>     HashMap<uint64_t, RefPtr<WebPageGroupProxy>> m_pageGroupMap;
>     RefPtr<InjectedBundle> m_injectedBundle;
> 
>-    RefPtr<EventDispatcher> m_eventDispatcher;
>+    Ref<EventDispatcher> m_eventDispatcher;
> #if PLATFORM(IOS_FAMILY)
>     RefPtr<ViewUpdateDispatcher> m_viewUpdateDispatcher;
> #endif
>Index: Source/WebKit/WebProcess/WebPage/WebPage.cpp
>===================================================================
>--- Source/WebKit/WebProcess/WebPage/WebPage.cpp	(revision 241275)
>+++ Source/WebKit/WebProcess/WebPage/WebPage.cpp	(working copy)
>@@ -364,7 +364,7 @@ WebPage::WebPage(uint64_t pageID, WebPag
> #endif
>     , m_layerHostingMode(parameters.layerHostingMode)
> #if PLATFORM(COCOA) || PLATFORM(GTK)
>-    , m_viewGestureGeometryCollector(makeUniqueRef<ViewGestureGeometryCollector>(*this))
>+    , m_viewGestureGeometryCollector(std::make_unique<ViewGestureGeometryCollector>(*this))
> #elif HAVE(ACCESSIBILITY) && PLATFORM(GTK)
>     , m_accessibilityObject(nullptr)
> #endif
>@@ -750,12 +750,6 @@ WebPage::~WebPage()
> {
>     ASSERT(!m_page);
> 
>-    auto& webProcess = WebProcess::singleton();
>-#if ENABLE(ASYNC_SCROLLING)
>-    if (m_useAsyncScrolling)
>-        webProcess.eventDispatcher().removeScrollingTreeForPage(this);
>-#endif
>-
>     platformDetach();
>     
>     m_sandboxExtensionTracker.invalidate();
>@@ -770,16 +764,6 @@ WebPage::~WebPage()
>         m_footerBanner->detachFromPage();
> #endif // !PLATFORM(IOS_FAMILY)
> 
>-    webProcess.removeMessageReceiver(Messages::WebPage::messageReceiverName(), m_pageID);
>-
>-    // FIXME: This should be done in the object destructors, and the objects themselves should be message receivers.
>-    webProcess.removeMessageReceiver(Messages::WebInspector::messageReceiverName(), m_pageID);
>-    webProcess.removeMessageReceiver(Messages::WebInspectorUI::messageReceiverName(), m_pageID);
>-    webProcess.removeMessageReceiver(Messages::RemoteWebInspectorUI::messageReceiverName(), m_pageID);
>-#if ENABLE(FULLSCREEN_API)
>-    webProcess.removeMessageReceiver(Messages::WebFullScreenManager::messageReceiverName(), m_pageID);
>-#endif
>-
> #ifndef NDEBUG
>     webPageCounter.decrement();
> #endif
>@@ -1318,6 +1302,21 @@ void WebPage::close()
>     bool isRunningModal = m_isRunningModal;
>     m_isRunningModal = false;
> 
>+    auto& webProcess = WebProcess::singleton();
>+#if ENABLE(ASYNC_SCROLLING)
>+    if (m_useAsyncScrolling)
>+        webProcess.eventDispatcher().removeScrollingTreeForPage(this);
>+#endif
>+    webProcess.removeMessageReceiver(Messages::WebPage::messageReceiverName(), m_pageID);
>+    // FIXME: This should be done in the object destructors, and the objects themselves should be message receivers.
>+    webProcess.removeMessageReceiver(Messages::WebInspector::messageReceiverName(), m_pageID);
>+    webProcess.removeMessageReceiver(Messages::WebInspectorUI::messageReceiverName(), m_pageID);
>+    webProcess.removeMessageReceiver(Messages::RemoteWebInspectorUI::messageReceiverName(), m_pageID);
>+#if ENABLE(FULLSCREEN_API)
>+    webProcess.removeMessageReceiver(Messages::WebFullScreenManager::messageReceiverName(), m_pageID);
>+#endif
>+    m_viewGestureGeometryCollector = nullptr;
>+
>     // The WebPage can be destroyed by this call.
>     WebProcess::singleton().removeWebPage(m_pageID);
> 
>@@ -4222,7 +4221,8 @@ void WebPage::mainFrameDidLayout()
>     }
> 
> #if PLATFORM(COCOA) || PLATFORM(GTK)
>-    m_viewGestureGeometryCollector->mainFrameDidLayout();
>+    if (m_viewGestureGeometryCollector)
>+        m_viewGestureGeometryCollector->mainFrameDidLayout();
> #endif
> #if PLATFORM(IOS_FAMILY)
>     if (FrameView* frameView = mainFrameView()) {
>Index: Source/WebKit/WebProcess/WebPage/WebPage.h
>===================================================================
>--- Source/WebKit/WebProcess/WebPage/WebPage.h	(revision 241275)
>+++ Source/WebKit/WebProcess/WebPage/WebPage.h	(working copy)
>@@ -1586,7 +1586,7 @@ private:
> #endif
> 
> #if PLATFORM(COCOA) || PLATFORM(GTK)
>-    UniqueRef<ViewGestureGeometryCollector> m_viewGestureGeometryCollector;
>+    std::unique_ptr<ViewGestureGeometryCollector> m_viewGestureGeometryCollector;
> #endif
> 
> #if PLATFORM(COCOA)
>Index: Tools/ChangeLog
>===================================================================
>--- Tools/ChangeLog	(revision 241283)
>+++ Tools/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2019-02-11  Alex Christensen  <achristensen@webkit.org>
>+
>+        WebPage::close needs to remove all message receivers associated with that WebPage, not WebPage::~WebPage
>+        https://bugs.webkit.org/show_bug.cgi?id=194522
>+        <rdar://problem/47789393>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
>+        * TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm: Added.
>+        (-[BundleRetainPagePlugIn webProcessPlugIn:didCreateBrowserContextController:]):
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-02-11  Adrian Perez de Castro  <aperez@igalia.com>
> 
>         [GTK][WPE] Add content extensions support in WKTR and unskip layout tests
>Index: Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>===================================================================
>--- Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj	(revision 241275)
>+++ Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj	(working copy)
>@@ -307,6 +307,7 @@
> 		5C4A84951F7EEFFC00ACFC54 /* Configuration.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C4A84941F7EEFD400ACFC54 /* Configuration.mm */; };
> 		5C69BDD51F82A7EF000F4F4B /* JavaScriptDuringNavigation.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C69BDD41F82A7EB000F4F4B /* JavaScriptDuringNavigation.mm */; };
> 		5C7148952123A40A00FDE3C5 /* WKWebsiteDatastore.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C7148942123A40700FDE3C5 /* WKWebsiteDatastore.mm */; };
>+		5C75716122124C5200B9E5AC /* BundleRetainPagePlugIn.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C75715F221249BD00B9E5AC /* BundleRetainPagePlugIn.mm */; };
> 		5C7964101EB0278D0075D74C /* EventModifiers.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 5C79640F1EB0269B0075D74C /* EventModifiers.cpp */; };
> 		5C7C74CB1FB529BA002F9ABE /* WebViewScheduleInRunLoop.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5C7C74CA1FB528D4002F9ABE /* WebViewScheduleInRunLoop.mm */; };
> 		5C838F7F1DB04F900082858F /* LoadInvalidURLRequest.mm in Sources */ = {isa = PBXBuildFile; fileRef = 57901FAE1CAF137100ED64F9 /* LoadInvalidURLRequest.mm */; };
>@@ -1690,6 +1691,7 @@
> 		5C5E633D1D0B67940085A025 /* UniqueRef.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = UniqueRef.cpp; sourceTree = "<group>"; };
> 		5C69BDD41F82A7EB000F4F4B /* JavaScriptDuringNavigation.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = JavaScriptDuringNavigation.mm; sourceTree = "<group>"; };
> 		5C7148942123A40700FDE3C5 /* WKWebsiteDatastore.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WKWebsiteDatastore.mm; sourceTree = "<group>"; };
>+		5C75715F221249BD00B9E5AC /* BundleRetainPagePlugIn.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = BundleRetainPagePlugIn.mm; sourceTree = "<group>"; };
> 		5C79640F1EB0269B0075D74C /* EventModifiers.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = EventModifiers.cpp; sourceTree = "<group>"; };
> 		5C7C74CA1FB528D4002F9ABE /* WebViewScheduleInRunLoop.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WebViewScheduleInRunLoop.mm; sourceTree = "<group>"; };
> 		5C8BC798218CF3E900813886 /* NetworkProcess.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = NetworkProcess.mm; sourceTree = "<group>"; };
>@@ -2467,6 +2469,7 @@
> 				37A709AD1E3EA8B000CA5969 /* BundleRangeHandle.mm */,
> 				37A709AA1E3EA79000CA5969 /* BundleRangeHandlePlugIn.mm */,
> 				37A709AC1E3EA7E800CA5969 /* BundleRangeHandleProtocol.h */,
>+				5C75715F221249BD00B9E5AC /* BundleRetainPagePlugIn.mm */,
> 				1C2B817E1C891E4200A5529F /* CancelFontSubresource.mm */,
> 				1C2B81811C891EFA00A5529F /* CancelFontSubresourcePlugIn.mm */,
> 				5CB18BA71F5645B200EE23C4 /* ClickAutoFillButton.mm */,
>@@ -4389,6 +4392,7 @@
> 				374B7A611DF371CF00ACCB6C /* BundleEditingDelegatePlugIn.mm in Sources */,
> 				A13EBBB01B87436F00097110 /* BundleParametersPlugIn.mm in Sources */,
> 				37A709AF1E3EA97E00CA5969 /* BundleRangeHandlePlugIn.mm in Sources */,
>+				5C75716122124C5200B9E5AC /* BundleRetainPagePlugIn.mm in Sources */,
> 				1C2B81831C891F0900A5529F /* CancelFontSubresourcePlugIn.mm in Sources */,
> 				5CB18BA81F5645E300EE23C4 /* ClickAutoFillButton.mm in Sources */,
> 				A14FC58B1B89927100D107EB /* ContentFilteringPlugIn.mm in Sources */,
>Index: Tools/TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm
>===================================================================
>--- Tools/TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm	(nonexistent)
>+++ Tools/TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm	(working copy)
>@@ -0,0 +1,45 @@
>+/*
>+ * Copyright (C) 2019 Apple Inc. All rights reserved.
>+ *
>+ * 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.
>+ */
>+
>+#import "config.h"
>+
>+#if WK_API_ENABLED
>+
>+#import <WebKit/WKWebProcessPlugIn.h>
>+#import <wtf/RetainPtr.h>
>+
>+@interface BundleRetainPagePlugIn : NSObject <WKWebProcessPlugIn>
>+@end
>+
>+@implementation BundleRetainPagePlugIn
>+
>+- (void)webProcessPlugIn:(WKWebProcessPlugInController *)plugInController didCreateBrowserContextController:(WKWebProcessPlugInBrowserContextController *)browserContextController
>+{
>+    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, 0.5*NSEC_PER_SEC), dispatch_get_main_queue(), [retainedPage = retainPtr(browserContextController)] { });
>+}
>+
>+@end
>+
>+#endif // WK_API_ENABLED
>Index: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>===================================================================
>--- Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm	(revision 241275)
>+++ Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm	(working copy)
>@@ -2404,11 +2404,16 @@ setInterval(() => {
> </body>
> )PSONRESOURCE";
> 
>-TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigation)
>+void testReuseSuspendedProcessForRegularNavigation(bool retainPageInBundle)
> {
>     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
>     [processPoolConfiguration setProcessSwapsOnNavigation:YES];
>+    if (retainPageInBundle)
>+        [processPoolConfiguration setInjectedBundleURL:[[NSBundle mainBundle] URLForResource:@"TestWebKitAPI" withExtension:@"wkbundle"]];
>+
>     auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
>+    if (retainPageInBundle)
>+        [processPool _setObject:@"BundleRetainPagePlugIn" forBundleParameter:TestWebKitAPI::Util::TestPlugInClassNameParameter];
> 
>     auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
>     [webViewConfiguration setProcessPool:processPool.get()];
>@@ -2446,6 +2451,16 @@ TEST(ProcessSwap, ReuseSuspendedProcessF
>     EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
> }
> 
>+TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigationRetainBundlePage)
>+{
>+    testReuseSuspendedProcessForRegularNavigation(true);
>+}
>+
>+TEST(ProcessSwap, ReuseSuspendedProcessForRegularNavigation)
>+{
>+    testReuseSuspendedProcessForRegularNavigation(false);
>+}
>+
> static const char* mainFramesOnlyMainFrame = R"PSONRESOURCE(
> <script>
> function loaded() {

Flags:

cdumez: review+
cdumez: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 194522: 361735 | 361819