Open a PWA or bookmark app in tablet mode, browser crash is observed |
|||||||||||||||
Issue descriptionChrome OS 10575.12.0, 67.0.3396.16 dev channel eve, nautilus devices What steps will reproduce the problem? (1) Install a PWA app or bookmark app (e.g., https://permission.site, Google Santa Tracker). (2) Switch to tablet mode (3) Open the same PWA/ bookmark app What happens instead? Repeated browser crash is happening. Crash report: https://crash.corp.google.com/browse?stbtiq=b23e52e55b0c7166
,
Apr 24 2018
Device logs in this link: https://pantheon.corp.google.com/storage/browser/chromiumos-test-logs/bugfiles/cr/836482/
,
Apr 25 2018
,
Apr 26 2018
,
Apr 26 2018
I'm guessing this happens because FadeIn is called when there's no layer. Best to just null check there for a merge-able fix. Long term we need to figure out a better way than disabling layer animations to get the buttons working.
,
Apr 26 2018
Issue 837086 has been merged into this issue.
,
Apr 26 2018
Looks like we start in immersive mode when in tablet mode so we never get the OnImmersiveRevealStarted() call to disable the fade in timer.
,
Apr 26 2018
^ That's incorrect. OnImmersiveRevealStarted() gets called before StartTitlebarAnimation() meaning the fade in timer is started after it's cancelled.
,
Apr 26 2018
,
Apr 27 2018
,
Apr 27 2018
Issue 837651 has been merged into this issue.
,
May 2 2018
See this happening in M67 Beta. Once fix has been identified, please provide test evidence and request merge for M67.
,
May 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cdd1046247720cc268f3fe8ad1ca57066b9fbd93 commit cdd1046247720cc268f3fe8ad1ca57066b9fbd93 Author: Alan Cutter <alancutter@chromium.org> Date: Sat May 05 11:24:52 2018 Fix hosted app crash when started in tablet mode The code was written with the expectation that HostedAppButtonContainer::StartTitlebarAnimation() is called before HostedAppButtonContainer::OnImmersiveRevealStarted(). This is not the case when in tablet mode and results in crashes. This CL updates HostedAppButtonContainer to allow these methods to be called in either order. Bug: 836482 Change-Id: I658578ca79599e21566fffd3986a6dc852cb6489 Reviewed-on: https://chromium-review.googlesource.com/1029882 Commit-Queue: Alan Cutter <alancutter@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Cr-Commit-Position: refs/heads/master@{#556327} [modify] https://crrev.com/cdd1046247720cc268f3fe8ad1ca57066b9fbd93/ash/public/cpp/immersive/immersive_fullscreen_controller_test_api.cc [modify] https://crrev.com/cdd1046247720cc268f3fe8ad1ca57066b9fbd93/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc [modify] https://crrev.com/cdd1046247720cc268f3fe8ad1ca57066b9fbd93/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h [modify] https://crrev.com/cdd1046247720cc268f3fe8ad1ca57066b9fbd93/chrome/browser/ui/views/frame/hosted_app_button_container.cc [modify] https://crrev.com/cdd1046247720cc268f3fe8ad1ca57066b9fbd93/chrome/browser/ui/views/frame/hosted_app_button_container.h [modify] https://crrev.com/cdd1046247720cc268f3fe8ad1ca57066b9fbd93/chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc
,
May 5 2018
Requesting merge for crash fix. There have been a few independent reports of people hitting this crash in tablet mode.
,
May 5 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6f3313d87852ec47a4559e990958b572651b0fac commit 6f3313d87852ec47a4559e990958b572651b0fac Author: Erik Chen <erikchen@chromium.org> Date: Sat May 05 14:12:34 2018 Compile errors: https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-dbg/5522 Original change's description: > Fix hosted app crash when started in tablet mode > > The code was written with the expectation that > HostedAppButtonContainer::StartTitlebarAnimation() > is called before > HostedAppButtonContainer::OnImmersiveRevealStarted(). > > This is not the case when in tablet mode and results > in crashes. > > This CL updates HostedAppButtonContainer to allow these > methods to be called in either order. > > Bug: 836482 > Change-Id: I658578ca79599e21566fffd3986a6dc852cb6489 > Reviewed-on: https://chromium-review.googlesource.com/1029882 > Commit-Queue: Alan Cutter <alancutter@chromium.org> > Reviewed-by: Scott Violet <sky@chromium.org> > Reviewed-by: James Cook <jamescook@chromium.org> > Cr-Commit-Position: refs/heads/master@{#556327} TBR=jamescook@chromium.org,sky@chromium.org,alancutter@chromium.org Change-Id: I8adc788a94cb297a86e2e8b6d04e733e8092e3ea No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 836482 Reviewed-on: https://chromium-review.googlesource.com/1045872 Reviewed-by: Erik Chen <erikchen@chromium.org> Commit-Queue: Erik Chen <erikchen@chromium.org> Cr-Commit-Position: refs/heads/master@{#556328} [modify] https://crrev.com/6f3313d87852ec47a4559e990958b572651b0fac/ash/public/cpp/immersive/immersive_fullscreen_controller_test_api.cc [modify] https://crrev.com/6f3313d87852ec47a4559e990958b572651b0fac/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc [modify] https://crrev.com/6f3313d87852ec47a4559e990958b572651b0fac/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h [modify] https://crrev.com/6f3313d87852ec47a4559e990958b572651b0fac/chrome/browser/ui/views/frame/hosted_app_button_container.cc [modify] https://crrev.com/6f3313d87852ec47a4559e990958b572651b0fac/chrome/browser/ui/views/frame/hosted_app_button_container.h [modify] https://crrev.com/6f3313d87852ec47a4559e990958b572651b0fac/chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc
,
May 6 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 7 2018
Per #14, has this fix been tested?
,
May 8 2018
The fix was reverted, new fix in progress: https://chromium-review.googlesource.com/c/chromium/src/+/1045955 Removing merge request until this has made it to canary, sorry for the noise.
,
May 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/34207ba5a7d39fde67a355c55a576ccd4939e34a commit 34207ba5a7d39fde67a355c55a576ccd4939e34a Author: Alan Cutter <alancutter@chromium.org> Date: Tue May 08 05:16:11 2018 Reland: Fix hosted app crash when started in tablet mode This is a reland of https://chromium-review.googlesource.com/c/chromium/src/+/1029882 with debug linkage fixed. Original description: The code was written with the expectation that HostedAppButtonContainer::StartTitlebarAnimation() is called before HostedAppButtonContainer::OnImmersiveRevealStarted(). This is not the case when in tablet mode and results in crashes. This CL updates HostedAppButtonContainer to allow these methods to be called in either order. Bug: 836482 Change-Id: Ia862c8bfccf448322cb878892e70b38eb68818e4 Reviewed-on: https://chromium-review.googlesource.com/1045955 Commit-Queue: Alan Cutter <alancutter@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#556695} [modify] https://crrev.com/34207ba5a7d39fde67a355c55a576ccd4939e34a/ash/public/cpp/immersive/immersive_fullscreen_controller_test_api.cc [modify] https://crrev.com/34207ba5a7d39fde67a355c55a576ccd4939e34a/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc [modify] https://crrev.com/34207ba5a7d39fde67a355c55a576ccd4939e34a/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h [modify] https://crrev.com/34207ba5a7d39fde67a355c55a576ccd4939e34a/chrome/browser/ui/views/frame/hosted_app_button_container.cc [modify] https://crrev.com/34207ba5a7d39fde67a355c55a576ccd4939e34a/chrome/browser/ui/views/frame/hosted_app_button_container.h [modify] https://crrev.com/34207ba5a7d39fde67a355c55a576ccd4939e34a/chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc
,
May 8 2018
Adding back merge request for 67. New fix has not been verified on Canary. I think it usually takes a few days for Chrome OS Canary to update, so I'd like to get approval-pending-verification so we can merge it as soon as we verify it.
,
May 8 2018
,
May 8 2018
Will provide merge input once testing has completed, per #14. Thanks.
,
May 8 2018
Re #20, since the fixes are in version 68.0.3425.0, I'm waiting for the new build to arrive. Will verify once it's available. Thanks!
,
May 9 2018
#23: Great, thank you!
,
May 9 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 10 2018
For clarity the current merge request is for commit 34207ba5a7d39fde67a355c55a576ccd4939e34a (6 lines of non-test change).
,
May 10 2018
Merge evaluation is pending verification in #23. Both for expected results and ensuring it didn't create any new issues.
,
May 10 2018
I'm not able to reproduce this issue on M68 (10663.0.0, 68.0.3425.0). I think the fix can be merged to M67.
,
May 10 2018
Approving merge to M67 Chrome OS.
,
May 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b0d98ef50326d483de7da24cff44d03d45b0967 commit 3b0d98ef50326d483de7da24cff44d03d45b0967 Author: Alan Cutter <alancutter@chromium.org> Date: Fri May 11 03:47:27 2018 Reland: Fix hosted app crash when started in tablet mode This is a reland of https://chromium-review.googlesource.com/c/chromium/src/+/1029882 with debug linkage fixed. Original description: The code was written with the expectation that HostedAppButtonContainer::StartTitlebarAnimation() is called before HostedAppButtonContainer::OnImmersiveRevealStarted(). This is not the case when in tablet mode and results in crashes. This CL updates HostedAppButtonContainer to allow these methods to be called in either order. TBR=alancutter@chromium.org (cherry picked from commit 34207ba5a7d39fde67a355c55a576ccd4939e34a) Bug: 836482 Change-Id: Ia862c8bfccf448322cb878892e70b38eb68818e4 Reviewed-on: https://chromium-review.googlesource.com/1045955 Commit-Queue: Alan Cutter <alancutter@chromium.org> Reviewed-by: Scott Violet <sky@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#556695} Reviewed-on: https://chromium-review.googlesource.com/1055228 Reviewed-by: Alan Cutter <alancutter@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#564} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/3b0d98ef50326d483de7da24cff44d03d45b0967/ash/public/cpp/immersive/immersive_fullscreen_controller_test_api.cc [modify] https://crrev.com/3b0d98ef50326d483de7da24cff44d03d45b0967/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc [modify] https://crrev.com/3b0d98ef50326d483de7da24cff44d03d45b0967/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h [modify] https://crrev.com/3b0d98ef50326d483de7da24cff44d03d45b0967/chrome/browser/ui/views/frame/hosted_app_button_container.cc [modify] https://crrev.com/3b0d98ef50326d483de7da24cff44d03d45b0967/chrome/browser/ui/views/frame/hosted_app_button_container.h [modify] https://crrev.com/3b0d98ef50326d483de7da24cff44d03d45b0967/chrome/browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc
,
Jun 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0b449f8eed589b50b76bf164aa353b4d36594b2c commit 0b449f8eed589b50b76bf164aa353b4d36594b2c Author: Alan Cutter <alancutter@chromium.org> Date: Thu Jun 07 00:50:29 2018 Remove duplicate definition of kTitlebarAnimationDelay kTitlebarAnimationDelay is defined twice in browser_non_client_frame_view_ash.cc likely due to a bad merge of https://chromium-review.googlesource.com/c/chromium/src/+/1055228. This CL removes the reduntant constant definition. Bug: 836482 Change-Id: I755c84182da0348b5e701bec2f6ad24cb9b8c0b9 Reviewed-on: https://chromium-review.googlesource.com/1088460 Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Alan Cutter <alancutter@chromium.org> Cr-Commit-Position: refs/heads/master@{#565128} [modify] https://crrev.com/0b449f8eed589b50b76bf164aa353b4d36594b2c/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by mkarkada@chromium.org
, Apr 24 2018