New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 908524 link

Starred by 1 user
Status: Fixed
Owner:
xiy...@chromium.org
Closed: Dec 17
Cc:
█ skuhne@chromium.org
msw@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Session restore failing after closing session with a minimized browser window

Project Member Reported by qnnguyen@chromium.org, Nov 26 
Chrome Version: found at 72.0.3616.0
OS: CrOS

What steps will reproduce the problem?
(1) Sign into a user session
(2) Open a browser window (leave it at the NTP)
(3) Minimize the window
(4) Exit the session
(5) Sign into the same user again

What is the expected result?
The session should start up gracefully (I believe also restore the window that was minimized).

What happens instead?
DCHECK failed. See error here: https://paste.googleplex.com/6446759211433984

Video:
https://drive.google.com/open?id=1xX5fy-B6IwP5il6KAULuHFHge9Du4sDd

Interestingly, this crash doesn't reproduce if the minimized page is something other than the new-tab page. The crash happens somewhere while attempting to focus the Omnibox, so there might be something with the URL view.

Assigning to derat@ for now since this seems ash-related, please move ownership as appropriate. Thanks!

Comment 1 by derat@chromium.org, Nov 26

Cc: skuhne@chromium.org
Labels: -Pri-3 Pri-1
Owner: osh...@chromium.org

Comment 2 by osh...@chromium.org, Nov 27

Owner: msw@chromium.org
WorkspaceLayoutManager unminmize the window before being actually activated after this CL:


https://chromium-review.googlesource.com/c/chromium/src/+/688166/7/ash/wm/workspace/workspace_layout_manager.cc


which causes inconsistency when the window being activated tries to set the focus.

It looks to me that CL isn't safe for above reason, although I don't know about original issue that CL was tring to fix.

Comment 3 by osh...@chromium.org, Nov 27

Cc: xiy...@chromium.org
+xiyuan@ who is also looking into session restore issue upon login

Comment 4 by osh...@chromium.org, Nov 27 

Forgot to mention. Revering "Activated->Activating" change did fix this particular issue.

Comment 5 by xiy...@chromium.org, Nov 27

Cc: -xiy...@chromium.org msw@chromium.org
Owner: xiy...@chromium.org
Think my CL (https://chromium-review.googlesource.com/c/chromium/src/+/1313728) that changes BrowserView::Show to defer the default focus change until the browser frame widget is activated should fix this.

The CL is current blocked on a test failure, working on it.

Comment 6 by omrilio@chromium.org, Nov 30

Components: -UI>Shell
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 6 

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

commit 2a66b11ee57844b24718b518f170706bc9242eab
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Thu Dec 06 15:52:16 2018

cros: Fix startup page focus

- Move RestoreFocus from BrowserView::Show to OnWidgetActivationChanged
  to do it when the browser window is activated;
- Remove SetInitialFocus in SessionRestoreImpl::ShowBrowser since
  BrowserView change should do that as part of RestoreFocus;
- Also remove the debugging code for SetInitialFocus crash
  under SessionRestoreImpl::RestoreTab since ShowBrowser no longer
  calls SetInitialFocus;

Bug:  859257 , 850626,  908524 
Change-Id: Ia24e4d68e1386de84765f914bb75319c9887dfd1
Reviewed-on: https://chromium-review.googlesource.com/c/1313728
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614364}
[modify] https://crrev.com/2a66b11ee57844b24718b518f170706bc9242eab/chrome/browser/sessions/session_restore.cc
[modify] https://crrev.com/2a66b11ee57844b24718b518f170706bc9242eab/chrome/browser/ui/startup/startup_browser_creator_interactive_uitest.cc
[modify] https://crrev.com/2a66b11ee57844b24718b518f170706bc9242eab/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/2a66b11ee57844b24718b518f170706bc9242eab/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/2a66b11ee57844b24718b518f170706bc9242eab/chrome/browser/ui/views/frame/browser_view_interactive_uitest.cc
[modify] https://crrev.com/2a66b11ee57844b24718b518f170706bc9242eab/chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
[modify] https://crrev.com/2a66b11ee57844b24718b518f170706bc9242eab/chrome/browser/ui/views/toolbar/toolbar_view_interactive_uitest.cc
[modify] https://crrev.com/2a66b11ee57844b24718b518f170706bc9242eab/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/2a66b11ee57844b24718b518f170706bc9242eab/content/browser/web_contents/web_contents_impl.h

Comment 8 by grt@chromium.org, Dec 11 

