Merge r184846 - Crash when using a removed ScriptMessageHandler

<rdar://problem/20888499> https://bugs.webkit.org/show_bug.cgi?id=145359 Reviewed by Dan Bernstein. Source/WebCore: Added tests: WKUserContentController.ScriptMessageHandlerBasicRemove WKUserContentController.ScriptMessageHandlerCallRemovedHandler * page/UserMessageHandler.cpp: (WebCore::UserMessageHandler::~UserMessageHandler): (WebCore::UserMessageHandler::postMessage): (WebCore::UserMessageHandler::name): * page/UserMessageHandler.h: (WebCore::UserMessageHandler::create): * page/UserMessageHandler.idl: * page/UserMessageHandlerDescriptor.cpp: (WebCore::UserMessageHandlerDescriptor::UserMessageHandlerDescriptor): * page/UserMessageHandlerDescriptor.h: (WebCore::UserMessageHandlerDescriptor::client): (WebCore::UserMessageHandlerDescriptor::invalidateClient): Add support for invalidating the descriptor and throw an exception if someone tries to post a message using an invalidated descriptor. * page/UserMessageHandlersNamespace.cpp: (WebCore::UserMessageHandlersNamespace::handler): Add logic to remove message handlers if their descriptor has been invalidated. Source/WebKit2: * WebProcess/UserContent/WebUserContentController.cpp: (WebKit::WebUserMessageHandlerDescriptorProxy::~WebUserMessageHandlerDescriptorProxy): Invalidate the descriptor when the message handler client (as implemented by WebUserMessageHandlerDescriptorProxy) goes away. This will happen if a script message handler is removed at the API level or the WebUserContentController is destroyed (which will happen if all the pages get destroyed). Tools: * TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm: Add tests for removing script message handlers.
WebKit · Jul 6, 2015 · 6339dc7 · 6339dc7
1 parent f87412b
commit 6339dc7
Show file tree

Hide file tree

Showing 13 changed files with 179 additions and 20 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,34 @@
+2015-05-24  Sam Weinig  <sam@webkit.org>
+
+        Crash when using a removed ScriptMessageHandler
+        <rdar://problem/20888499>
+        https://bugs.webkit.org/show_bug.cgi?id=145359
+
+        Reviewed by Dan Bernstein.
+
+        Added tests:
+            WKUserContentController.ScriptMessageHandlerBasicRemove
+            WKUserContentController.ScriptMessageHandlerCallRemovedHandler
+
+        * page/UserMessageHandler.cpp:
+        (WebCore::UserMessageHandler::~UserMessageHandler):
+        (WebCore::UserMessageHandler::postMessage):
+        (WebCore::UserMessageHandler::name):
+        * page/UserMessageHandler.h:
+        (WebCore::UserMessageHandler::create):
+        * page/UserMessageHandler.idl:
+        * page/UserMessageHandlerDescriptor.cpp:
+        (WebCore::UserMessageHandlerDescriptor::UserMessageHandlerDescriptor):
+        * page/UserMessageHandlerDescriptor.h:
+        (WebCore::UserMessageHandlerDescriptor::client):
+        (WebCore::UserMessageHandlerDescriptor::invalidateClient):
+        Add support for invalidating the descriptor and throw an exception if someone tries
+        to post a message using an invalidated descriptor.
+
+        * page/UserMessageHandlersNamespace.cpp:
+        (WebCore::UserMessageHandlersNamespace::handler):
+        Add logic to remove message handlers if their descriptor has been invalidated.
+
 2015-05-22  Mark Lam  <mark.lam@apple.com>
 
         Document::ensurePlugInsInjectedScript() should evaluate the injected script on its own frame.

diff --git a/Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp b/Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp
@@ -86,7 +86,11 @@ gboolean webkit_dom_dom_window_webkit_message_handlers_post_message(WebKitDOMDOM
         return FALSE;
 
     WebCore::JSMainThreadNullState state;
-    handler->postMessage(WebCore::SerializedScriptValue::create(String::fromUTF8(message)));
+    WebCore::ExceptionCode ec = 0;
+    handler->postMessage(WebCore::SerializedScriptValue::create(String::fromUTF8(message)), ec);
+    if (ec)
+        return FALSE;
+
     return TRUE;
 }
 

