Changeset 204245 in webkit

Timestamp:

Aug 7, 2016, 1:50:43 PM (10 years ago)

Author:

mitz@apple.com

Message:

[Cocoa] Reply block leaks if the remote object doesn’t call it
​https://bugs.webkit.org/show_bug.cgi?id=160642

Reviewed by Sam Weinig.

Source/WebKit2:

• Shared/API/Cocoa/RemoteObjectRegistry.h: Declared new member functions.
• Shared/API/Cocoa/RemoteObjectRegistry.messages.in: Added ReleaseUnusedReplyBlock message.
• Shared/API/Cocoa/RemoteObjectRegistry.mm:

(WebKit::RemoteObjectRegistry::sendUnusedReply): Send the ReleaseUnusedReplyBlock message.
(WebKit::RemoteObjectRegistry::releaseUnusedReplyBlock): Message receiver that call through

to -_releaseReplyWithID:.

• Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:

(-[_WKRemoteObjectRegistry _invokeMethod:]): Define a ReplyBlockCallChecker object and

capture an instance of it in the reply block we pass to the exported object. Have that
block set a flag on the checker when it’s called. If the checker gets destroyed without
the block having been called, which means that the block got destroyed without being
called, call sendUnusedReply to let the other side know that the block will not be invoked.

(-[_WKRemoteObjectRegistry _releaseReplyWithID:]): Added. Removed the pending reply from the

map, which release the block.

• Shared/API/Cocoa/_WKRemoteObjectRegistryInternal.h:

Tools:

• TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistry.h: Declared a new method.
• TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistry.mm:

(TEST): Added a test case that checks that the reply block is released even when it’s not

called.

• TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistryPlugIn.mm:

(-[RemoteObjectRegistryPlugIn doNotCallCompletionHandler:]): Implement new method by not

calling the completion handler.

Location:

trunk

Files:

• Source/WebKit2/ChangeLog (modified) (1 diff)
• Source/WebKit2/Shared/API/Cocoa/RemoteObjectRegistry.h (modified) (2 diffs)
• Source/WebKit2/Shared/API/Cocoa/RemoteObjectRegistry.messages.in (modified) (1 diff)
• Source/WebKit2/Shared/API/Cocoa/RemoteObjectRegistry.mm (modified) (2 diffs)
• Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm (modified) (2 diffs)
• Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectRegistryInternal.h (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistry.h (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistry.mm (modified) (2 diffs)
• Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistryPlugIn.mm (modified) (1 diff)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2016-08-07  Dan Bernstein  <mitz@apple.com>
+
+        [Cocoa] Reply block leaks if the remote object doesn’t call it
+        https://bugs.webkit.org/show_bug.cgi?id=160642
+
+        Reviewed by Sam Weinig.
+
+        * Shared/API/Cocoa/RemoteObjectRegistry.h: Declared new member functions.
+        * Shared/API/Cocoa/RemoteObjectRegistry.messages.in: Added ReleaseUnusedReplyBlock message.
+        * Shared/API/Cocoa/RemoteObjectRegistry.mm:
+        (WebKit::RemoteObjectRegistry::sendUnusedReply): Send the ReleaseUnusedReplyBlock message.
+        (WebKit::RemoteObjectRegistry::releaseUnusedReplyBlock): Message receiver that call through
+          to -_releaseReplyWithID:.
+
+        * Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:
+        (-[_WKRemoteObjectRegistry _invokeMethod:]): Define a ReplyBlockCallChecker object and
+          capture an instance of it in the reply block we pass to the exported object. Have that
+          block set a flag on the checker when it’s called. If the checker gets destroyed without
+          the block having been called, which means that the block got destroyed without being
+          called, call sendUnusedReply to let the other side know that the block will not be invoked.
+        (-[_WKRemoteObjectRegistry _releaseReplyWithID:]): Added. Removed the pending reply from the
+          map, which release the block.
+        * Shared/API/Cocoa/_WKRemoteObjectRegistryInternal.h:
+
 2016-08-07  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit2/Shared/API/Cocoa/RemoteObjectRegistry.h

@@ -47 +47 @@
     void sendInvocation(const RemoteObjectInvocation&);
     void sendReplyBlock(uint64_t replyID, const UserData& blockInvocation);
+    void sendUnusedReply(uint64_t replyID);
 
 private:
@@ -55 +56 @@
     void invokeMethod(const RemoteObjectInvocation&);
     void callReplyBlock(uint64_t replyID, const UserData& blockInvocation);
+    void releaseUnusedReplyBlock(uint64_t replyID);
 
     _WKRemoteObjectRegistry *m_remoteObjectRegistry;

trunk/Source/WebKit2/Shared/API/Cocoa/RemoteObjectRegistry.messages.in

@@ -24 +24 @@
     InvokeMethod(WebKit::RemoteObjectInvocation invocation)
     CallReplyBlock(uint64_t replyID, WebKit::UserData blockInvocation);
+    ReleaseUnusedReplyBlock(uint64_t replyID);
 }

trunk/Source/WebKit2/Shared/API/Cocoa/RemoteObjectRegistry.mm

@@ -55 +55 @@
 }
 
+void RemoteObjectRegistry::sendUnusedReply(uint64_t replyID)
+{
+    m_messageSender.send(Messages::RemoteObjectRegistry::ReleaseUnusedReplyBlock(replyID));
+}
+
 void RemoteObjectRegistry::invokeMethod(const RemoteObjectInvocation& invocation)
 {
@@ -69 +74 @@
 }
 
