Bug 213867

Summary: Crash in +[UIViewController _viewControllerForFullScreenPresentationFromView:] when WKContentView is deallocated
Product: WebKit Reporter: Austin <ablackwood>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, david_quesada, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Austin 2020-07-01 16:02:05 PDT
Crash in +[UIViewController _viewControllerForFullScreenPresentationFromView:] when WKContentView is deallocated
Comment 1 Austin 2020-07-01 16:02:48 PDT
Created attachment 403326 [details]
Patch
Comment 2 Darin Adler 2020-07-01 16:27:03 PDT
Comment on attachment 403326 [details]
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];
Comment 3 Austin 2020-07-02 09:50:13 PDT
Created attachment 403374 [details]
Patch
Comment 4 Darin Adler 2020-07-02 11:26:51 PDT
Comment on attachment 403374 [details]
Patch

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

> Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:287
> +    RetainPtr<WKContentView> view = _view.get();
> +    if (view)

OK as is. But could write this:

    if (RetainPtr<WKContentView> view = _view.get())

or this:

    if (auto view = retainPtr(_view.get()))
Comment 5 Austin 2020-07-02 14:12:16 PDT
Created attachment 403394 [details]
Patch
Comment 6 EWS 2020-07-02 15:40:29 PDT
ablackwood@apple.com does not have committer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 403394 [details] from commit queue.
Comment 7 EWS 2020-07-02 15:44:04 PDT
Committed r263869: <https://trac.webkit.org/changeset/263869>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403394 [details].
Comment 8 Radar WebKit Bug Importer 2020-07-02 15:45:20 PDT
<rdar://problem/65053840>