diff --git a/Source/WebCore/page/UserMessageHandler.cpp b/Source/WebCore/page/UserMessageHandler.cpp
@@ -28,6 +28,7 @@
 
 #if ENABLE(USER_MESSAGE_HANDLERS)
 
+#include "ExceptionCode.h"
 #include "Frame.h"
 #include "SerializedScriptValue.h"
 
@@ -43,9 +44,16 @@ UserMessageHandler::~UserMessageHandler()
 {
 }
 
-void UserMessageHandler::postMessage(PassRefPtr<SerializedScriptValue> value)
+void UserMessageHandler::postMessage(PassRefPtr<SerializedScriptValue> value, ExceptionCode& ec)
 {
-    m_descriptor->client().didPostMessage(*this, value.get());
+    // Check to see if the descriptor has been removed. This can happen if the host application has
+    // removed the named message handler at the WebKit2 API level.
+    if (!m_descriptor->client()) {
+        ec = INVALID_ACCESS_ERR;
+        return;
+    }
+
+    m_descriptor->client()->didPostMessage(*this, value.get());
 }
 
 const AtomicString& UserMessageHandler::name()

diff --git a/Source/WebCore/page/UserMessageHandler.h b/Source/WebCore/page/UserMessageHandler.h
@@ -34,6 +34,8 @@
 
 namespace WebCore {
 
+typedef int ExceptionCode;
+
 class UserMessageHandler : public RefCounted<UserMessageHandler>, public FrameDestructionObserver {
 public:
     static Ref<UserMessageHandler> create(Frame& frame, UserMessageHandlerDescriptor& descriptor)
@@ -42,7 +44,7 @@ class UserMessageHandler : public RefCounted<UserMessageHandler>, public FrameDe
     }
     virtual ~UserMessageHandler();
 
-    void postMessage(PassRefPtr<SerializedScriptValue>);
+    void postMessage(PassRefPtr<SerializedScriptValue>, ExceptionCode&);
 
     const AtomicString& name();
     DOMWrapperWorld& world();

diff --git a/Source/WebCore/page/UserMessageHandler.idl b/Source/WebCore/page/UserMessageHandler.idl
@@ -26,5 +26,5 @@
 [
     Conditional=USER_MESSAGE_HANDLERS
 ] interface UserMessageHandler {
-    void postMessage(SerializedScriptValue message);
+    [RaisesException] void postMessage(SerializedScriptValue message);
 };
diff --git a/Source/WebCore/page/UserMessageHandlerDescriptor.cpp b/Source/WebCore/page/UserMessageHandlerDescriptor.cpp
@@ -35,7 +35,7 @@ namespace WebCore {
 UserMessageHandlerDescriptor::UserMessageHandlerDescriptor(const AtomicString& name, DOMWrapperWorld& world, Client& client)
     : m_name(name)
     , m_world(world)
-    , m_client(client)
+    , m_client(&client)
 {
 }
 

diff --git a/Source/WebCore/page/UserMessageHandlerDescriptor.h b/Source/WebCore/page/UserMessageHandlerDescriptor.h
@@ -56,15 +56,16 @@ class UserMessageHandlerDescriptor : public RefCounted<UserMessageHandlerDescrip
 
     const AtomicString& name();
     DOMWrapperWorld& world();
-
-    Client& client() const { return m_client; }
+
+    Client* client() const { return m_client; }
+    void invalidateClient() { m_client = nullptr; }
 
 private:
     WEBCORE_EXPORT explicit UserMessageHandlerDescriptor(const AtomicString&, DOMWrapperWorld&, Client&);
-    
+
     AtomicString m_name;
     Ref<DOMWrapperWorld> m_world;
-    Client& m_client;
+    Client* m_client;
 };
 
 } // namespace WebCore