+void RemoteObjectRegistry::releaseUnusedReplyBlock(uint64_t replyID)
+{
+#if WK_API_ENABLED
+    [m_remoteObjectRegistry _releaseReplyWithID:replyID];
+#endif
+}
 } // namespace WebKit

trunk/Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm

@@ -217 +217 @@
             RetainPtr<_WKRemoteObjectRegistry> remoteObjectRegistry = self;
             uint64_t replyID = replyInfo->replyID;
-            id replyBlock = __NSMakeSpecialForwardingCaptureBlock(wireBlockSignature._typeString.UTF8String, [interface, remoteObjectRegistry, replyID](NSInvocation *invocation) {
+
+            class ReplyBlockCallChecker : public WTF::ThreadSafeRefCounted<ReplyBlockCallChecker> {
+            public:
+                static Ref<ReplyBlockCallChecker> create(_WKRemoteObjectRegistry *registry, uint64_t replyID) { return adoptRef(*new ReplyBlockCallChecker(registry, replyID)); }
+
+                ~ReplyBlockCallChecker()
+                {
+                    if (!m_didCallReplyBlock)
+                        m_remoteObjectRegistry->_remoteObjectRegistry->sendUnusedReply(m_replyID);
+                }
+
+                void didCallReplyBlock() { m_didCallReplyBlock = true; }
+
+            private:
+                ReplyBlockCallChecker(_WKRemoteObjectRegistry *registry, uint64_t replyID)
+                    : m_remoteObjectRegistry(registry)
+                    , m_replyID(replyID)
+                {
+                }
+
+                RetainPtr<_WKRemoteObjectRegistry> m_remoteObjectRegistry;
+                uint64_t m_replyID = 0;
+                bool m_didCallReplyBlock = false;
+            };
+
+            RefPtr<ReplyBlockCallChecker> checker = ReplyBlockCallChecker::create(self, replyID);
+            id replyBlock = __NSMakeSpecialForwardingCaptureBlock(wireBlockSignature._typeString.UTF8String, [interface, remoteObjectRegistry, replyID, checker](NSInvocation *invocation) {
                 auto encoder = adoptNS([[WKRemoteObjectEncoder alloc] init]);
                 [encoder encodeObject:invocation forKey:invocationKey];
 
                 remoteObjectRegistry->_remoteObjectRegistry->sendReplyBlock(replyID, UserData([encoder rootObjectDictionary]));
+                checker->didCallReplyBlock();
             });
 
@@ -271 +298 @@
 }
 
+- (void)_releaseReplyWithID:(uint64_t)replyID
+{
+    _pendingReplies.remove(replyID);
+}
+
 @end
 

trunk/Source/WebKit2/Shared/API/Cocoa/_WKRemoteObjectRegistryInternal.h

@@ -49 +49 @@
 
 - (void)_callReplyWithID:(uint64_t)replyID blockInvocation:(const WebKit::UserData&)blockInvocation;
+- (void)_releaseReplyWithID:(uint64_t)replyID;
 
 @end

trunk/Tools/ChangeLog

@@ +1 @@
+2016-08-07  Dan Bernstein  <mitz@apple.com>
+
+        [Cocoa] Reply block leaks if the remote object doesn’t call it
+        https://bugs.webkit.org/show_bug.cgi?id=160642
+
+        Reviewed by Sam Weinig.
+
+        * TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistry.h: Declared a new method.
+        * TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistry.mm:
+        (TEST): Added a test case that checks that the reply block is released even when it’s not
+          called.
+        * TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistryPlugIn.mm:
+        (-[RemoteObjectRegistryPlugIn doNotCallCompletionHandler:]): Implement new method by not
+          calling the completion handler.
+
 2016-08-07  Chris Dumez  <cdumez@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistry.h

@@ -36 +36 @@
 - (void)selectionAndClickInformationForClickAtPoint:(NSValue *)pointValue completionHandler:(void (^)(NSDictionary *))completionHandler;
 - (void)takeRange:(NSRange)range completionHandler:(void (^)(NSUInteger location, NSUInteger length))completionHandler;
+- (void)doNotCallCompletionHandler:(void (^)())completionHandler;
 
 @end

trunk/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistry.mm

@@ -36 +36 @@
 #import <WebKit/_WKRemoteObjectInterface.h>
 #import <WebKit/_WKRemoteObjectRegistry.h>
+#import <wtf/RefCounted.h>
 #import <wtf/RetainPtr.h>
 
@@ -83 +84 @@
         }];
         TestWebKitAPI::Util::run(&isDone);
+
+        isDone = false;
+
+        class DoneWhenDestroyed : public RefCounted<DoneWhenDestroyed> {
+        public:
+            ~DoneWhenDestroyed() { isDone = true; }
+        };
+
+        {
+            RefPtr<DoneWhenDestroyed> doneWhenDestroyed = adoptRef(*new DoneWhenDestroyed);
+            [object doNotCallCompletionHandler:[doneWhenDestroyed]() {
+            }];
+        }
+
+        TestWebKitAPI::Util::run(&isDone);
     }
 }

trunk/Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RemoteObjectRegistryPlugIn.mm

@@ -82 +82 @@
 }
 
+- (void)doNotCallCompletionHandler:(void (^)())completionHandler
+{
+}
+
 @end