If a reply block is provided but is nil, we shouldn't call `invokeMethod` - invocations expect callbacks to be callable. <rdar://problem/75781150>
Created attachment 428999 [details] Patch
Comment on attachment 428999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428999&action=review > Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:269 > + if (!replyInfo) > + return; We should NSLog an error message here.
Comment on attachment 428999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428999&action=review > Source/WebKit/ChangeLog:10 > + _invokeMethod's argument-checking loop needs to be run > + even if replyInfo is nil, as otherwise we can call perform an invocation > + with a nil completion handler. I'm not sure this description is complete. I think the problem is that we call [invocation invoke] on an invocation that says it has a block argument by having the description be "@?" but doesn't actually have one. The change looks good, though. I'm not sure a log is necessary but it wouldn't hurt. The other logs are to help with legitimate use, but this can't happen with legitimate use. > Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:112 > +TEST(IPCTestingAPI, CanDetectNilReplyBlocks) Test needs some work. It passes for me even without the fix.
(In reply to Alex Christensen from comment #3) > Comment on attachment 428999 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428999&action=review > > > Source/WebKit/ChangeLog:10 > > + _invokeMethod's argument-checking loop needs to be run > > + even if replyInfo is nil, as otherwise we can call perform an invocation > > + with a nil completion handler. > > I'm not sure this description is complete. I think the problem is that we > call [invocation invoke] on an invocation that says it has a block argument > by having the description be "@?" but doesn't actually have one. > Yep, the `nil` completion block is really from the perspective of the called function. I'll re-word this. > The change looks good, though. I'm not sure a log is necessary but it > wouldn't hurt. The other logs are to help with legitimate use, but this > can't happen with legitimate use. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:112 > > +TEST(IPCTestingAPI, CanDetectNilReplyBlocks) > > Test needs some work. It passes for me even without the fix. Working on this, we turned up some interesting causes in chat.
Created attachment 429105 [details] Patch
Comment on attachment 429105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429105&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:51 > + BOOL didCallSayHello = NO; Don't we usually have curly brackets around the data members? @implementation IPCTestingAPIDelegate { BOOL didCallSayHello; } Also, I don't believe we need the NO initializer in ObjC. > Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:161 > + EXPECT_EQ([delegate.get() sayHelloWasCalled], NO); EXPECT_FALSE([delegate.get() sayHelloWasCalled]);
Comment on attachment 429105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429105&action=review >> Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:51 >> + BOOL didCallSayHello = NO; > > Don't we usually have curly brackets around the data members? > > @implementation IPCTestingAPIDelegate { > BOOL didCallSayHello; > } > > Also, I don't believe we need the NO initializer in ObjC. (and a leading underscore)
Created attachment 429250 [details] Patch
Committed r277849 (237991@main): <https://commits.webkit.org/237991@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429250 [details].