diff --git a/Source/WebCore/page/UserMessageHandlersNamespace.cpp b/Source/WebCore/page/UserMessageHandlersNamespace.cpp
@@ -46,13 +46,6 @@ UserMessageHandlersNamespace::~UserMessageHandlersNamespace()
 
 UserMessageHandler* UserMessageHandlersNamespace::handler(const AtomicString& name, DOMWrapperWorld& world)
 {
-    // First, check if we have a handler instance already.
-    for (auto& handler : m_messageHandlers) {
-        if (handler->name() == name && &handler->world() == &world)
-            return &handler.get();
-    }
-
-    // Second, attempt to create a handler instance from a descriptor.
     if (!frame())
         return nullptr;
 
@@ -69,8 +62,17 @@ UserMessageHandler* UserMessageHandlersNamespace::handler(const AtomicString& na
         return nullptr;
 
     RefPtr<UserMessageHandlerDescriptor> descriptor = userMessageHandlerDescriptors->get(std::make_pair(name, &world));
-    if (!descriptor)
+    if (!descriptor) {
+        m_messageHandlers.removeFirstMatching([&name, &world](Ref<UserMessageHandler>& handler) {
+            return handler->name() == name && &handler->world() == &world;
+        });
         return nullptr;
+    }
+
+    for (auto& handler : m_messageHandlers) {
+        if (handler->name() == name && &handler->world() == &world)
+            return &handler.get();
+    }
 
     m_messageHandlers.append(UserMessageHandler::create(*frame(), *descriptor));
     return &m_messageHandlers.last().get();

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
@@ -1,3 +1,17 @@
+2015-05-24  Sam Weinig  <sam@webkit.org>
+
+        Crash when using a removed ScriptMessageHandler
+        <rdar://problem/20888499>
+        https://bugs.webkit.org/show_bug.cgi?id=145359
+
+        Reviewed by Dan Bernstein.
+
+        * WebProcess/UserContent/WebUserContentController.cpp:
+        (WebKit::WebUserMessageHandlerDescriptorProxy::~WebUserMessageHandlerDescriptorProxy):
+        Invalidate the descriptor when the message handler client (as implemented by WebUserMessageHandlerDescriptorProxy)
+        goes away. This will happen if a script message handler is removed at the API level or the WebUserContentController
+        is destroyed (which will happen if all the pages get destroyed).
+
 2015-05-20  Gavin Barraclough  <barraclough@apple.com>
 
         dispatchViewStateChange should not wait for sync reply if the page isn't visible

diff --git a/Source/WebKit2/WebProcess/UserContent/WebUserContentController.cpp b/Source/WebKit2/WebProcess/UserContent/WebUserContentController.cpp
@@ -116,6 +116,7 @@ class WebUserMessageHandlerDescriptorProxy : public RefCounted<WebUserMessageHan
 
     virtual ~WebUserMessageHandlerDescriptorProxy()
     {
+        m_descriptor->invalidateClient();
     }
 
     // WebCore::UserMessageHandlerDescriptor::Client

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
@@ -1,3 +1,14 @@
+2015-05-24  Sam Weinig  <sam@webkit.org>
+
+        Crash when using a removed ScriptMessageHandler
+        <rdar://problem/20888499>
+        https://bugs.webkit.org/show_bug.cgi?id=145359
+
+        Reviewed by Dan Bernstein.
+
+        * TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:
+        Add tests for removing script message handlers.
+
 2015-05-07  Ada Chan  <adachan@apple.com>
 
         Add a test for WKPageCopySessionState() with filtering.

diff --git a/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm b/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm
@@ -62,7 +62,7 @@ - (void)userContentController:(WKUserContentController *)userContentController d
 
 @end
 
-TEST(WKUserContentController, ScriptMessageHandlerSimple)
+TEST(WKUserContentController, ScriptMessageHandlerBasicPost)
 {
     RetainPtr<ScriptMessageHandler> handler = adoptNS([[ScriptMessageHandler alloc] init]);
     RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
@@ -87,6 +87,91 @@ - (void)userContentController:(WKUserContentController *)userContentController d
     EXPECT_WK_STREQ(@"Hello", (NSString *)[lastScriptMessage body]);
 }
 
+TEST(WKUserContentController, ScriptMessageHandlerBasicRemove)
+{
+    RetainPtr<ScriptMessageHandler> handler = adoptNS([[ScriptMessageHandler alloc] init]);
+    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    RetainPtr<WKUserContentController> userContentController = [configuration userContentController];
+    [userContentController addScriptMessageHandler:handler.get() name:@"handlerToRemove"];
+    [userContentController addScriptMessageHandler:handler.get() name:@"handlerToPost"];
+
+    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.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);
+
+    // Test that handlerToRemove was succesfully added.
+    [webView evaluateJavaScript:
+        @"if (window.webkit.messageHandlers.handlerToRemove) {"
+         "    window.webkit.messageHandlers.handlerToPost.postMessage('PASS');"
+         "} else {"
+         "    window.webkit.messageHandlers.handlerToPost.postMessage('FAIL');"
+         "}" completionHandler:nil];
+
+    TestWebKitAPI::Util::run(&receivedScriptMessage);
+    receivedScriptMessage = false;
+
+    EXPECT_WK_STREQ(@"PASS", (NSString *)[lastScriptMessage body]);
+
+    [userContentController removeScriptMessageHandlerForName:@"handlerToRemove"];
+
+    // Test that handlerToRemove has been removed.
+    [webView evaluateJavaScript:
+        @"if (window.webkit.messageHandlers.handlerToRemove) {"
+         "    window.webkit.messageHandlers.handlerToPost.postMessage('FAIL');"
+         "} else {"
+         "    window.webkit.messageHandlers.handlerToPost.postMessage('PASS');"
+         "}" completionHandler:nil];
+
+    TestWebKitAPI::Util::run(&receivedScriptMessage);
+    receivedScriptMessage = false;
+
+    EXPECT_WK_STREQ(@"PASS", (NSString *)[lastScriptMessage body]);
+}
+
+TEST(WKUserContentController, ScriptMessageHandlerCallRemovedHandler)
+{
+    RetainPtr<ScriptMessageHandler> handler = adoptNS([[ScriptMessageHandler alloc] init]);
+    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    RetainPtr<WKUserContentController> userContentController = [configuration userContentController];
+    [userContentController addScriptMessageHandler:handler.get() name:@"handlerToRemove"];
+    [userContentController addScriptMessageHandler:handler.get() name:@"handlerToPost"];
+
+    RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.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);
+
+    [webView evaluateJavaScript:@"var handlerToRemove = window.webkit.messageHandlers.handlerToRemove;" completionHandler:nil];
+
+    [userContentController removeScriptMessageHandlerForName:@"handlerToRemove"];
+
+    // Test that we throw an exception if you try to use a message handler that has been removed.
+    [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)
 {

diff --git a/Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp b/Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp
@@ -106,7 +106,7 @@ static void documentLoadedCallback(WebKitWebPage* webPage, WebKitWebExtension* e
     if (WebKitDOMWebKitNamespace* webkit = webkit_dom_dom_window_get_webkit_namespace(window.get())) {
         WebKitDOMUserMessageHandlersNamespace* messageHandlers = webkit_dom_webkit_namespace_get_message_handlers(webkit);
         if (WebKitDOMUserMessageHandler* handler = webkit_dom_user_message_handlers_namespace_get_handler(messageHandlers, "dom"))
-            webkit_dom_user_message_handler_post_message(handler, "DocumentLoaded");
+            webkit_dom_user_message_handler_post_message(handler, "DocumentLoaded", nullptr);
     }
 
     webkit_dom_dom_window_webkit_message_handlers_post_message(window.get(), "dom-convenience", "DocumentLoaded");