Bug 165225

Summary: UIViewController with WKWebView presented modally causes the presented UIViewController to be dismissed.
Product: WebKit Reporter: Brad Wright <bwright2>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Blocker CC: achristensen, andersca, buildbot, commit-queue, darin, joepeck, mitz, rniwa, sam, thorton, wenson_hsieh, zwaugh
Priority: P1 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: iPhone / iPad   
OS: All   
Attachments:
Description Flags
Patch
none
Example Project that demonstrates the problem
none
New Patch with changes made to conform to the style guide
none
Able to now fully test in the simulator. Add critical fixes to WKFileUploadPanel.mm
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Example test project for reproducing the problem.
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Patch
none
Patch
none
Patch none

Description Brad Wright 2016-11-30 15:00:14 PST
UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.
Comment 1 Brad Wright 2016-12-01 12:44:26 PST
Created attachment 295877 [details]
Patch
Comment 2 WebKit Commit Bot 2016-12-01 12:46:45 PST
Attachment 295877 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:559:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brad Wright 2016-12-01 14:50:05 PST
I recreated this original problem in a WkWebView project.
Comment 4 Brad Wright 2016-12-01 15:05:27 PST
Created attachment 295902 [details]
Example Project that demonstrates the problem

The example project that demonstrates the problem in the WKWebView that this change is trying to solve.

1. Compile and run the project in an iPhone Simulator.
2. When the simulator launches, click on the button in the main view called "launch modal web view".
3. Click the "Choose File" button.
4. Select "iCloud" or "Photo Library" from the menu.
5. The modal web view gets dismissed by this WebKit bug.

The web view should not get dismissed.  The problem here is that the WebKit is trying to close a UIDocumentMenuViewController by calling dismissViewControllerAnimated.  The UIDocumentMenuViewController automatically dismisses itself.  This extra call to dismissViewControllerAnimated is dismissing the modally presented WkWebView.
Comment 5 Joseph Pecoraro 2016-12-01 15:32:24 PST
Comment on attachment 295877 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.

This title seems insufficient. It seems the issues is specific to File Upload so that should be mentioned.

Perhaps a better title would be:

    WKWebView file upload dialog may incorrectly dismiss unrelated modal view controller