I've noticed on Win canary that a freshly restored window (from being minimized) often doesn't have keyboard focus. I have to explicitly click in the window somewhere in order for things like Ctrl-Tab to work. Could this be related? (I'm using 73.0.3636.0 (Official Build) canary (64-bit) (cohort: Clang-64) at the moment.) Should I open a new bug?

Comment 9 by xiy...@chromium.org, Dec 11 

Re #8: Please file a new bug. CL in #7 could be related. Could you a version without it (prior 73.0.3633.0) ?

Comment 10 by grt@chromium.org, Dec 13 

I've bisected and found that the regression was introduced before this CL. I've filed  issue 914753 . Thanks.
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 13 

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

commit ecde845099001740fee119a82d45664e622d0ade
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Thu Dec 13 17:17:50 2018

wm: Update nested activation handling

- NOTREACHED() and no-ope if the nested activation wants to change
  to a different active window other than the one being activated;
- Allow focus shift in nested activations as long as they don't
  change the window to be activated

Bug:  908524 
Test: FocusControllerApiTest.ActivateWhileActivating
Change-Id: I105c85bf2671c7e396fdcbedff7b8082f007f642
Reviewed-on: https://chromium-review.googlesource.com/c/1357017
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Mitsuru Oshima (gardener - slow) <oshima@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616346}
[modify] https://crrev.com/ecde845099001740fee119a82d45664e622d0ade/ui/wm/core/focus_controller.cc
[modify] https://crrev.com/ecde845099001740fee119a82d45664e622d0ade/ui/wm/core/focus_controller.h
[modify] https://crrev.com/ecde845099001740fee119a82d45664e622d0ade/ui/wm/core/focus_controller_unittest.cc

Comment 12 by xiy...@chromium.org, Dec 13

Status: Fixed (was: Untriaged)
Project Member

Comment 13 by bugdroid1@chromium.org, Dec 13 

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

commit 20eddc260df5448dce01c53adebe3a7485763db2
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Thu Dec 13 18:51:16 2018

Revert "wm: Update nested activation handling"

This reverts commit ecde845099001740fee119a82d45664e622d0ade.

Reason for revert:
EXPECT_DEATH_IF_SUPPORTED not working in NDEBUG without DCHECK.

Original change's description:
> wm: Update nested activation handling
> 
> - NOTREACHED() and no-ope if the nested activation wants to change
>   to a different active window other than the one being activated;
> - Allow focus shift in nested activations as long as they don't
>   change the window to be activated
> 
> Bug:  908524 
> Test: FocusControllerApiTest.ActivateWhileActivating
> Change-Id: I105c85bf2671c7e396fdcbedff7b8082f007f642
> Reviewed-on: https://chromium-review.googlesource.com/c/1357017
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Mitsuru Oshima (gardener - slow) <oshima@chromium.org>
> Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616346}

TBR=xiyuan@chromium.org,sky@chromium.org,oshima@chromium.org

Change-Id: I8fd73d291c7a6774b1f48049d618a232340e936e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  908524 
Reviewed-on: https://chromium-review.googlesource.com/c/1376137
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616375}
[modify] https://crrev.com/20eddc260df5448dce01c53adebe3a7485763db2/ui/wm/core/focus_controller.cc
[modify] https://crrev.com/20eddc260df5448dce01c53adebe3a7485763db2/ui/wm/core/focus_controller.h
[modify] https://crrev.com/20eddc260df5448dce01c53adebe3a7485763db2/ui/wm/core/focus_controller_unittest.cc

Comment 14 by xiy...@chromium.org, Dec 13

Status: Started (was: Fixed)
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 13 

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

commit 77fbe34018de545f5b733709a2afcb1391ef1364
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Thu Dec 13 23:44:02 2018

Reland "wm: Update nested activation handling"

This is a reland of ecde845099001740fee119a82d45664e622d0ade

Original change's description:
> wm: Update nested activation handling
>
> - NOTREACHED() and no-ope if the nested activation wants to change
>   to a different active window other than the one being activated;
> - Allow focus shift in nested activations as long as they don't
>   change the window to be activated
>
> Bug:  908524 
> Test: FocusControllerApiTest.ActivateWhileActivating
> Change-Id: I105c85bf2671c7e396fdcbedff7b8082f007f642
> Reviewed-on: https://chromium-review.googlesource.com/c/1357017
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Mitsuru Oshima (gardener - slow) <oshima@chromium.org>
> Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616346}

Bug:  908524 ,  914915 
Change-Id: Id0072941bfb0def54f093bcb771b2d8485c9b04e
Reviewed-on: https://chromium-review.googlesource.com/c/1376726
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616496}
[modify] https://crrev.com/77fbe34018de545f5b733709a2afcb1391ef1364/ui/wm/core/focus_controller.cc
[modify] https://crrev.com/77fbe34018de545f5b733709a2afcb1391ef1364/ui/wm/core/focus_controller.h
[modify] https://crrev.com/77fbe34018de545f5b733709a2afcb1391ef1364/ui/wm/core/focus_controller_unittest.cc
Project Member

