[webkit-reviews] review granted: [Bug 213867] Crash in +[UIViewController _viewControllerForFullScreenPresentationFromView:] when WKContentView is deallocated : [Attachment 403326] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Austin <ablackwood at apple.com>'s
request for review:
Bug 213867: Crash in +[UIViewController
_viewControllerForFullScreenPresentationFromView:] when WKContentView is
deallocated
https://bugs.webkit.org/show_bug.cgi?id=213867

Attachment 403326: Patch

https://bugs.webkit.org/attachment.cgi?id=403326&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 403326
  --> https://bugs.webkit.org/attachment.cgi?id=403326
Patch

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

> Source/WebKit/ChangeLog:8
> +	   No new tests (OOPS!).

Can’t land a patch with this line. Need to delete it.

> Source/WebKit/ChangeLog:11
> +	   (-[WKFileUploadPanel dismiss]):

Should have a comment here. Even a simple one like "Add a null check." would
do.

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:288
> -    [[UIViewController
_viewControllerForFullScreenPresentationFromView:_view.getAutoreleased()]
dismissViewControllerAnimated:NO completion:nil];
> +    UIView *autoreleasedView = _view.getAutoreleased();
> +    if (autoreleasedView)
> +	   [[UIViewController
_viewControllerForFullScreenPresentationFromView:autoreleasedView]
dismissViewControllerAnimated:NO completion:nil];

It’s wasteful and unnecessary to use getAutoreleased here; you didn’t add it,
but it was wrong. Should just be get(). Autoreleased is only needed when the
value is returned from an Objective-C function, not when it’s passed into one.
Also there is no reason for the local variable. Should just be:

    if (_view)
	[[UIViewController
_viewControllerForFullScreenPresentationFromView:_view.get()]
dismissViewControllerAnimated:NO completion:nil];