rohitrao@ could you PTAL?

srikanthg: What about with earlier version(s) of iOS? Does the problem occur with M68 and iOS earlier than 11.4.1?

I am to reproduce on iOS11.4, 11.4.1
Not on other os versions.

Something is calling dismissViewControllerAnimated when BVC isn't presenting anything.  This causes BVC to dismiss itself.  On M69+, this would drop you into the tab switcher, which is a recoverable state, but on M68 it destroys all our visible UI instead.

Last time we had a bug like this, it was due to a bug in WebKit on iOS 10.  This is likely to be a similar OS bug, but we should be able to detect when this happens and add a workaround.

+marq if you follow the steps in the original report, you'll end up in the tab grid.  I'm working on figuring out why that happens.

But once you're in this state, you can't exit the tab grid unless you first close all tabs.  Tapping on a tab or opening a new tab updates the tiles, but doesn't exit the grid.  Do you know why that might be happening?

Friendly ping. Stable cut is next Thursday.

I looked at this for a bit last week and I couldn't figure out why this is happening.  Something in UIKit is dismissing the BVC, but that call doesn't appear to be going through dismissViewControllerAnimated:completion:, so we're not able to intercept it.  I can't put in a workaround if I don't understand how the BVC is being dismissed in the first place.

I am not optimistic that I will have a fix for M68.

In M69, we drop users into the tab grid, rather than the splash screen, but the tab grid is currently unresponsive in this state.  That seems fixable for M69.

Moving to M69 because I don't have a fix for M68.

 Issue 869849  has been merged into this issue.

Hi Rohit, any update here?

Found it!  This is actually the same bug as Issue 811671, but now we've introduced a BrowserContainerViewController, which is what ends up presenting the WK file upload dialogs.  Since BrowserContainerViewController is now handling the presentation, my previous fixes in BVC no longer work.

I can fix this for M69 by copying the BVC fix into BrowserContainerViewController.  I believe it will be safe.  The underlying WK bug has been fixed in iOS12.

Ok great! Request merge when ready.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e0162d3250b262731079d8ab7297b9b7be1682a

commit 4e0162d3250b262731079d8ab7297b9b7be1682a
Author: Rohit Rao <rohitrao@chromium.org>
Date: Tue Aug 14 13:30:52 2018

[ios] Fixes an issue where BVC is overdismissed by a bug in WebKit.

The WKFileUploadPanel has a bug in iOS 11 and earlier which causes view
controllers to be overdismissed when the HTML file picker is
used. WebKit attempts to dismiss the file picker view controller, but
due to a bug inadvertently dismisses its presenting view controller
instead.

We previously attempted to work around this bug in
BrowserViewController, but now the file picker is presented on top of
BrowserContainerViewController instead. This CL copies the BVC fix into
BrowserContainerViewController as well.

BUG= 852367 

Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I8c3ef4b55cc052f832aa3e9e995c20880a96db00
Reviewed-on: https://chromium-review.googlesource.com/1172505
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582899}
[modify] https://crrev.com/4e0162d3250b262731079d8ab7297b9b7be1682a/ios/chrome/browser/ui/browser_container/browser_container_view_controller.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a27b8d3c84490a24cf5a9f905fd00782ec00899e

commit a27b8d3c84490a24cf5a9f905fd00782ec00899e
Author: Rohit Rao <rohitrao@chromium.org>
Date: Tue Aug 14 13:34:18 2018

Revert "[ios] Fixes an issue where BVC is overdismissed by a bug in WebKit."

This reverts commit 4e0162d3250b262731079d8ab7297b9b7be1682a.

Reason for revert: Logic in the conditional was wrong.  Reverting and relanding to make cherrypicking easier.

Original change's description:
> [ios] Fixes an issue where BVC is overdismissed by a bug in WebKit.
> 
> The WKFileUploadPanel has a bug in iOS 11 and earlier which causes view
> controllers to be overdismissed when the HTML file picker is
> used. WebKit attempts to dismiss the file picker view controller, but
> due to a bug inadvertently dismisses its presenting view controller
> instead.
> 
> We previously attempted to work around this bug in
> BrowserViewController, but now the file picker is presented on top of
> BrowserContainerViewController instead. This CL copies the BVC fix into
> BrowserContainerViewController as well.
> 
> BUG= 852367 
> 
> Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
> Change-Id: I8c3ef4b55cc052f832aa3e9e995c20880a96db00
> Reviewed-on: https://chromium-review.googlesource.com/1172505
> Reviewed-by: Mark Cogan <marq@chromium.org>
> Commit-Queue: Rohit Rao <rohitrao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#582899}

TBR=rohitrao@chromium.org,marq@chromium.org