Does that sound accurate to you?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557
> -    if (UICurrentUserInterfaceIdiomIsPad())
> +    if (UICurrentUserInterfaceIdiomIsPad()) {
>          [self _presentPopoverWithContentViewController:viewController animated:YES];
> -    else
> -        [self _presentFullscreenViewController:viewController animated:YES];
> +    } else {

Style: WebKit's Style guideline is to not use braces for single statement conditional blocks. So these braces for the if should not be added. The else, which is multiline should have braces.
https://webkit.org/code-style-guidelines/#braces-one-line

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:560
> +        _presentationViewController = nil;

Hmm, this doesn't seem right.

  - The previous code attempts to dismiss the old view controller it presented and is then presenting the new view controller.
  - The new code ignores the old view controller and presents the new view controller.

Shouldn't we continue to dismiss the old view controller? if _presentationViewController was the incorrect view controller to be dismissing (an ancestor), then perhaps _presentationViewController's presentedViewController would be the correct view controller to dismiss instead of dismissing nothing.

What do you think?
Comment 6 Brad Wright 2016-12-01 16:58:56 PST
(In reply to comment #5)
> Comment on attachment 295877 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295877&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.
> 
> This title seems insufficient. It seems the issues is specific to File
> Upload so that should be mentioned.
> 
> Perhaps a better title would be:
> 
>     WKWebView file upload dialog may incorrectly dismiss unrelated modal
> view controller
> 
> Does that sound accurate to you?
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557
> > -    if (UICurrentUserInterfaceIdiomIsPad())
> > +    if (UICurrentUserInterfaceIdiomIsPad()) {
> >          [self _presentPopoverWithContentViewController:viewController animated:YES];
> > -    else
> > -        [self _presentFullscreenViewController:viewController animated:YES];
> > +    } else {
> 
> Style: WebKit's Style guideline is to not use braces for single statement
> conditional blocks. So these braces for the if should not be added. The
> else, which is multiline should have braces.
> https://webkit.org/code-style-guidelines/#braces-one-line
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:560
> > +        _presentationViewController = nil;
> 
> Hmm, this doesn't seem right.
> 
>   - The previous code attempts to dismiss the old view controller it
> presented and is then presenting the new view controller.
>   - The new code ignores the old view controller and presents the new view
> controller.

You don't need to dismiss this view controller because it dismisses itself. By explicitly calling another dismiss, the code is a duplicate dismiss which then removes the modal view controller.

This problem will never appear in Safari, because there are no modally presented views.  If you run the same sample project that I uploaded, you will see that the modal view controller is not dismissed.  This is because the popup VC code path does not call dismissViewControllerAnimated.  The problem only happens on a modal VC with a WkWebView and on the iPhone.

> 
> Shouldn't we continue to dismiss the old view controller? if
> _presentationViewController was the incorrect view controller to be
> dismissing (an ancestor), then perhaps _presentationViewController's
> presentedViewController would be the correct view controller to dismiss
> instead of dismissing nothing.
> 
> What do you think?
Comment 7 Brad Wright 2016-12-01 17:01:03 PST
(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 295877 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=295877&action=review
> > 
> > > Source/WebKit2/ChangeLog:3
> > > +        UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.
> > 
> > This title seems insufficient. It seems the issues is specific to File
> > Upload so that should be mentioned.
> > 
> > Perhaps a better title would be:
> > 
> >     WKWebView file upload dialog may incorrectly dismiss unrelated modal
> > view controller
> > 
> > Does that sound accurate to you?
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557
> > > -    if (UICurrentUserInterfaceIdiomIsPad())
> > > +    if (UICurrentUserInterfaceIdiomIsPad()) {
> > >          [self _presentPopoverWithContentViewController:viewController animated:YES];
> > > -    else
> > > -        [self _presentFullscreenViewController:viewController animated:YES];
> > > +    } else {
> > 
> > Style: WebKit's Style guideline is to not use braces for single statement
> > conditional blocks. So these braces for the if should not be added. The
> > else, which is multiline should have braces.
> > https://webkit.org/code-style-guidelines/#braces-one-line
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:560
> > > +        _presentationViewController = nil;
> > 
> > Hmm, this doesn't seem right.
> > 
> >   - The previous code attempts to dismiss the old view controller it
> > presented and is then presenting the new view controller.
> >   - The new code ignores the old view controller and presents the new view
> > controller.
> 
> You don't need to dismiss this view controller because it dismisses itself.
> By explicitly calling another dismiss, the code is a duplicate dismiss which
> then removes the modal view controller.
> 
> This problem will never appear in Safari, because there are no modally
> presented views.  If you run the same sample project that I uploaded on the iPad, you
> will see that the modal view controller is not dismissed.  This is because
> the popup VC code path does not call dismissViewControllerAnimated.  The
> problem only happens on a modal VC with a WkWebView and on the iPhone.
> 
> > 
> > Shouldn't we continue to dismiss the old view controller? if
> > _presentationViewController was the incorrect view controller to be
> > dismissing (an ancestor), then perhaps _presentationViewController's
> > presentedViewController would be the correct view controller to dismiss
> > instead of dismissing nothing.
> > 
> > What do you think?
Comment 8 Brad Wright 2016-12-01 17:09:19 PST
(In reply to comment #5)
> Comment on attachment 295877 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295877&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        UIViewController with WkWebView presented modally causes the presented UIViewController to be dismissed.
> 
> This title seems insufficient. It seems the issues is specific to File
> Upload so that should be mentioned.
> 
> Perhaps a better title would be:
> 
>     WKWebView file upload dialog may incorrectly dismiss unrelated modal
> view controller
> 
> Does that sound accurate to you?
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:557
> > -    if (UICurrentUserInterfaceIdiomIsPad())
> > +    if (UICurrentUserInterfaceIdiomIsPad()) {
> >          [self _presentPopoverWithContentViewController:viewController animated:YES];
> > -    else
> > -        [self _presentFullscreenViewController:viewController animated:YES];
> > +    } else {
> 
> Style: WebKit's Style guideline is to not use braces for single statement
> conditional blocks. So these braces for the if should not be added. The
> else, which is multiline should have braces.
> https://webkit.org/code-style-guidelines/#braces-one-line
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:560
> > +        _presentationViewController = nil;
> 
> Hmm, this doesn't seem right.
> 
>   - The previous code attempts to dismiss the old view controller it
> presented and is then presenting the new view controller.
>   - The new code ignores the old view controller and presents the new view
> controller.
> 
> Shouldn't we continue to dismiss the old view controller? if
> _presentationViewController was the incorrect view controller to be
> dismissing (an ancestor), then perhaps _presentationViewController's
> presentedViewController would be the correct view controller to dismiss
> instead of dismissing nothing.
> 
> What do you think?

If you run the included example project on the iPhone, you will see the WkWebView get's dismissed when it shouldn't.  Then try running on the iPad, and you will see the WkWebView doesn't get dismissed.  This is because there is no extra call to dismissViewControllerAnimated.

Every time you call, dissmissViewControllerAnimaged it just removes another ViewController from the stack of modally presented View Controllers.  UIDocumentMenuViewController already removes itself from the stack automatically.  Thus this duplicate call, is dismissing the wrong ViewController.
Comment 9 Brad Wright 2016-12-02 09:16:05 PST
Here's the relative section from Apple's documentation on dismissViewControllerAnimated:

"If you present several view controllers in succession, thus building a stack of presented view controllers, calling this method on a view controller lower in the stack dismisses its immediate child view controller and all view controllers above that child on the stack."

In other words, don't call dismissViewControllerAnimated when you don't need to.
Comment 10 Brad Wright 2016-12-05 12:29:56 PST
We have many customers affected by this issue:

<rdar://problem/25565732>
Comment 11 Wenson Hsieh 2016-12-09 07:48:36 PST
Comment on attachment 295877 [details]
Patch

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

> Source/WebKit2/ChangeLog:7
> +

^ The above information would be great to include in the ChangeLog here, after the "Reviewed by" line.
Comment 12 Tim Horton 2017-02-22 13:33:05 PST
This is rdar://problem/28723142.
Comment 13 Brad Wright 2017-02-24 13:23:44 PST
Created attachment 302681 [details]
New Patch with changes made to conform to the style guide

No significant changes here.  I have been able to run this through and test it on both the iPad and iPhone.
Comment 14 Brad Wright 2017-02-27 17:39:43 PST
Created attachment 302901 [details]
Able to now fully test in the simulator.  Add critical fixes to WKFileUploadPanel.mm

This has now been tested against the test project in the simulator to verify that the fix does indeed work.
Comment 15 Wenson Hsieh 2017-02-27 17:43:47 PST
(In reply to comment #14)
> Created attachment 302901 [details]
> Able to now fully test in the simulator.  Add critical fixes to
> WKFileUploadPanel.mm
> 
> This has now been tested against the test project in the simulator to verify
> that the fix does indeed work.

It doesn't look like you uploaded the full patch here. You can use `./Tools/Scripts/webkit-patch upload` to upload the new version of your patch.
Comment 16 Brad Wright 2017-02-27 17:53:33 PST
Created attachment 302902 [details]
Patch
Comment 17 Wenson Hsieh 2017-02-27 19:31:00 PST
Comment on attachment 302902 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:583
> +    _presentationViewController = nil;

I don't think there's any purpose in keeping the _presentationViewController ivar if it's going to be nil in all circumstances outside of this method. Is this approach any different than just removing

    if (_presentationViewController) {
        [_presentationViewController dismissViewControllerAnimated:animated completion:^{
            _presentationViewController = nil;
        }];
    }

in -_dismissDisplayAnimated:?
Comment 18 Wenson Hsieh 2017-02-28 07:29:50 PST
Comment on attachment 302902 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        Need a short description: Fixed the file WKFileUploadPanel.mm and was able run the test app inside the simulator.  

You can remove the "Need a short description:" template here.

> Source/WebKit2/ChangeLog:4
> +        Need the bug URL:https://bugs.webkit.org/show_bug.cgi?id=165225

You can remove the "Need the bug URL:" template here.

> Source/WebKit2/ChangeLog:15
> +2017-02-27  Brad Wright  <bwright2@apple.com>

Please remove this extra ChangeLog entry.
Comment 19 Brad Wright 2017-02-28 17:25:34 PST
Created attachment 303014 [details]
Patch
Comment 20 Brad Wright 2017-02-28 17:32:09 PST
(In reply to comment #17)
> Comment on attachment 302902 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302902&action=review
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:583
> > +    _presentationViewController = nil;
> 
> I don't think there's any purpose in keeping the _presentationViewController
> ivar if it's going to be nil in all circumstances outside of this method. Is
> this approach any different than just removing
> 
>     if (_presentationViewController) {
>         [_presentationViewController dismissViewControllerAnimated:animated
> completion:^{
>             _presentationViewController = nil;
>         }];
>     }
> 
> in -_dismissDisplayAnimated:?

I found that the _presentationViewController should not be removed.  I went down the path of removing it, and found that it potentially needed in the class's dismiss routine which was being called externally.  I found that the much easier solution was to simply set the _presentationViewController to nil when the UIDocumentMenuViewController controller was being presented.  Overall, this makes the proposed changes much smaller.  It reduces the risk of any regression bugs from this change.
Comment 21 Brad Wright 2017-03-01 13:56:39 PST
Created attachment 303114 [details]
Patch
Comment 22 Wenson Hsieh 2017-03-01 14:05:08 PST
Comment on attachment 303114 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:-389
> -    [self _dismissDisplayAnimated:NO];

_dismissDisplayAnimated: handles dismissing the _presentationPopover. If we aren't going to go through _dismissDisplayAnimated: here, we need to still dismissPopoverAnimated: on the _presentationPopover.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
> +    UIViewController *presentedVC = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];

WebKit style guidelines (https://webkit.org/code-style-guidelines/) recommend using full variable names rather than abbreviations, so presentedVC should be presentedViewController.
Comment 23 Brad Wright 2017-03-01 14:15:19 PST
(In reply to comment #22)
> Comment on attachment 303114 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303114&action=review
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:-389
> > -    [self _dismissDisplayAnimated:NO];
> 
> _dismissDisplayAnimated: handles dismissing the _presentationPopover. If we
> aren't going to go through _dismissDisplayAnimated: here, we need to still
> dismissPopoverAnimated: on the _presentationPopover.

The original dismiss had animated set to NO.  I tested this condition on the iPad with the popover displayed, and it correctly removed the popover from the screen.  Therefore, I believe that calling dismissPopoverAnimated is not needed under this condition, since animated is always set to NO.
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
> > +    UIViewController *presentedVC = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> 
> WebKit style guidelines (https://webkit.org/code-style-guidelines/)
> recommend using full variable names rather than abbreviations, so
> presentedVC should be presentedViewController.

Sounds good, I'll change that.
Comment 24 Brad Wright 2017-03-01 15:00:35 PST
Created attachment 303130 [details]
Patch
Comment 25 Darin Adler 2017-03-01 15:10:33 PST
Comment on attachment 303130 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.

Normally we would want to use a capital D in this kind of sentence-style comment.

VC is not normally an abbreviation we use for view controller in WebKit code.

"This is works all", is not grammatically correct.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> +    if (presentedViewController)
> +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];

Would be nice to define the variable inside the "if" since this is C++ code. In fact, I personally would use auto instead of writing out UIViewController * for the type of the variable.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> +    _presentationViewController = nil;

I don’t understand this comment and therefore don’t understand the code.

On iPhone, the method called just before this will call _presentFullscreenViewController. Then this line of code will clear out the reference to that just-presented view controller. Given that, why does this comment talk about clearing out a "previous view controller reference"?
Comment 26 Brad Wright 2017-03-01 15:18:20 PST
(In reply to comment #25)
> Comment on attachment 303130 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303130&action=review
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> > +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.

This is works for all VC.  Missed the for, in the comment.
> 
> Normally we would want to use a capital D in this kind of sentence-style
> comment.
> 
> VC is not normally an abbreviation we use for view controller in WebKit code.
> 
> "This is works all", is not grammatically correct.
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> > +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> > +    if (presentedViewController)
> > +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];
> 
> Would be nice to define the variable inside the "if" since this is C++ code.
> In fact, I personally would use auto instead of writing out UIViewController
> * for the type of the variable.
> 
> > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> > +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> > +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> > +    _presentationViewController = nil;
> 
> I don’t understand this comment and therefore don’t understand the code.

So what happening is that the _presentationViewController is used to called in:

- (void)_dismissDisplayAnimated:(BOOL)animated
{
    if (_presentationPopover) {
        [_presentationPopover dismissPopoverAnimated:animated];
        [_presentationPopover setDelegate:nil];
        _presentationPopover = nil;
    }

    if (_presentationViewController) {
        [_presentationViewController dismissViewControllerAnimated:animated completion:^{
            _presentationViewController = nil;
        }];
    }
}


The UIDocumentMenuViewController dismisses itself, so when _presentationViewController dismissViewControllerAnimated:animated is called in this function, it dismisses the WKWebView when is presented modally.  That's why it's set to nil in this function, so that the errant code path is never executed.
> 
> On iPhone, the method called just before this will call
> _presentFullscreenViewController. Then this line of code will clear out the
> reference to that just-presented view controller. Given that, why does this
> comment talk about clearing out a "previous view controller reference"?
Comment 27 Brad Wright 2017-03-01 15:33:39 PST
Created attachment 303134 [details]
Patch
Comment 28 Brad Wright 2017-03-01 15:54:20 PST
(In reply to comment #26)
> (In reply to comment #25)
> > Comment on attachment 303130 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=303130&action=review
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> > > +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.
> 
> This is works for all VC.  Missed the for, in the comment.
> > 
> > Normally we would want to use a capital D in this kind of sentence-style
> > comment.
> > 
> > VC is not normally an abbreviation we use for view controller in WebKit code.
> > 
> > "This is works all", is not grammatically correct.
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> > > +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> > > +    if (presentedViewController)
> > > +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];
> > 
> > Would be nice to define the variable inside the "if" since this is C++ code.
> > In fact, I personally would use auto instead of writing out UIViewController
> > * for the type of the variable.
> > 
> > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> > > +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> > > +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> > > +    _presentationViewController = nil;
> > 
> > I don’t understand this comment and therefore don’t understand the code.
> 
> So what happening is that the _presentationViewController is used to called
> in:
> 
> - (void)_dismissDisplayAnimated:(BOOL)animated
> {
>     if (_presentationPopover) {
>         [_presentationPopover dismissPopoverAnimated:animated];
>         [_presentationPopover setDelegate:nil];
>         _presentationPopover = nil;
>     }
> 
>     if (_presentationViewController) {
>         [_presentationViewController dismissViewControllerAnimated:animated
> completion:^{
>             _presentationViewController = nil;
>         }];
>     }
> }
> 
> 
> The UIDocumentMenuViewController dismisses itself, so when
> _presentationViewController dismissViewControllerAnimated:animated is called
> in this function, it dismisses the WKWebView when is presented modally. 
> That's why it's set to nil in this function, so that the errant code path is
> never executed.
> > 
> > On iPhone, the method called just before this will call
> > _presentFullscreenViewController. Then this line of code will clear out the
> > reference to that just-presented view controller. Given that, why does this
> > comment talk about clearing out a "previous view controller reference"?


Inside the call stack this is the reference that needs to be cleared out.

1. when this called
- (void)_showDocumentPickerMenu
2.It then calls this:
- (void)_presentMenuOptionForCurrentInterfaceIdiom:(UIViewController *)viewController
3. Finally, inside here the reference is saved.
- (void)_presentFullscreenViewController:(UIViewController *)viewController animated:(BOOL)animated
{
    ...
    _presentationViewController = [UIViewController 
...
}

4. When we return up the stack, I need to unset the reference.

- (void)_showDocumentPickerMenu


I put this comment and the nil reference as close together as possible to minimize future errors.
Comment 29 Brad Wright 2017-03-01 15:57:03 PST
(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > Comment on attachment 303130 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=303130&action=review
> > > 
> > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> > > > +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.
> > 
> > This is works for all VC.  Missed the for, in the comment.
> > > 
> > > Normally we would want to use a capital D in this kind of sentence-style
> > > comment.
> > > 
> > > VC is not normally an abbreviation we use for view controller in WebKit code.
> > > 
> > > "This is works all", is not grammatically correct.
> > > 
> > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> > > > +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> > > > +    if (presentedViewController)
> > > > +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];
> > > 
> > > Would be nice to define the variable inside the "if" since this is C++ code.
> > > In fact, I personally would use auto instead of writing out UIViewController
> > > * for the type of the variable.
> > > 
> > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> > > > +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> > > > +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> > > > +    _presentationViewController = nil;
> > > 
> > > I don’t understand this comment and therefore don’t understand the code.
> > 
> > So what happening is that the _presentationViewController is used to called
> > in:
> > 
> > - (void)_dismissDisplayAnimated:(BOOL)animated
> > {
> >     if (_presentationPopover) {
> >         [_presentationPopover dismissPopoverAnimated:animated];
> >         [_presentationPopover setDelegate:nil];
> >         _presentationPopover = nil;
> >     }
> > 
> >     if (_presentationViewController) {
> >         [_presentationViewController dismissViewControllerAnimated:animated
> > completion:^{
> >             _presentationViewController = nil;
> >         }];
> >     }
> > }
> > 
> > 
> > The UIDocumentMenuViewController dismisses itself, so when
> > _presentationViewController dismissViewControllerAnimated:animated is called
> > in this function, it dismisses the WKWebView when is presented modally. 
> > That's why it's set to nil in this function, so that the errant code path is
> > never executed.
> > > 
> > > On iPhone, the method called just before this will call
> > > _presentFullscreenViewController. Then this line of code will clear out the
> > > reference to that just-presented view controller. Given that, why does this
> > > comment talk about clearing out a "previous view controller reference"?
> 
> 
> Inside the call stack this is the reference that needs to be cleared out.
> 
> 1. We start here inside:
> - (void)_showDocumentPickerMenu
> 2.Then this is called:
> - (void)_presentMenuOptionForCurrentInterfaceIdiom:(UIViewController
> *)viewController
> 3. Finally, inside here the property _presentationViewController is set.
> - (void)_presentFullscreenViewController:(UIViewController *)viewController
> animated:(BOOL)animated
> {
>     ...
>     _presentationViewController = [UIViewController 
> ...
> }
> 
> 4. When we return up the stack, we need to set _presentationViewController back to nil.
> 
> - (void)_showDocumentPickerMenu
> 
> 
> I put this comment and the nil reference as close together as possible to
> minimize future errors.
Comment 30 Brad Wright 2017-03-02 09:57:49 PST
(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (In reply to comment #25)
> > > > Comment on attachment 303130 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=303130&action=review
> > > > 
> > > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> > > > > +    // dismiss any viewController that is being presented. This is works all VC, popovers, etc.
> > > 
> > > This is works for all VC.  Missed the for, in the comment.
> > > > 
> > > > Normally we would want to use a capital D in this kind of sentence-style
> > > > comment.
> > > > 
> > > > VC is not normally an abbreviation we use for view controller in WebKit code.
> > > > 
> > > > "This is works all", is not grammatically correct.
> > > > 
> > > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:394
> > > > > +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> > > > > +    if (presentedViewController)
> > > > > +        [presentedViewController dismissViewControllerAnimated:NO completion:nil];
> > > > 
> > > > Would be nice to define the variable inside the "if" since this is C++ code.
> > > > In fact, I personally would use auto instead of writing out UIViewController
> > > > * for the type of the variable.
> > > > 
> > > > > Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:508
> > > > > +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that
> > > > > +    // was no longer being presented. Don't save a reference to the UIDocumentMenuViewController as it is self dismissing.
> > > > > +    _presentationViewController = nil;
> > > > 
> > > > I don’t understand this comment and therefore don’t understand the code.
> > > 
> > > So what happening is that the _presentationViewController is used to called
> > > in:
> > > 
> > > - (void)_dismissDisplayAnimated:(BOOL)animated
> > > {
> > >     if (_presentationPopover) {
> > >         [_presentationPopover dismissPopoverAnimated:animated];
> > >         [_presentationPopover setDelegate:nil];
> > >         _presentationPopover = nil;
> > >     }
> > > 
> > >     if (_presentationViewController) {
> > >         [_presentationViewController dismissViewControllerAnimated:animated
> > > completion:^{
> > >             _presentationViewController = nil;
> > >         }];
> > >     }
> > > }
> > > 
> > > 
> > > The UIDocumentMenuViewController dismisses itself, so when
> > > _presentationViewController dismissViewControllerAnimated:animated is called
> > > in this function, it dismisses the WKWebView when is presented modally. 
> > > That's why it's set to nil in this function, so that the errant code path is
> > > never executed.
> > > > 
> > > > On iPhone, the method called just before this will call
> > > > _presentFullscreenViewController. Then this line of code will clear out the
> > > > reference to that just-presented view controller. Given that, why does this
> > > > comment talk about clearing out a "previous view controller reference"?
> > 
> > 
> > Inside the call stack this is the reference that needs to be cleared out.
> > 
> > 1. We start here inside:
> > - (void)_showDocumentPickerMenu
> > 2.Then this is called:
> > - (void)_presentMenuOptionForCurrentInterfaceIdiom:(UIViewController
> > *)viewController
> > 3. Finally, inside here the property _presentationViewController is set.
> > - (void)_presentFullscreenViewController:(UIViewController *)viewController
> > animated:(BOOL)animated
> > {
> >     ...
> >     _presentationViewController = [UIViewController 
> > ...
> > }
> > 
> > 4. When we return up the stack, we need to set _presentationViewController back to nil.
> > 
> > - (void)_showDocumentPickerMenu
> > 
> > 
> > I put this comment and the nil reference as close together as possible to
> > minimize future errors.

The above code is not C++, but Objective-C.  I know this is a mixed Objective-C and C++  .mm file, but I'm not aware of any memory overhead penalty for this pointer variable in Objective-C.
Comment 31 Brad Wright 2017-03-02 14:56:18 PST
Created attachment 303250 [details]
Example test project for reproducing the problem.

Here's an example test project that was created to reproduce the problem.  

To Test:

1.  Drag the project into the WebKit.xcworkspace
2.  Compile and run.
3.  Click on the button on the main window to launch the modal WKWebView.
4.  Click on the button inside the WKWebView.
5.  Select Photos or Cloud from the menu.
6.  Without the fix, the Modal WKWebView will get disappear.
7.  With the fix, you will be able to select a photo.
Comment 32 Wenson Hsieh 2017-03-02 16:07:02 PST
Comment on attachment 303134 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // dismiss any viewController that is being presented. This is works for all types of viewcontrollers , popovers, etc.

Please capitalize "dismiss"

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
> +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];

So if I understand correctly, [UIViewController _viewControllerForFullScreenPresentationFromView:_view] will fetch the _presentationPopover, if it exists, as well? If that is really the case, then this is OK, but we should at least tear down _presentationPopover properly (i.e. nil out _presentationPopover and set its delegate to nil).

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:506
> +    // Clear out any previous view controller reference, to prevent calling dismiss on a view controller that

Per Darin's advice a few comments above, this should really say "Clear out the view controller we just presented" rather than refer to the view controller we're clearing out as the "previous" one.
Comment 33 Wenson Hsieh 2017-03-02 16:18:35 PST
Comment on attachment 303134 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        Don't dismiss the viewcontroller if the viewcontroller dismisses itself.  - (void)dismiss now removes any presented view controller directly.

Please fix this title as well.
Comment 34 Wenson Hsieh 2017-03-02 16:47:38 PST
Comment on attachment 303134 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:393
> +    if (presentedViewController)

This nil check is not needed (if !presentedViewController, this will just be a noop).
Comment 35 Brad Wright 2017-03-02 16:53:56 PST
Comment on attachment 303134 [details]
Patch

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

>> Source/WebKit2/ChangeLog:3
>> +        Don't dismiss the viewcontroller if the viewcontroller dismisses itself.  - (void)dismiss now removes any presented view controller directly.
> 
> Please fix this title as well.

Is this okay?  

Clear out the view controller we just presented. Don't dismiss the UIDocumentMenuViewController as it is self dismissing.
Comment 36 Wenson Hsieh 2017-03-02 16:56:28 PST
(In reply to comment #35)
> Comment on attachment 303134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303134&action=review
> 
> >> Source/WebKit2/ChangeLog:3
> >> +        Don't dismiss the viewcontroller if the viewcontroller dismisses itself.  - (void)dismiss now removes any presented view controller directly.
> > 
> > Please fix this title as well.
> 
> Is this okay?  
> 
> Clear out the view controller we just presented. Don't dismiss the
> UIDocumentMenuViewController as it is self dismissing.

Oh, sorry -- I should've clarified; typically, the title of the ChangeLog entry is the title of the bug itself (so in this case, it would be 'UIViewController with WKWebView presented modally causes the presented UIViewController to be dismissed.')
Comment 37 Tim Horton 2017-03-02 17:03:15 PST
(In reply to comment #36)
> (In reply to comment #35)
> > Comment on attachment 303134 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=303134&action=review
> > 
> > >> Source/WebKit2/ChangeLog:3
> > >> +        Don't dismiss the viewcontroller if the viewcontroller dismisses itself.  - (void)dismiss now removes any presented view controller directly.
> > > 
> > > Please fix this title as well.
> > 
> > Is this okay?  
> > 
> > Clear out the view controller we just presented. Don't dismiss the
> > UIDocumentMenuViewController as it is self dismissing.
> 
> Oh, sorry -- I should've clarified; typically, the title of the ChangeLog
> entry is the title of the bug itself (so in this case, it would be
> 'UIViewController with WKWebView presented modally causes the presented
> UIViewController to be dismissed.')

Take a look at https://trac.webkit.org/changeset/213304 for a good example of change log format.
Comment 38 Brad Wright 2017-03-02 17:38:23 PST
Comment on attachment 303134 [details]
Patch

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

>> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
>> +    UIViewController *presentedViewController = [UIViewController _viewControllerForFullScreenPresentationFromView:_view];
> 
> So if I understand correctly, [UIViewController _viewControllerForFullScreenPresentationFromView:_view] will fetch the _presentationPopover, if it exists, as well? If that is really the case, then this is OK, but we should at least tear down _presentationPopover properly (i.e. nil out _presentationPopover and set its delegate to nil).

We don't actually need to call [_presentationPopover dismissPopoverAnimated:animated]; because this will take care of it:
[[UIViewController _viewControllerForFullScreenPresentationFromView:_view] dismissViewControllerAnimated:NO completion:nil];
I'm going to add this:
// Clean up any pointers
    [_presentationPopover setDelegate:nil];
    _presentationPopover = nil;
    _presentationViewController = nil;
Comment 39 Brad Wright 2017-03-02 18:11:55 PST
Created attachment 303279 [details]
Patch
Comment 40 Wenson Hsieh 2017-03-02 18:14:09 PST
Comment on attachment 303279 [details]
Patch

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

Added a very minor comment. I think this is a reasonable change, but I'm not a WK2 reviewer -- Tim, what do you think?

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:396
> +    // Clean up any pointers

Nit - I don't think this comment adds much.
Comment 41 Tim Horton 2017-03-02 18:17:08 PST
Comment on attachment 303279 [details]
Patch

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

> Source/WebKit2/ChangeLog:13
> +        (-[WKFileUploadPanel _cancel]):
> +        (-[WKFileUploadPanel dismiss]):
> +        (-[WKFileUploadPanel _showDocumentPickerMenu]):
> +        (-[WKFileUploadPanel _presentMenuOptionForCurrentInterfaceIdiom:]):
> +        (-[WKFileUploadPanel _presentForCurrentInterfaceIdiom:]): Deleted.

Ideally this would explain why you're doing what you're doing.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // Dismiss any viewController that is being presented. This works for all types of viewcontrollers , popovers, etc.

"view controllers, popovers, etc." (spaces!) Also, each mention of viewController in this comment should probably be "view controller"

>> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:396
>> +    // Clean up any pointers
> 
> Nit - I don't think this comment adds much.

Yep, this is a "what" comment, not a "why" comment, so it should go away.
Comment 42 Brad Wright 2017-03-02 18:21:07 PST
Created attachment 303282 [details]
Patch
Comment 43 Wenson Hsieh 2017-03-02 18:33:09 PST
Comment on attachment 303282 [details]
Patch

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

Please account for Tim's review feedback -- the rest of the patch looks good!

> Source/WebKit2/ChangeLog:8
> +        * UIProcess/ios/forms/WKFileUploadPanel.mm:

As Tim suggested earlier, you should include a couple of sentences here describing what the problem was, and how this change addresses the problem. You can refer to https://trac.webkit.org/changeset/213304 for a great example of what a ChangeLog should look like.

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // Dismiss any viewController that is being presented. This works for all types of viewcontrollers , popovers, etc.

As Tim suggested earlier, please replace "viewController" with "view controller". There is also an extra space before ", popovers" that you should remove.
Comment 44 Brad Wright 2017-03-03 08:16:54 PST
Created attachment 303318 [details]
Patch
Comment 45 Wenson Hsieh 2017-03-03 08:27:21 PST
Comment on attachment 303318 [details]
Patch

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

Just minor nits and ChangeLog formatting issues -- the rest of the patch looks good.

> Source/WebKit2/ChangeLog:6
> +        The problem happens on an iPhone with a WKWebView inside a view controller presented modally. If the user selects an HTML file input 

Please move this description down below the "Reviewed by" line. The ordering of things should be:

```
<Title of the bug>
<bug URL>
<rdar link, if any>

Reviewed by <reviewer name>

<description of problem and how the change fixes it>

<list of individial methods changed, with inline comments if needed>
```

You may refer to an example, such as https://trac.webkit.org/changeset/213348/trunk/LayoutTests/ChangeLog, for what a ChangeLog entry should look like.

> Source/WebKit2/ChangeLog:13
> +        https://bugs.webkit.org/show_bug.cgi?id=165225

Please remove this duplicated bug URL

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:391
> +    // Dismiss any viewController that is being presented. This works for all types of view controllers, popovers, etc.

As Tim suggested earlier, please replace "viewController" with "view controller".

> Source/WebKit2/UIProcess/ios/forms/WKFileUploadPanel.mm:392
> +    // If there is any kind of viewController presented on this view, it will be removed. 

Please replace "viewController" with "view controller" here too.
Comment 46 Build Bot 2017-03-03 09:19:28 PST
Comment on attachment 303318 [details]
Patch

Attachment 303318 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3230152

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html
Comment 47 Build Bot 2017-03-03 09:19:32 PST
Created attachment 303325 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 48 Brad Wright 2017-03-06 09:22:40 PST
These changes are iOS only.  There is no impact to the layout of HTML.
Comment 49 Brad Wright 2017-03-07 09:42:59 PST
Created attachment 303666 [details]
Patch
Comment 50 Brad Wright 2017-03-07 09:48:59 PST
Created attachment 303668 [details]
Patch
Comment 51 Wenson Hsieh 2017-03-07 09:50:03 PST
Comment on attachment 303668 [details]
Patch

r=me (and Tim Horton)
Comment 52 Wenson Hsieh 2017-03-07 10:25:11 PST
Comment on attachment 303668 [details]
Patch

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

On a second look, I noticed the ChangeLog is still formatted strangely. The description should be after the "Reviewed by" line with a newline in between. Please address this issue!

> Source/WebKit2/ChangeLog:13
> +        Reviewed by NOBODY (OOPS!).

The description goes after the "Reviewed by" line.
Comment 53 Brad Wright 2017-03-07 10:44:14 PST
Created attachment 303676 [details]
Patch
Comment 54 WebKit Commit Bot 2017-03-08 01:15:13 PST
Comment on attachment 303676 [details]
Patch

Clearing flags on attachment: 303676

Committed r213570: <http://trac.webkit.org/changeset/213570>
Comment 55 WebKit Commit Bot 2017-03-08 01:15:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 56 Zach Waugh 2018-05-03 09:03:09 PDT
This issue is still present as of iOS 11.3. Choosing a file from within a WKWebView presented in a modal will show the iOS 11 file browser modal. Selecting a file or choosing "Cancel" from from the file browser modal will dismiss the file browser and the UIViewController underneath it
Comment 57 Brad Wright 2018-05-03 09:23:39 PDT
Hi Zach:

I just tested the file input tag in iOS 11.3, and it passed my test.  The WKWebView does not close when presented modally.  It appears to me that this problem has been fixed.  You may be having a different issue, so please take a look at how I conducted my test.  Here is the relevant HTML that I used for the file input tag:

<div class="contentBody">
<p> Click on this button <input type="file"/></p>
</div> 

(In reply to Zach Waugh from comment #56)
> This issue is still present as of iOS 11.3. Choosing a file from within a
> WKWebView presented in a modal will show the iOS 11 file browser modal.
> Selecting a file or choosing "Cancel" from from the file browser modal will
> dismiss the file browser and the UIViewController underneath it
Comment 58 Wenson Hsieh 2018-05-03 09:31:19 PDT
(In reply to Zach Waugh from comment #56)
> This issue is still present as of iOS 11.3. Choosing a file from within a
> WKWebView presented in a modal will show the iOS 11 file browser modal.
> Selecting a file or choosing "Cancel" from from the file browser modal will
> dismiss the file browser and the UIViewController underneath it

Hi Zach — thanks for letting us know! Would you mind opening a new bug for this issue, with a test case? The original repro case (attached here by Brad) seems fixed...
Comment 59 Zach Waugh 2018-05-03 10:01:18 PDT
Thanks for the quick response. I've filed a new bug here: https://bugs.webkit.org/show_bug.cgi?id=185257