Add necessary colon to CFNetwork selector

[John Wilander]

There needs to be a colon in the CFNetwork selector we're testing for if the selector takes a parameter.

[Radar WebKit Bug Importer]

<rdar://problem/39213124>

[John Wilander]

Created attachment 337280 [details]
Patch

[John Wilander]

Comment on attachment 337280 [details]
Patch

Mac-WK2 build successful.

[John Wilander]

Thanks for the review, Brent!

[WebKit Commit Bot]

Comment on attachment 337280 [details]
Patch

Clearing flags on attachment: 337280

Committed r230311: <https://trac.webkit.org/changeset/230311>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 337280 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:350
> +    if ([request respondsToSelector:@selector(_setIgnoreHSTS:)])
> +        [request performSelector:@selector(_setIgnoreHSTS:) withObject:[NSNumber numberWithBool:ignoreHSTS]];

This code is absolutely *not* correct. It is passing an object to a function that takes a boolean. I will provide a correct example shortly.

[Darin Adler]

There are three problems with these HSTS functions:

1) These functions do not belong in the CFNetworkSPI.h, since they are not CFNetwork SPI, but rather convenience functions to make it easy to call the SPI. That’s not what should be in our SPI.h headers.

2) Two of the functions use the performSelector: return value incorrectly. The performSelector: method's return value only works if the return type is an object, or technically if it’s some non-object pointer type or if the return value is ignored. It is *not* correct to use if the return type is BOOL.

3) One of the functions uses performSelector:withObject: to pass an argument that is not an. Passing an NSNumber when the function takes a boolean won’t work properly; part of the NSNumber pointer is likely to be interpreted as the BOOL.

There are multiple correct ways to do this, none of which involve the performSelector: methods. One is to simply use the methods declared in the CFNetworkSPI.h header and be careful only to call if if the method is implemented. That’s probably the preferred one and easiest to get right. Another is to use <wtf/ObjcRuntimeExtras.h>, which contains correct alternatives to performSelector: for cases like these. A third is to use NSInvocation, which requires passing in the method signature.

Here is a sketch of the the first solution, leaving the code in a style suitable for a header (although I don’t think it should be in a header, and certainly not in CFNetworkSPI.h):

static bool schemeWasUpgradedDueToDynamicHSTS(NSURLRequest *request)
{
#if !USE(CFNETWORK_IGNORE_HSTS)
   UNUSED_PARAM(request);
   return false;
#else
   return [request respondsToSelector:@selector(_schemeWasUpgradedDueToDynamicHSTS)]
       && [request _schemeWasUpgradedDueToDynamicHSTS];
#endif
}

static void setIgnoreHSTS(NSMutableURLRequest *request, bool ignoreHSTS)
{
#if !USE(CFNETWORK_IGNORE_HSTS)
   UNUSED_PARAM(request);
#else
    if ([request respondsToSelector:@selector(_setIgnoreHSTS:)])
        [request _setIgnoreHSTS:ignoreHSTS];
#endif
}

static bool ignoreHSTS(NSURLRequest *request)
{
#if !USE(CFNETWORK_IGNORE_HSTS)
   UNUSED(request);
   return false;
#else
   return [request respondsToSelector:@selector(_ignoreHSTS)]
       && [request _ignoreHSTS];
#endif
}

I suggest filing a new bug and dealing with these issues.

[Darin Adler]

The ObjcRuntimeExtras.h versions would look something like this:

static bool schemeWasUpgradedDueToDynamicHSTS(NSURLRequest *request)
{
   return [request respondsToSelector:@selector(_schemeWasUpgradedDueToDynamicHSTS)]
       && wtfObjcMsgSend<BOOL>(request, @selector(_schemeWasUpgradedDueToDynamicHSTS));
}

static void setIgnoreHSTS(NSMutableURLRequest *request, bool ignoreHSTS)
{
    BOOL ignoreHSTSArgument = ignoreHSTS; // Must be BOOL instead of bool.
    if ([request respondsToSelector:@selector(_setIgnoreHSTS:)])
        wtfObjcMsgSend<void>(request, @selector(_setIgnoreHSTS:), ignoreHSTSArgument);
}

[Darin Adler]

Testing will show what does and does not work in practice. I suspect that the current argument problem in the current setIgnoreHSTS function without my suggested fix always sets the flag to true, but there is a chance it might behave differently on different architectures (ARM vs. Intel, 32-bit vs. 64-bit). The return value part might behave correctly by accident on some architectures, and incorrectly on others. Or might be harmless everywhere although incorrect; not sure.

[John Wilander]

Thanks for your comments, Darin. I will look into them today, including further tests of the existing code.

[John Wilander]

Follow-up work tracked in https://bugs.webkit.org/show_bug.cgi?id=184433.