Change-Id: Id199b64138930f430e5b753d00e78cc1ecab54a1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  852367 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/1174551
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582900}
[modify] https://crrev.com/a27b8d3c84490a24cf5a9f905fd00782ec00899e/ios/chrome/browser/ui/browser_container/browser_container_view_controller.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee72b70f154b32a30338dc9ed02dc3a638210f26

commit ee72b70f154b32a30338dc9ed02dc3a638210f26
Author: Rohit Rao <rohitrao@chromium.org>
Date: Tue Aug 14 15:30:36 2018

[ios] Fixes an issue where BVC is overdismissed by a bug in WebKit.

The WKFileUploadPanel has a bug in iOS 11 and earlier which causes view
controllers to be overdismissed when the HTML file picker is
used. WebKit attempts to dismiss the file picker view controller, but
due to a bug inadvertently dismisses its presenting view controller
instead.

We previously attempted to work around this bug in
BrowserViewController, but now the file picker is presented on top of
BrowserContainerViewController instead. This CL copies the BVC fix into
BrowserContainerViewController as well.

BUG= 852367 

Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I61651810ae9e06caee27a4c441942e4c4d09c71c
Reviewed-on: https://chromium-review.googlesource.com/1174611
Reviewed-by: Mark Cogan <marq@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582918}
[modify] https://crrev.com/ee72b70f154b32a30338dc9ed02dc3a638210f26/ios/chrome/browser/ui/browser_container/browser_container_view_controller.mm

Requesting merge for https://chromium-review.googlesource.com/1174611.  Please ignore the other CLs on this bug.

Still need to verify on tomorrow's canary.

This bug requires manual review: Less than 17 days to go before AppStore submit on M69
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

 Issue 874473  has been merged into this issue.

Canary verification please.

Verified on M70.0.3528.0 canary
iOS: 12.0 beta#8, iPhoneX.

How confident are you in this fix? Is this fix behind a flag?

It is not behind a flag.

The fix is a copy of a fix that has already been shipped, just moved to a different location.  I am pretty sure (but not 100% confident) that anytime the fix is invoked, the app would've become unusable without the fix.

If the fix fires when it shouldn't, we would end up with undismissable UI on the screen.  (As an example, maybe you'd get stuck looking at Bookmarks, unable to close the bookmarks list.)

The fix itself is overriding -[BrowserContainerViewController dismissViewControllerAnimated:completion:] to do nothing if there is no presented view controller.  I believe that if this method were ever invoked without a presented view controller, then BCVC would dismiss itself instead, which would break the app 100% of the time.

Per this section "If the fix fires when it shouldn't, we would end up with undismissable UI on the screen.  (As an example, maybe you'd get stuck looking at Bookmarks, unable to close the bookmarks list.)"

How often would you expect the fix to fire when it shouldn't? It seems that with or without the fix the user could end up in an unusable state? Why is this important to merge to M69?

Why it's important to merge: The file select dialog is effectively broken on iOS 11.4.  If you try to use it, you will put the app into an unrecoverable state.

Why it's ok to leave out: This is broken on M68 as well.

Risk: This fix is basically going to ship to users with minimal testing, since it's a late merge.

- I'm 100% confident that if BCVC ever dismissed itself, the app would break.
- The potential risk is in cases where BCVC.presentedViewController is nil, but somehow calling dismiss would dismiss some other VC.  I don't know how this could happen; I don't know of any specific cases where it might.
- Not very many objects in our app know that BCVC exists, so we are never intentionally calling dismiss on it directly.

I think the risk here is small, but I don't have a way to quantify it.  If something broke, it would be a case we didn't know about.  The effect of leaving the bug in is clear, in that it causes the app to become unusable if you ever try to use the file picker dialog.

+marq if he has any additional thoughts.

marq and I did some tests with VC presentation:

"If VC A contains B, and then A presents VC C, then A and B both have C as their presentedViewController.
And thus calling [B dismiss:C] works.
(e.g, C is dismissed)"

To summarize, we think this is low risk for M69.  The "potential risk" I outlined above should not be possible because children VCs inherit the presentedVC of their parent.  We think the bug is severe enough that it's worth merging to 69, even if it's currently broken in 68.

Thanks Rohit for the detailed explanation.

+ecabrera For M68 have you seen any user reports for this issue? Where a user goes to select a file, but instead gets the chrome splash screen?

Hi
Checking reports for this I can't find almost any report using those keywords. the only one I was able to find are these two

Example 
https://listnr.corp.google.com/report/85599160252
https://listnr.corp.google.com/report/85608870123

I would feel much more comfortable to merge if this fix was behind a flag. Is that possible?

At this point I think it's too late to take this for M69.  The fix is already in M70.

Please let me know if you disagree.  I'll keep this as-is for the day and then flip it to M70 if there are no objections.

No disagreement. The problem is in M68 and we have not heard any user feedback about this.

SGTM. I'm okay not merging this for same reasons as Peter.

Issue 891278 has been merged into this issue.