-[_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
Created attachment 396720 [details] Patch v1
<rdar://problem/61910231>
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.
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.
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!
Created attachment 396825 [details] Patch for landing
Comment on attachment 396720 [details] Patch v1 Oops, forgot to save this before uploading the land-safely patch.
Committed r260306: <https://trac.webkit.org/changeset/260306> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396825 [details].