Bug 95242 – objc_msgSend and IMP should be cast appropriately before using

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Pratik Solanki]

Future compilers/runtimes will warn if we try to use objc_msgSend without the correct casts. We should add casts in WebKit/WebCore when we call objc_msgSend or IMP.

[Pratik Solanki]

<rdar://problem/10906849>

[Pratik Solanki]

Created attachment 161060 [details]
Patch

[Benjamin Poulain]

Comment on attachment 161060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161060&action=review

> Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:87
> -        return objc_msgSend(delegate, selector, self);
> +        return reinterpret_cast<id (*)(id, SEL, id)>(objc_msgSend)(delegate, selector, self);

What about making a bunch of webkit_objc_msgSend like this?:
template<typename Arg1Type, typename Arg2Type>
id webkit_objc_msgSend(id target, SEL selector, Arg1Type arg1, Arg2Type arg2)
{
    return reinterpret_cast<id (*)(id, SEL, Arg1Type, Arg2Type)>(objc_msgSend)(target, selector, arg1, arg2);
}

[Benjamin Poulain]

Maybe we should also modify the style checker to prevent bare use of objc_msgSend?

[Benjamin Poulain]

Comment on attachment 161060 [details]
Patch

Clearing the review flag, Pratik has an update.

[Pratik Solanki]

Created attachment 161264 [details]
Patch

[Pratik Solanki]

(In reply to comment #3)
> (From update of attachment 161060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161060&action=review
> 
> > Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:87
> > -        return objc_msgSend(delegate, selector, self);
> > +        return reinterpret_cast<id (*)(id, SEL, id)>(objc_msgSend)(delegate, selector, self);
> 
> What about making a bunch of webkit_objc_msgSend like this?:
> template<typename Arg1Type, typename Arg2Type>
> id webkit_objc_msgSend(id target, SEL selector, Arg1Type arg1, Arg2Type arg2)
> {
>     return reinterpret_cast<id (*)(id, SEL, Arg1Type, Arg2Type)>(objc_msgSend)(target, selector, arg1, arg2);
> }

This was a great idea. New patch uploaded with this template code.

[David Kilzer (:ddkilzer)]

Comment on attachment 161264 [details]
Patch

r=me

[Benjamin Poulain]

Comment on attachment 161264 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161264&action=review

> Source/WebKit/mac/WebView/WebDelegateImplementationCaching.mm:86
>  
> +template<typename Arg1Type>
> +id webkit_objc_msgSend(id target, SEL selector, Arg1Type arg1)
> +{
> +    return reinterpret_cast<id (*)(id, SEL, Arg1Type)>(objc_msgSend)(target, selector, arg1);
> +}

I would put the templates in a new header in WTF. Then I would ban the use of objc_msgSend through the style checker.

I am afraid new code with objc_msgSend might come without the reviewer knowing about this change.

[Alexey Proskuryakov]

This template is limited in what it can do - specifically, it assumes that the functions returns an id.

[WebKit Review Bot]

Comment on attachment 161264 [details]
Patch

Attachment 161264 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13686455

New failing tests:
CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread

[Pratik Solanki]

How did this fail? Searching for CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread on the log linked to above, I see

[----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate
[ RUN      ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread
[       OK ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread (120 ms)
[----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate (120 ms total)

What failed?

[Benjamin Poulain]

(In reply to comment #12)
> How did this fail? Searching for CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread on the log linked to above, I see
> 
> [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate
> [ RUN      ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread
> [       OK ] CCLayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread (120 ms)
> [----------] 1 test from CCLayerTreeHostTestAtomicCommitWithPartialUpdate (120 ms total)
> 
> What failed?

It is likely a sick bot. You patch does not touch anything on Linux.

[Pratik Solanki]

Created attachment 161524 [details]
Patch

[Benjamin Poulain]

Comment on attachment 161524 [details]
Patch

Very nice!

[Pratik Solanki]

Committed r127193: <http://trac.webkit.org/changeset/127193>

[Pratik Solanki]

Looks like llvm-gcc doesn't like my patch. From <http://webkit-commit-queue.appspot.com/results/13724238>

In file included from /Volumes/Data/WebKit/Source/WebCore/page/mac/EventHandlerMac.mm:53:
/Volumes/Data/WebKit/WebKitBuild/Release/usr/local/include/wtf/ObjcRuntimeExtras.h:29: error: default template arguments may not be used in function templates
/Volumes/Data/WebKit/WebKitBuild/Release/usr/local/include/wtf/ObjcRuntimeExtras.h:35: error: no default argument for 'Arg1Type'
/Volumes/Data/WebKit/WebKitBuild/Release/usr/local/include/wtf/ObjcRuntimeExtras.h:35: error: default template arguments may not be used in function templates
....
and more such errors like this. I'm looking into it.

[Pratik Solanki]

Ok. I can reproduce the build failure by switching to llvm-gcc-4.2 on Mountain Lion (and removing a few -W arguments to the compiler). The template format is

template<typename RetType = id, typename Arg1Type>
RetType wtfObjcMsgSend(id target, SEL selector, Arg1Type arg1)
{
    return reinterpret_cast<RetType (*)(id, SEL, Arg1Type)>(objc_msgSend)(target, selector, arg1);
}

If I remove the " = id" part, it's happy. I could do that. And then have each call site explicitly specify the return type. Alternatively, what are the chances we can get the EWS bot updated with a more recent toolchain?

[Benjamin Poulain]

> If I remove the " = id" part, it's happy. I could do that. And then have each call site explicitly specify the return type. Alternatively, what are the chances we can get the EWS bot updated with a more recent toolchain?

I am afraid we have to support the old compilers.
If the EWS have an old SDK, chances are many people do too.

I think the best is add an explanation in a FIXME and remove the default arg :( :(

[Jessie Berlin]

(In reply to comment #19)
> > If I remove the " = id" part, it's happy. I could do that. And then have each call site explicitly specify the return type. Alternatively, what are the chances we can get the EWS bot updated with a more recent toolchain?
> 
> I am afraid we have to support the old compilers.
> If the EWS have an old SDK, chances are many people do too.
> 
> I think the best is add an explanation in a FIXME and remove the default arg :( :(

I agree with Benjamin. Please either do that soon or roll out http://trac.webkit.org/changeset/127193 (it is causing the mac-ews queue to get clogged).

[Pratik Solanki]

Working on this now. I should have a patch soon.

[Pratik Solanki]

Reopening to attach new patch.

[Pratik Solanki]

Created attachment 161752 [details]
Fix for llvm-gcc

[Benjamin Poulain]

Comment on attachment 161752 [details]
Fix for llvm-gcc

Looks good.

I think we should land this so we can take the EWS back online for everyone. Then we fix the EWS and we can eventually revert the patch.

[Pratik Solanki]

Committed r127306: <http://trac.webkit.org/changeset/127306>

[Pratik Solanki]

Reopening to attach new patch.

[Pratik Solanki]

Created attachment 161762 [details]
Patch

[Benjamin Poulain]

Comment on attachment 161762 [details]
Patch

Damn GCC.

[Pratik Solanki]

Committed r127313: <http://trac.webkit.org/changeset/127313>