Bug 225941 – WKRemoteObjectRegistry _invokeMethod needs to check for nil completionHandlers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Julian Gonzalez]

If a reply block is provided but is nil, we shouldn't call `invokeMethod` - invocations expect callbacks to be callable.

<rdar://problem/75781150>

[Julian Gonzalez]

Created attachment 428999 [details]
Patch

[Ryosuke Niwa]

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.

[Alex Christensen]

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.

[Julian Gonzalez]

(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.

[Julian Gonzalez]

Created attachment 429105 [details]
Patch

[Chris Dumez]

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]);

[Tim Horton]

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)

[Julian Gonzalez]

Created attachment 429250 [details]
Patch

[EWS]

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].