Comment 16 by bugdroid1@chromium.org, Dec 14 

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

commit 7762ed06e63d2622c869d179ecfb8674441711e8
Author: Hitoshi Yoshida <peria@chromium.org>
Date: Fri Dec 14 01:39:56 2018

Revert "Reland "wm: Update nested activation handling""

This reverts commit 77fbe34018de545f5b733709a2afcb1391ef1364.

Reason for revert: A test still fails with this.
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20TSan%20Tests/31030

Original change's description:
> Reland "wm: Update nested activation handling"
> 
> This is a reland of ecde845099001740fee119a82d45664e622d0ade
> 
> Original change's description:
> > wm: Update nested activation handling
> >
> > - NOTREACHED() and no-ope if the nested activation wants to change
> >   to a different active window other than the one being activated;
> > - Allow focus shift in nested activations as long as they don't
> >   change the window to be activated
> >
> > Bug:  908524 
> > Test: FocusControllerApiTest.ActivateWhileActivating
> > Change-Id: I105c85bf2671c7e396fdcbedff7b8082f007f642
> > Reviewed-on: https://chromium-review.googlesource.com/c/1357017
> > Reviewed-by: Scott Violet <sky@chromium.org>
> > Reviewed-by: Mitsuru Oshima (gardener - slow) <oshima@chromium.org>
> > Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#616346}
> 
> Bug:  908524 ,  914915 
> Change-Id: Id0072941bfb0def54f093bcb771b2d8485c9b04e
> Reviewed-on: https://chromium-review.googlesource.com/c/1376726
> Reviewed-by: Scott Violet <sky@chromium.org>
> Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616496}

TBR=xiyuan@chromium.org,sky@chromium.org,oshima@chromium.org

Change-Id: I8fd98eca46991af0cd6a780f44f7912c5d6bad13
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  908524 ,  914915 
Reviewed-on: https://chromium-review.googlesource.com/c/1377477
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Commit-Queue: Hitoshi Yoshida <peria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616552}
[modify] https://crrev.com/7762ed06e63d2622c869d179ecfb8674441711e8/ui/wm/core/focus_controller.cc
[modify] https://crrev.com/7762ed06e63d2622c869d179ecfb8674441711e8/ui/wm/core/focus_controller.h
[modify] https://crrev.com/7762ed06e63d2622c869d179ecfb8674441711e8/ui/wm/core/focus_controller_unittest.cc
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 14 

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

commit fd8ad21305da505a82ed7ab6dc3c49738eaa802e
Author: Xiyuan Xia <xiyuan@chromium.org>
Date: Fri Dec 14 18:27:28 2018

Reland "Reland "wm: Update nested activation handling""

This is a reland of 77fbe34018de545f5b733709a2afcb1391ef1364
with fix for the UAF.

Original change's description:
> Reland "wm: Update nested activation handling"
>
> This is a reland of ecde845099001740fee119a82d45664e622d0ade
>
> Original change's description:
> > wm: Update nested activation handling
> >
> > - DCHECK() and no-op if the nested activation wants to change
> >   to a different active window other than the one being activated;
> > - Allow focus shift in nested activations as long as they don't
> >   change the window to be activated
> >
> > Bug:  908524 
> > Test: FocusControllerApiTest.ActivateWhileActivating
> > Change-Id: I105c85bf2671c7e396fdcbedff7b8082f007f642
> > Reviewed-on: https://chromium-review.googlesource.com/c/1357017
> > Reviewed-by: Scott Violet <sky@chromium.org>
> > Reviewed-by: Mitsuru Oshima (gardener - slow) <oshima@chromium.org>
> > Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#616346}
>
> Bug:  908524 ,  914915 
> Change-Id: Id0072941bfb0def54f093bcb771b2d8485c9b04e
> Reviewed-on: https://chromium-review.googlesource.com/c/1376726
> Reviewed-by: Scott Violet <sky@chromium.org>
> Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#616496}

Bug:  908524 ,  914915 
Change-Id: Id3786796d109cfdb7c4caf1529243af93830da69
Reviewed-on: https://chromium-review.googlesource.com/c/1378236
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616751}
[modify] https://crrev.com/fd8ad21305da505a82ed7ab6dc3c49738eaa802e/ui/wm/core/focus_controller.cc
[modify] https://crrev.com/fd8ad21305da505a82ed7ab6dc3c49738eaa802e/ui/wm/core/focus_controller.h
[modify] https://crrev.com/fd8ad21305da505a82ed7ab6dc3c49738eaa802e/ui/wm/core/focus_controller_unittest.cc

Comment 18 by xiy...@chromium.org, Dec 17

Status: Fixed (was: Started)
►

Sign in to add a comment