[Navi] Browser crashes when using browser-back to get to google apps module |
||||||||||
Issue descriptionrepro: 1) go through onboarding flow 2) once you get PAST google apps module, navigate away (or wait until you get to chrome://welcome/email-interstitial, which is actually a whole different page) 3) hit browser-back button until you're supposed to land on google-apps observer that your browser crashes.
,
Nov 29
,
Nov 29
Not sure if related, but also experience same crash when: 1) On through onboarding flow past "Save Your Progress" screen by clicking continue 2) hit browser-back to land on "Save Your Progress" 3) Navigate anywhere (to another URL, back button, continue, skip) observe browser crashes
,
Dec 4
,
Dec 5
Issue 911869 has been merged into this issue.
,
Dec 5
Updating the regression info and labelling. Adding an RB label for tracking purpose. Feel free to remove if needed. ------------------------------------- Manual regression range finder link -------------------------------------- https://crash.corp.google.com/browse?q=expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27nux%3A%3ABookmarkHandler%3A%3AHandleToggleBookmarkBar%27+AND+expanded_custom_data.ChromeCrashProto.ptype%3D%27browser%27#-property-selector,-samplereports,+productname,+productversion:1000,+directory,-clientid,+operatingsystem,+url,+simplifiedurl,+extensions
,
Dec 5
,
Dec 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6223083496b5fed8d8bb69c38b20d7819ca3be9 commit a6223083496b5fed8d8bb69c38b20d7819ca3be9 Author: Scott Chen <scottchen@chromium.org> Date: Fri Dec 07 04:43:15 2018 Nux Onboarding: fix race-condition crashes There are a couple race-conditions happening in the code that in combination causes 'undefined' to be passed to the C++ handler, which crashes on a CHECK in BookmarkHandler: - in some places toggleBookmarkBar() depends on isBookmarkBarShown() result, without taking into account that isBookmarkBarShown is async. - multiple toggleBookmarkBar() async calls fire consecutively, overriding each other's effects. The above two issues are fixed together by locally cached bookmark visibility state that can be updated and retrieved synchronously (inside of the wrapper BookmarkBarManager). There doesn't seem to be a way for toggling bookmark visibility to fail in the backend. Even if that's possible, the best we can do is to keep using the local cache value until the visibility state becomes eventually consistent, so simply updating this local cache should suffice. After the above fix, another timing issue is exposed: - the navigation behavior's onRouteChange is always called in the order that elements are created. This is problematic since sometimes module's clean-up code run after another module's init code and causing conflicting effects. The above issue is fixed in this CL by adding additional enter and exit hooks to each route, and navigation behavior will trigger them in the expected order. Bug: 908544 Change-Id: If462d2ec955ece0d8b17fc342fc7d1ce0225e72c Reviewed-on: https://chromium-review.googlesource.com/c/1357523 Commit-Queue: Scott Chen <scottchen@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Reviewed-by: Hector Carmona <hcarmona@chromium.org> Cr-Commit-Position: refs/heads/master@{#614604} [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/browser/resources/welcome/onboarding_welcome/BUILD.gn [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/browser/resources/welcome/onboarding_welcome/email/email_chooser.js [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/browser/resources/welcome/onboarding_welcome/email/nux_email.js [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/browser/resources/welcome/onboarding_welcome/google_apps/apps_chooser.js [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/browser/resources/welcome/onboarding_welcome/google_apps/nux_google_apps.js [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/browser/resources/welcome/onboarding_welcome/navigation_behavior.js [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/browser/resources/welcome/onboarding_welcome/shared/bookmark_proxy.js [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/browser/resources/welcome/onboarding_welcome/welcome_app.html [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/browser/resources/welcome/onboarding_welcome/welcome_app.js [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/test/data/webui/welcome/email_chooser_test.js [add] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/test/data/webui/welcome/navigation_behavior_test.js [modify] https://crrev.com/a6223083496b5fed8d8bb69c38b20d7819ca3be9/chrome/test/data/webui/welcome/onboarding_welcome_browsertest.js
,
Dec 7
,
Dec 8
Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc commit 5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc Author: Scott Chen <scottchen@chromium.org> Date: Mon Dec 10 19:58:25 2018 Nux Onboarding: fix race-condition crashes There are a couple race-conditions happening in the code that in combination causes 'undefined' to be passed to the C++ handler, which crashes on a CHECK in BookmarkHandler: - in some places toggleBookmarkBar() depends on isBookmarkBarShown() result, without taking into account that isBookmarkBarShown is async. - multiple toggleBookmarkBar() async calls fire consecutively, overriding each other's effects. The above two issues are fixed together by locally cached bookmark visibility state that can be updated and retrieved synchronously (inside of the wrapper BookmarkBarManager). There doesn't seem to be a way for toggling bookmark visibility to fail in the backend. Even if that's possible, the best we can do is to keep using the local cache value until the visibility state becomes eventually consistent, so simply updating this local cache should suffice. After the above fix, another timing issue is exposed: - the navigation behavior's onRouteChange is always called in the order that elements are created. This is problematic since sometimes module's clean-up code run after another module's init code and causing conflicting effects. The above issue is fixed in this CL by adding additional enter and exit hooks to each route, and navigation behavior will trigger them in the expected order. Bug: 908544 Change-Id: If462d2ec955ece0d8b17fc342fc7d1ce0225e72c Reviewed-on: https://chromium-review.googlesource.com/c/1357523 Commit-Queue: Scott Chen <scottchen@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Reviewed-by: Hector Carmona <hcarmona@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#614604}(cherry picked from commit a6223083496b5fed8d8bb69c38b20d7819ca3be9) Reviewed-on: https://chromium-review.googlesource.com/c/1370402 Reviewed-by: Scott Chen <scottchen@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#230} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/browser/resources/welcome/onboarding_welcome/BUILD.gn [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/browser/resources/welcome/onboarding_welcome/email/email_chooser.js [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/browser/resources/welcome/onboarding_welcome/email/nux_email.js [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/browser/resources/welcome/onboarding_welcome/google_apps/apps_chooser.js [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/browser/resources/welcome/onboarding_welcome/google_apps/nux_google_apps.js [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/browser/resources/welcome/onboarding_welcome/navigation_behavior.js [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/browser/resources/welcome/onboarding_welcome/shared/bookmark_proxy.js [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/browser/resources/welcome/onboarding_welcome/welcome_app.html [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/browser/resources/welcome/onboarding_welcome/welcome_app.js [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/test/data/webui/welcome/email_chooser_test.js [add] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/test/data/webui/welcome/navigation_behavior_test.js [modify] https://crrev.com/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc/chrome/test/data/webui/welcome/onboarding_welcome_browsertest.js
,
Dec 10
,
Dec 11
Not seeing any crashes post 73.0.3634.0 after patch in #8 landed.
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc Commit: 5bde839fbde3e6b5c4e8c7a48a8d321c20cf43dc Author: scottchen@chromium.org Commiter: scottchen@chromium.org Date: 2018-12-10 19:58:25 +0000 UTC Nux Onboarding: fix race-condition crashes There are a couple race-conditions happening in the code that in combination causes 'undefined' to be passed to the C++ handler, which crashes on a CHECK in BookmarkHandler: - in some places toggleBookmarkBar() depends on isBookmarkBarShown() result, without taking into account that isBookmarkBarShown is async. - multiple toggleBookmarkBar() async calls fire consecutively, overriding each other's effects. The above two issues are fixed together by locally cached bookmark visibility state that can be updated and retrieved synchronously (inside of the wrapper BookmarkBarManager). There doesn't seem to be a way for toggling bookmark visibility to fail in the backend. Even if that's possible, the best we can do is to keep using the local cache value until the visibility state becomes eventually consistent, so simply updating this local cache should suffice. After the above fix, another timing issue is exposed: - the navigation behavior's onRouteChange is always called in the order that elements are created. This is problematic since sometimes module's clean-up code run after another module's init code and causing conflicting effects. The above issue is fixed in this CL by adding additional enter and exit hooks to each route, and navigation behavior will trigger them in the expected order. Bug: 908544 Change-Id: If462d2ec955ece0d8b17fc342fc7d1ce0225e72c Reviewed-on: https://chromium-review.googlesource.com/c/1357523 Commit-Queue: Scott Chen <scottchen@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Reviewed-by: Hector Carmona <hcarmona@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#614604}(cherry picked from commit a6223083496b5fed8d8bb69c38b20d7819ca3be9) Reviewed-on: https://chromium-review.googlesource.com/c/1370402 Reviewed-by: Scott Chen <scottchen@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#230} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by scottchen@chromium.org
, Nov 26