[iOS WK1] -[_WebSafeForwarder asyncForwarder] uses non-static dispatch_once_t predicate

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

-[_WebSafeForwarder asyncForwarder] uses non-static dispatch_once_t predicate.

@interface _WebSafeForwarder : NSObject
{
    id target; // Non-retained. Don't retain delegates.
    id defaultTarget;
#if PLATFORM(IOS_FAMILY)
    _WebSafeAsyncForwarder *asyncForwarder;
    dispatch_once_t asyncForwarderPred;
#endif
}

[...]

- (id)asyncForwarder
{
    dispatch_once(&asyncForwarderPred, ^{
        asyncForwarder = [[_WebSafeAsyncForwarder alloc] initWithForwarder:self];
    });
    return asyncForwarder;
}

Found by clang static analyzer:

Call to 'dispatch_once' uses the instance variable 'asyncForwarderPred' for the predicate value.  Using such transient memory for the predicate is potentially dangerous

[David Kilzer (:ddkilzer)]

Created attachment 396720 [details]
Patch v1

[Radar WebKit Bug Importer]

<rdar://problem/61910231>

[David Kilzer (:ddkilzer)]

Comment on attachment 396720 [details]
Patch v1

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

> Source/WebKitLegacy/mac/WebView/WebView.mm:552
>  @interface _WebSafeAsyncForwarder : NSObject {
> -    _WebSafeForwarder *_forwarder;
> +    __weak _WebSafeForwarder *_forwarder;

I could have used __unsafe_unretained instead of __weak here to prevent the retain cycle with _WebSafeForwarder (and avoided the overhead of __weak), but every time I see __unsafe_unretained it makes me wonder if there is a bug lurking in the code.

[Daniel Bates]

Comment on attachment 396720 [details]
Patch v1

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

Patch looks good.

> Source/WebKitLegacy/mac/WebView/WebView.mm:563
> +    _WebSafeAsyncForwarder *_asyncForwarder;

This is ok-as is. No change needed. The optimal solution would use RetainPtr because it:

1. Removes the need for -release and nil assignment in -dealloc.
2. Annotates that this ivar is strongly held.
3. Future proofs code for possible ARC transition.

> Source/WebKitLegacy/mac/WebView/WebView.mm:5349
> +@synthesize asyncForwarder=_asyncForwarder;

This is ok as-is. No change needed. A slightly better solution would put spaces around the =.

> Source/WebKitLegacy/mac/WebView/WebView.mm:5376
> +    if ([_target respondsToSelector:[invocation selector]]) {

This is ok as-is. No change needed. Dot notation could also be used on this line and lines below for modernization.

[David Kilzer (:ddkilzer)]

Comment on attachment 396720 [details]
Patch v1

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

>> Source/WebKitLegacy/mac/WebView/WebView.mm:563
>> +    _WebSafeAsyncForwarder *_asyncForwarder;
> 
> This is ok-as is. No change needed. The optimal solution would use RetainPtr because it:
> 
> 1. Removes the need for -release and nil assignment in -dealloc.
> 2. Annotates that this ivar is strongly held.
> 3. Future proofs code for possible ARC transition.

Thanks!  I think the @property statement below also documents #2.

When using RetainPtr<>, getters need to be implemented manually because .get() must be called, correct?  I'd rather use @synthesize to generate the getter and just delete -dealloc when switching to ARC.

>> Source/WebKitLegacy/mac/WebView/WebView.mm:5349
>> +@synthesize asyncForwarder=_asyncForwarder;
> 
> This is ok as-is. No change needed. A slightly better solution would put spaces around the =.

check-webkit-style disagrees:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]

>> Source/WebKitLegacy/mac/WebView/WebView.mm:5376
>> +    if ([_target respondsToSelector:[invocation selector]]) {
> 
> This is ok as-is. No change needed. Dot notation could also be used on this line and lines below for modernization.

I'll use invocation.selector here.  Thanks!

[David Kilzer (:ddkilzer)]

Created attachment 396825 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

Comment on attachment 396720 [details]
Patch v1

Oops, forgot to save this before uploading the land-safely patch.

[EWS]

Committed r260306: <https://trac.webkit.org/changeset/260306>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396825 [details].