commit 7f4eeb1dc31b90ffd906e50c6c9f1cbc3cec8a50
author Erik Chen <erikchen@chromium.org> Sat Sep 07 00:20:59 2024
committer Chromium LUCI CQ <chromium-scoped@luci-project-accounts.iam.gserviceaccount.com> Sat Sep 07 00:20:59 2024
tree 33b952049512c9ca56af9a0089a677c0a4ffb45a
parent f06a82b800906f5356766ddad13d0c549308c239

Fix DCHECK failure in AccountSelectionModalView.

Prior to this CL, AccountSelectionModalView created a dialog widget with
constrained_window::ShowWebModalDialogViews. This method attaches a
helper that attempts to automatically show/hide the widget when the tab
becomes foreground/background.

This has two problems:

(1) There are some known edge cases with
constrained_window::ShowWebModalDialogViews, such as crbug.com/353174863

(2) The show/hide logic in constrained_window::ShowWebModalDialogViews
is redundant with, and conflicts with the show/hide logic in
FedCmAccountSelectionView. Theoretically, multiple calls to
Widget::Show() or Widget::Hide() should be idempotent, but the
implementation of constrained_window::ShowWebModalDialogViews breaks
invariants in aura::Window. constrained_window::ShowWebModalDialogViews
adds an animation to hiding/showing windows via
wm::WINDOW_VISIBILITY_ANIMATION_TYPE_ROTATE. The implementation of this
in AddLayerAnimationsForRotate() sets the window's visibility to true,
but also sets the ui::Layer's target opacity to 0. This breaks the
invariant in aura::Window that a visible window should have non-zero
target opacity. This invariant is only checked in Widget::Show(), and
thus this is not normally triggered (although this is really just a
latent race condition). The second call to Widget::Show() from
FedCmAccountSelectionView deterministically triggers this pre-existing
bug.

This CL fixes the problem by removing the call to
constrained_window::ShowWebModalDialogViews and showing a DialogDelegate
via a normal widget. The main purpose of
constrained_window::ShowWebModalDialogViews is to show/hide the dialog
when the tab shows/hides, but that is already implemented by
FedCmAccountSelectionView.

This CL does not touch the logic for positioning the modal dialog, which
is still done by
constrained_window::UpdateWebContentsModalDialogPosition. This is
unusual but does not cause any problems.

Bug: 364667534, 353174863
Change-Id: Iffed9b4a640ece73801ab1e00b94133ffc8e1052
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5841351
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1352378}