Crash when using a removed ScriptMessageHandler
authorweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 24 May 2015 23:22:06 +0000 (23:22 +0000)
committerweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 24 May 2015 23:22:06 +0000 (23:22 +0000)
<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.

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

13 files changed:
Source/WebCore/ChangeLog
Source/WebCore/bindings/gobject/WebKitDOMCustom.cpp
Source/WebCore/page/UserMessageHandler.cpp
Source/WebCore/page/UserMessageHandler.h
Source/WebCore/page/UserMessageHandler.idl
Source/WebCore/page/UserMessageHandlerDescriptor.cpp
Source/WebCore/page/UserMessageHandlerDescriptor.h
Source/WebCore/page/UserMessageHandlersNamespace.cpp
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/UserContent/WebUserContentController.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm
Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp

index 4645e56..cd09afb 100644 (file)
@@ -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-23  Dan Bernstein  <mitz@apple.com>
 
         Remove unused definitions of WEBKIT_VERSION_MIN_REQUIRED
index 7cfc2df..a75e1c1 100644 (file)
@@ -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;
 }
 
index a5ce400..05c5a76 100644 (file)
@@ -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()
index b2cdfff..5a1ea77 100644 (file)
@@ -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 @@ public:
     }
     virtual ~UserMessageHandler();
 
-    void postMessage(PassRefPtr<SerializedScriptValue>);
+    void postMessage(PassRefPtr<SerializedScriptValue>, ExceptionCode&);
 
     const AtomicString& name();
     DOMWrapperWorld& world();
index 6923767..b1a0387 100644 (file)
@@ -26,5 +26,5 @@
 [
     Conditional=USER_MESSAGE_HANDLERS
 ] interface UserMessageHandler {
-    void postMessage(SerializedScriptValue message);
+    [RaisesException] void postMessage(SerializedScriptValue message);
 };
index 90a9eb4..72f02be 100644 (file)
@@ -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)
 {
 }
 
index 44db3af..1f1f20b 100644 (file)
@@ -56,15 +56,16 @@ public:
 
     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
index c9ae926..b4a5081 100644 (file)
@@ -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();
index 2e519ad..e87058f 100644 (file)
@@ -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-23  Dan Bernstein  <mitz@apple.com>
 
         <rdar://problem/21090327> /S/L/PrivateFrameworks/WebKit.framework is missing Headers and PrivateHeaders symlinks
index aa7b645..fbf9d0d 100644 (file)
@@ -117,6 +117,7 @@ public:
 
     virtual ~WebUserMessageHandlerDescriptorProxy()
     {
+        m_descriptor->invalidateClient();
     }
 
     // WebCore::UserMessageHandlerDescriptor::Client
index e5e1462..c9c1da7 100644 (file)
@@ -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-23  Dan Bernstein  <mitz@apple.com>
 
         Remove unused definitions of WEBKIT_VERSION_MIN_REQUIRED
index 1cd0cc2..5330035 100644 (file)
@@ -62,7 +62,7 @@ static RetainPtr<WKScriptMessage> lastScriptMessage;
 
 @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 @@ TEST(WKUserContentController, ScriptMessageHandlerSimple)
     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)
 {
index 5667fa8..765a42f 100644 (file)
@@ -104,7 +104,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");