Removing and re-adding a script message handler with the same name results in an...
authortimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Mar 2016 19:56:41 +0000 (19:56 +0000)
committertimothy_horton@apple.com <timothy_horton@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Mar 2016 19:56:41 +0000 (19:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=155223

Reviewed by Sam Weinig.
Source/WebCore:

New API test: WKUserContentController.ScriptMessageHandlerReplaceWithSameName.

* page/UserMessageHandler.h:
(WebCore::UserMessageHandler::descriptor):
* page/UserMessageHandlersNamespace.cpp:
(WebCore::UserMessageHandlersNamespace::handler):
This lazy removal mechanism combined with the fact that we only compare
handler name and world makes it such that m_messageHandlers could have
a stale UserMessageHandler with a UserMessageHandlerDescriptor that differed
only in client.

It is safe to compare the descriptors by pointer instead because m_messageHandler
holds a strong reference to its UserMessageHandlerDescriptors, and this will ensure
that the add-remove-add path (with identical name and world) causes a new
UserContentController to be created.

We also now clean up any stale UserMessageHandlers whenever we're about to
add a new one, by removing any which the UserContentController no longer knows about.

Tools:

* TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:
(TEST):
Add a test ensuring that it is possible to remove and re-add a script message handler
with the same name and still dispatch messages to it.

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

Source/WebCore/ChangeLog
Source/WebCore/page/UserMessageHandler.h
Source/WebCore/page/UserMessageHandlersNamespace.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm

index 0ae98c1..1e1f52e 100644 (file)
@@ -1,3 +1,30 @@
+2016-03-09  Tim Horton  <timothy_horton@apple.com>
+
+        Removing and re-adding a script message handler with the same name results in an unusable message handler
+        https://bugs.webkit.org/show_bug.cgi?id=155223
+
+        Reviewed by Sam Weinig.
+        Patch by Geoff Garen and myself.
+
+        New API test: WKUserContentController.ScriptMessageHandlerReplaceWithSameName.
+
+        * page/UserMessageHandler.h:
+        (WebCore::UserMessageHandler::descriptor):
+        * page/UserMessageHandlersNamespace.cpp:
+        (WebCore::UserMessageHandlersNamespace::handler):
+        This lazy removal mechanism combined with the fact that we only compare
+        handler name and world makes it such that m_messageHandlers could have
+        a stale UserMessageHandler with a UserMessageHandlerDescriptor that differed
+        only in client.
+
+        It is safe to compare the descriptors by pointer instead because m_messageHandler
+        holds a strong reference to its UserMessageHandlerDescriptors, and this will ensure
+        that the add-remove-add path (with identical name and world) causes a new
+        UserContentController to be created.
+
+        We also now clean up any stale UserMessageHandlers whenever we're about to
+        add a new one, by removing any which the UserContentController no longer knows about.
+
 2016-03-09  Chris Dumez  <cdumez@apple.com>
 
         Align HTMLKeygenElement.keytype with the specification
index 5a1ea77..e9a8814 100644 (file)
@@ -48,6 +48,7 @@ public:
 
     const AtomicString& name();
     DOMWrapperWorld& world();
+    const UserMessageHandlerDescriptor& descriptor() const { return m_descriptor.get(); }
 
 private:
     UserMessageHandler(Frame&, UserMessageHandlerDescriptor&);
index b4a5081..03321f5 100644 (file)
@@ -62,18 +62,23 @@ UserMessageHandler* UserMessageHandlersNamespace::handler(const AtomicString& na
         return nullptr;
 
     RefPtr<UserMessageHandlerDescriptor> descriptor = userMessageHandlerDescriptors->get(std::make_pair(name, &world));
-    if (!descriptor) {
-        m_messageHandlers.removeFirstMatching([&name, &world](Ref<UserMessageHandler>& handler) {
-            return handler->name() == name && &handler->world() == &world;
-        });
+    if (!descriptor)
         return nullptr;
-    }
 
     for (auto& handler : m_messageHandlers) {
-        if (handler->name() == name && &handler->world() == &world)
+        if (&handler->descriptor() == descriptor.get())
             return &handler.get();
     }
 
+    auto liveHandlers = userMessageHandlerDescriptors->values();
+    m_messageHandlers.removeAllMatching([liveHandlers](const Ref<UserMessageHandler>& handler) {
+        for (const auto& liveHandler : liveHandlers) {
+            if (liveHandler.get() == &handler->descriptor())
+                return true;
+        }
+        return false;
+    });
+
     m_messageHandlers.append(UserMessageHandler::create(*frame(), *descriptor));
     return &m_messageHandlers.last().get();
 }
index ed482b4..619d710 100644 (file)
@@ -1,3 +1,15 @@
+2016-03-09  Tim Horton  <timothy_horton@apple.com>
+
+        Removing and re-adding a script message handler with the same name results in an unusable message handler
+        https://bugs.webkit.org/show_bug.cgi?id=155223
+
+        Reviewed by Sam Weinig.
+
+        * TestWebKitAPI/Tests/WebKit2Cocoa/UserContentController.mm:
+        (TEST):
+        Add a test ensuring that it is possible to remove and re-add a script message handler
+        with the same name and still dispatch messages to it.
+
 2016-03-08  Alexey Proskuryakov  <ap@apple.com>
 
         Fix iOS Simulator EWS.
index e213321..095f6e7 100644 (file)
@@ -264,6 +264,44 @@ TEST(WKUserContentController, ScriptMessageHandlerWithNavigation)
 }
 #endif
 
+TEST(WKUserContentController, ScriptMessageHandlerReplaceWithSameName)
+{
+    RetainPtr<ScriptMessageHandler> handler = adoptNS([[ScriptMessageHandler alloc] init]);
+    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+    RetainPtr<WKUserContentController> userContentController = [configuration userContentController];
+    [userContentController addScriptMessageHandler:handler.get() name:@"handlerToReplace"];
+
+    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 handlerToReplace was succesfully added.
+    [webView evaluateJavaScript:@"window.webkit.messageHandlers.handlerToReplace.postMessage('PASS1');" completionHandler:nil];
+
+    TestWebKitAPI::Util::run(&receivedScriptMessage);
+    receivedScriptMessage = false;
+
+    EXPECT_WK_STREQ(@"PASS1", (NSString *)[lastScriptMessage body]);
+
+    [userContentController removeScriptMessageHandlerForName:@"handlerToReplace"];
+    [userContentController addScriptMessageHandler:handler.get() name:@"handlerToReplace"];
+
+    // Test that handlerToReplace still works.
+    [webView evaluateJavaScript:@"window.webkit.messageHandlers.handlerToReplace.postMessage('PASS2');" completionHandler:nil];
+
+    TestWebKitAPI::Util::run(&receivedScriptMessage);
+    receivedScriptMessage = false;
+
+    EXPECT_WK_STREQ(@"PASS2", (NSString *)[lastScriptMessage body]);
+}
+
 static NSString *styleSheetSource = @"body { background-color: green !important; }";
 static NSString *backgroundColorScript = @"window.getComputedStyle(document.body, null).getPropertyValue('background-color')";
 static NSString *frameBackgroundColorScript = @"window.getComputedStyle(document.getElementsByTagName('iframe')[0].contentDocument.body, null).getPropertyValue('background-color')";