Cancel dialogs for Tabs when they are removed from their TabModels.

Cancel dialogs for Tabs when they are removed from their TabModels.

A WebState attempts to cancel its dialogs in its destructor.  However,
the BVC uninstalls itself as the WebStateDelegate before the WebState
is deallocated, so the WebStateImpl doesn't have access to its
JavaScriptDialogPresenter to handle the cancellation.

This CL adds a call to |-cancelDialogForWebState:| when the delegates
are uninstalled for a Tab to ensure that alerts aren't queued in the
DialogPresenter for WebStates that have been deallocated.

BUG=720778
Review-Url: https://codereview.chromium.org/2926413004
Cr-Commit-Position: refs/heads/master@{#478729}
Committed: https://chromium.googlesource.com/chromium/src/+/496fdd75e503c044babb47801e772c7255da94fa

[kkhorimoto, 2017-06-09 20:28:58]

kkhorimoto@chromium.org changed reviewers:
+ marq@chromium.org

[kkhorimoto, 2017-06-09 23:39:44]

kkhorimoto@chromium.org changed reviewers:
+ rohitrao@chromium.org

[kkhorimoto, 2017-06-09 23:39:44]

+ Rohit, so we can land this faster

[kkhorimoto, 2017-06-09 23:49:15]

kkhorimoto@chromium.org changed reviewers:
+ pkl@chromium.org, sdefresne@chromium.org

[sdefresne, 2017-06-10 16:43:40]

lgtm with a question

https://codereview.chromium.org/2926413004/diff/1/ios/chrome/browser/ui/brows...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2926413004/diff/1/ios/chrome/browser/ui/brows...
ios/chrome/browser/ui/browser_view_controller.mm:4636: [self.dialogPresenter
cancelDialogForWebState:tab.webState];
There is the following DECHK in -cancelDialogForWebState:

  DCHECK_NE(webState, self.presentedDialogWebState);

Reading the code of DialogPresenter, I see nothing that a WebState that is going
to be closed cannot show a dialog. Thus, I think it is possible for that DCHECK
to fail. Should we change that function to support webState ==
self.presentedDialogWebState, or add another function that do support this
state?

[frankzamorano350, 2017-06-10 17:11:58]

frankzamorano350@gmail.com changed reviewers:
+ frankzamorano350@gmail.com

[rohitrao (ping after 24h), 2017-06-12 12:33:34]

lgtm

Seems reasonable to remove the dialogs at the same time that we uninstall the
delegates.

Does this patch apply cleanly on M59?

Do we understand why this broke in M59?

[sdefresne, 2017-06-12 12:41:28]

On 2017/06/12 12:33:34, rohitrao (ping after 24h) wrote:
> lgtm
> 
> Seems reasonable to remove the dialogs at the same time that we uninstall the
> delegates.
> 
> Does this patch apply cleanly on M59?
> 
> Do we understand why this broke in M59?

I think this broke when I landed the CL that changed lifetime of Tab & WebState.
Before that CL, WebState was destroyed after Tab -dealloc was called. Now,
WebState is destroyed before Tab -dealloc. This mean that if objects used to
deregister themselves from CRWWebController in Tab -dealloc (or when another
object was deallocated), then it is possible that they are deregistered after
CRWWebController -close (as Tab uses -webStateDestroyed: method to be informed
of WebState destruction and this callback is called after CRWWebController
-close).

That CL flipping the ownership landed after M58 branched and before M59 branched
(https://codereview.chromium.org/2775623002).

[marq (ping after 24h), 2017-06-12 17:35:25]

lgtm

[kkhorimoto, 2017-06-12 19:05:54]

Sylvain: You're right, there's nothing preventing the currently-presented dialog
from being cancelled.  For example, if a page is showing a dialog, then there's
a renderer crash on that page, the dialog would be canceled and that DCHECK
would be incorrect.  I've updated the CL to allow cancelling the current dialog.

Rohit: Yes, this applies cleanly to ToT and M59 branch.  Sylvain's reasoning on
why this crash was introduced is correct; this occurs because the WebState
doesn't have a delegate at deallocation.

[kkhorimoto, 2017-06-12 19:24:44]

Reverted to patch #1 after offline discussion with Rohit.  Patch #2 will be
uploaded as dependent CL.

[kkhorimoto, 2017-06-12 19:24:52]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-06-12 19:24:53]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org,
sdefresne@chromium.org, rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/2926413004/#ps40001
(title: "Revert to patch #1; DCHECK fix is non-essential and will be in
follow-up CL")

[commit-bot: I haz the power, 2017-06-12 19:25:39]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rohitrao (ping after 24h), 2017-06-12 19:26:00]

There's a substantial amount of additional code in patchset #2 -- I'm wary of
landing something that large directly on a post-stable branch.

From chatting with Kurt, it sounds like the issue that Sylvain raised (closing a
WebState that's presenting a dialog) was present in M58 as well.  We think it's
an uncommon edge case that probably leaves the UI in an inconsistent state.

If it was possible to enter this inconsistent state in M58 as well, I'd like to
land Kurt's original CL, with just the crash fix, and cherry-pick that to M59. 
Then we can land patchset #2 as a followup fix, on trunk.

Sylvain, if we land the original patchset, do you agree that this will put us in
basically the same situation as M58?  Do you think there's a greater chance of
having issues in M59 if we only land the original CL?

[commit-bot: I haz the power, 2017-06-12 19:56:34]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497295492584790,
"parent_rev": "5b8d6939094b8aa2f96be68ebf5ebc253869376d", "commit_rev":
"496fdd75e503c044babb47801e772c7255da94fa"}

[commit-bot: I haz the power, 2017-06-12 19:56:48]

Description was changed from

==========
Cancel dialogs for Tabs when they are removed from their TabModels.

A WebState attempts to cancel its dialogs in its destructor.  However,
the BVC uninstalls itself as the WebStateDelegate before the WebState
is deallocated, so the WebStateImpl doesn't have access to its
JavaScriptDialogPresenter to handle the cancellation.

This CL adds a call to |-cancelDialogForWebState:| when the delegates
are uninstalled for a Tab to ensure that alerts aren't queued in the
DialogPresenter for WebStates that have been deallocated.

BUG=720778
==========

to

==========
Cancel dialogs for Tabs when they are removed from their TabModels.

A WebState attempts to cancel its dialogs in its destructor.  However,
the BVC uninstalls itself as the WebStateDelegate before the WebState
is deallocated, so the WebStateImpl doesn't have access to its
JavaScriptDialogPresenter to handle the cancellation.

This CL adds a call to |-cancelDialogForWebState:| when the delegates
are uninstalled for a Tab to ensure that alerts aren't queued in the
DialogPresenter for WebStates that have been deallocated.

BUG=720778

Review-Url: https://codereview.chromium.org/2926413004
Cr-Commit-Position: refs/heads/master@{#478729}
Committed:
https://chromium.googlesource.com/chromium/src/+/496fdd75e503c044babb47801e77...
==========

[commit-bot: I haz the power, 2017-06-12 19:56:49]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/496fdd75e503c044babb47801e77...

[sdefresne, 2017-06-13 09:57:19]

I agree with rohitrao that we should only land patchset #1 on M59 (to get back
to the state that M58 was).