Bug 213867 - Crash in +[UIViewController _viewControllerForFullScreenPresentationFromView:] when WKContentView is deallocated
Summary: Crash in +[UIViewController _viewControllerForFullScreenPresentationFromView:...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-01 16:02 PDT by Austin
Modified: 2020-07-02 15:45 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.82 KB, patch)
2020-07-01 16:02 PDT, Austin
no flags Details | Formatted Diff | Diff
Patch (2.19 KB, patch)
2020-07-02 09:50 PDT, Austin
no flags Details | Formatted Diff | Diff
Patch (2.16 KB, patch)
2020-07-02 14:12 PDT, Austin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>