Reproducible "Unhanded web process message 'WebUserContentController:AddUserScripts...
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Jan 2016 22:25:56 +0000 (22:25 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Jan 2016 22:25:56 +0000 (22:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=153193
<rdar://problem/24222034>

Reviewed by Darin Adler.

The WebPageProxy constructor assumes that if its WebProcess is already running,
it can add itself to the existing WebUserContentController(Proxy) and all will be well.

However, if the API client constructs a different WKUserContentController for two views,
and forces them both into the same process, WebPageProxy's constructor sends a message
with a WebUserContentController ID that doesn't exist yet on the WebProcess side
(because createWebPage, which usually brings it up, hasn't happened yet), and we
drop the message on the floor (and get the aforementioned logging).

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::WebPageProxy):
(WebKit::WebPageProxy::initializeWebPageAfterProcessLaunch):
Instead of connecting our WebUserContentControllerProxy and WebVisitedLinkStoreProxy
to our WebProcessProxy in the constructor, when doing so might send messages
to the WebProcess that arrive before their corresponding WebProcess objects have
been created, do this in initializeWebPageAfterProcessLaunch, which always runs
during-or-after inititalizeWebPage (and always after the CreateWebPage message goes out).

(WebKit::WebPageProxy::initializeWebPage):
Mark us as needing initializeWebPageAfterProcessLaunch run, and run it right now
if the WebProcess is already up.
(WebKit::WebPageProxy::processDidFinishLaunching):
If the WebProcess wasn't up at the end of initializeWebPage, we'll eventually end up here
after it is launched, and will initializeWebPageAfterProcessLaunch.

(WebKit::WebPageProxy::resetStateAfterProcessExited):
If the WebProcess dies, we don't want to initializeWebPageAfterProcessLaunch
until after initializeWebPage runs again, so make sure the flag isn't set.

* UIProcess/WebPageProxy.h:

* TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:
(webViewForScriptMessageHandlerMultipleHandlerRemovalTest):
(TEST):
Add a test that exhibits the problems we're fixing here.
Before, it would both log and assert in debug, and crash in release.
Now it runs happily to completion.

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

Source/WebKit2/ChangeLog
Source/WebKit2/UIProcess/WebPageProxy.cpp
Source/WebKit2/UIProcess/WebPageProxy.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm

index d1b13bb..654d2bd 100644 (file)
@@ -1,3 +1,42 @@
+2016-01-22  Tim Horton  <timothy_horton@apple.com>
+
+        Reproducible "Unhanded web process message 'WebUserContentController:AddUserScripts'" and friends
+        https://bugs.webkit.org/show_bug.cgi?id=153193
+        <rdar://problem/24222034>
+
+        Reviewed by Darin Adler.
+
+        The WebPageProxy constructor assumes that if its WebProcess is already running,
+        it can add itself to the existing WebUserContentController(Proxy) and all will be well.
+
+        However, if the API client constructs a different WKUserContentController for two views,
+        and forces them both into the same process, WebPageProxy's constructor sends a message
+        with a WebUserContentController ID that doesn't exist yet on the WebProcess side
+        (because createWebPage, which usually brings it up, hasn't happened yet), and we 
+        drop the message on the floor (and get the aforementioned logging).
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::WebPageProxy):
+        (WebKit::WebPageProxy::initializeWebPageAfterProcessLaunch):
+        Instead of connecting our WebUserContentControllerProxy and WebVisitedLinkStoreProxy
+        to our WebProcessProxy in the constructor, when doing so might send messages
+        to the WebProcess that arrive before their corresponding WebProcess objects have
+        been created, do this in initializeWebPageAfterProcessLaunch, which always runs
+        during-or-after inititalizeWebPage (and always after the CreateWebPage message goes out).
+
+        (WebKit::WebPageProxy::initializeWebPage):
+        Mark us as needing initializeWebPageAfterProcessLaunch run, and run it right now
+        if the WebProcess is already up.
+        (WebKit::WebPageProxy::processDidFinishLaunching):
+        If the WebProcess wasn't up at the end of initializeWebPage, we'll eventually end up here
+        after it is launched, and will initializeWebPageAfterProcessLaunch.
+
+        (WebKit::WebPageProxy::resetStateAfterProcessExited):
+        If the WebProcess dies, we don't want to initializeWebPageAfterProcessLaunch
+        until after initializeWebPage runs again, so make sure the flag isn't set.
+
+        * UIProcess/WebPageProxy.h:
+
 2016-01-22  Darin Adler  <darin@apple.com>
 
         Reduce use of equalIgnoringCase to just ignore ASCII case
index 5e04390..bcf06e7 100644 (file)
@@ -443,12 +443,6 @@ WebPageProxy::WebPageProxy(PageClient& pageClient, WebProcessProxy& process, uin
     m_webProcessLifetimeTracker.addObserver(m_visitedLinkStore);
     m_webProcessLifetimeTracker.addObserver(m_websiteDataStore);
 
-    if (m_process->state() == WebProcessProxy::State::Running) {
-        if (m_userContentController)
-            m_process->addWebUserContentControllerProxy(*m_userContentController);
-        m_process->addVisitedLinkStore(m_visitedLinkStore);
-    }
-
     updateViewState();
     updateActivityToken();
     updateProccessSuppressionState();
@@ -798,6 +792,23 @@ void WebPageProxy::initializeWebPage()
 #if PLATFORM(COCOA)
     send(Messages::WebPage::SetSmartInsertDeleteEnabled(m_isSmartInsertDeleteEnabled));
 #endif
+
+    m_needsToFinishInitializingWebPageAfterProcessLaunch = true;
+    finishInitializingWebPageAfterProcessLaunch();
+}
+
+void WebPageProxy::finishInitializingWebPageAfterProcessLaunch()
+{
+    if (!m_needsToFinishInitializingWebPageAfterProcessLaunch)
+        return;
+    if (m_process->state() != WebProcessProxy::State::Running)
+        return;
+
+    m_needsToFinishInitializingWebPageAfterProcessLaunch = false;
+
+    if (m_userContentController)
+        m_process->addWebUserContentControllerProxy(*m_userContentController);
+    m_process->addVisitedLinkStore(m_visitedLinkStore);
 }
 
 void WebPageProxy::close()
@@ -3615,10 +3626,7 @@ void WebPageProxy::webProcessWillShutDown()
 void WebPageProxy::processDidFinishLaunching()
 {
     ASSERT(m_process->state() == WebProcessProxy::State::Running);
-
-    if (m_userContentController)
-        m_process->addWebUserContentControllerProxy(*m_userContentController);
-    m_process->addVisitedLinkStore(m_visitedLinkStore);
+    finishInitializingWebPageAfterProcessLaunch();
 }
 
 #if ENABLE(NETSCAPE_PLUGIN_API)
@@ -5064,6 +5072,8 @@ void WebPageProxy::resetStateAfterProcessExited()
     m_isValid = false;
     m_isPageSuspended = false;
 
+    m_needsToFinishInitializingWebPageAfterProcessLaunch = false;
+
     m_editorState = EditorState();
 
     m_pageClient.processDidExit();
index e7317e1..d4e579a 100644 (file)
@@ -1463,6 +1463,8 @@ private:
 
     void handleAutoFillButtonClick(const UserData&);
 
+    void finishInitializingWebPageAfterProcessLaunch();
+
     void handleMessage(IPC::Connection&, const String& messageName, const UserData& messageBody);
     void handleSynchronousMessage(IPC::Connection&, const String& messageName, const UserData& messageBody, UserData& returnUserData);
 
@@ -1613,6 +1615,8 @@ private:
     // Whether it can run modal child web pages.
     bool m_canRunModal;
 
+    bool m_needsToFinishInitializingWebPageAfterProcessLaunch { false };
+
     bool m_isInPrintingMode;
     bool m_isPerformingDOMPrintOperation;
 
index c042082..d4b3407 100644 (file)
@@ -1,3 +1,18 @@
+2016-01-22  Tim Horton  <timothy_horton@apple.com>
+
+        Reproducible "Unhanded web process message 'WebUserContentController:AddUserScripts'" and friends
+        https://bugs.webkit.org/show_bug.cgi?id=153193
+        <rdar://problem/24222034>
+
+        Reviewed by Darin Adler.
+
+        * TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:
+        (webViewForScriptMessageHandlerMultipleHandlerRemovalTest):
+        (TEST):
+        Add a test that exhibits the problems we're fixing here.
+        Before, it would both log and assert in debug, and crash in release.
+        Now it runs happily to completion.
+
 2016-01-22  Enrica Casucci  <enrica@apple.com>
 
         Add support for testing data detection.
index 7c83c59..f874864 100644 (file)
@@ -28,7 +28,9 @@
 #import "PlatformUtilities.h"
 #import "Test.h"
 #import <WebKit/WebKit.h>
+#import <WebKit/WKProcessPoolPrivate.h>
 #import <WebKit/WKUserContentControllerPrivate.h>
+#import <WebKit/_WKProcessPoolConfiguration.h>
 #import <WebKit/_WKUserStyleSheet.h>
 #import <wtf/RetainPtr.h>
 
@@ -174,6 +176,50 @@ TEST(WKUserContentController, ScriptMessageHandlerCallRemovedHandler)
     EXPECT_WK_STREQ(@"PASS", (NSString *)[lastScriptMessage body]);
 }
 
+static RetainPtr<WKWebView> webViewForScriptMessageHandlerMultipleHandlerRemovalTest(WKWebViewConfiguration *configuration)
+{
+    RetainPtr<ScriptMessageHandler> handler = adoptNS([[ScriptMessageHandler alloc] init]);
+    RetainPtr<WKWebViewConfiguration> configurationCopy = adoptNS([configuration copy]);
+    [configurationCopy setUserContentController:[[[WKUserContentController alloc] init] autorelease]];
+    [[configurationCopy userContentController] addScriptMessageHandler:handler.get() name:@"handlerToRemove"];
+    [[configurationCopy userContentController] addScriptMessageHandler:handler.get() name:@"handlerToPost"];
+    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configurationCopy.get()]);
+    RetainPtr<SimpleNavigationDelegate> delegate = adoptNS([[SimpleNavigationDelegate alloc] init]);
+    [webView setNavigationDelegate:delegate.get()];
+    NSURLRequest *request = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
+    [webView loadRequest:request];
+    TestWebKitAPI::Util::run(&isDoneWithNavigation);
+    isDoneWithNavigation = false;
+
+    return webView;
+}
+
+TEST(WKUserContentController, ScriptMessageHandlerMultipleHandlerRemoval)
+{
+    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    RetainPtr<_WKProcessPoolConfiguration> processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+    [processPoolConfiguration setMaximumProcessCount:1];
+    [configuration setProcessPool:[[[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()] autorelease]];
+
+    RetainPtr<WKWebView> webView = webViewForScriptMessageHandlerMultipleHandlerRemovalTest(configuration.get());
+    RetainPtr<WKWebView> webView2 = webViewForScriptMessageHandlerMultipleHandlerRemovalTest(configuration.get());
+
+    [[[webView configuration] userContentController] removeScriptMessageHandlerForName:@"handlerToRemove"];
+    [[[webView2 configuration] userContentController] removeScriptMessageHandlerForName:@"handlerToRemove"];
+
+    [webView evaluateJavaScript:
+     @"try {"
+     "    handlerToRemove.postMessage('FAIL');"
+     "} catch (e) {"
+     "    window.webkit.messageHandlers.handlerToPost.postMessage('PASS');"
+     "}" completionHandler:nil];
+
+    TestWebKitAPI::Util::run(&receivedScriptMessage);
+    receivedScriptMessage = false;
+
+    EXPECT_WK_STREQ(@"PASS", (NSString *)[lastScriptMessage body]);
+}
+
 #if !PLATFORM(IOS) // FIXME: hangs in the iOS simulator
 TEST(WKUserContentController, ScriptMessageHandlerWithNavigation)
 {