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

Issue 908544 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit 29 days ago
Closed: Dec 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

[Navi] Browser crashes when using browser-back to get to google apps module

Project Member Reported by scottchen@chromium.org, Nov 26

Issue description

repro:
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.
 
the crash error is:
[FATAL:bookmark_handler.cc(33)] Check failed: args->GetBoolean(0, &show). 


Suspecting this line passing undefined to backend:
https://cs.chromium.org/chromium/src/chrome/browser/resources/welcome/onboarding_welcome/google_apps/apps_chooser.js?q=apps_chooser.js&sq=package:chromium&dr&l=171
Owner: scottchen@chromium.org
Status: Assigned (was: Available)
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
Status: Started (was: Assigned)
Issue 911869 has been merged into this issue.
Cc: ligim...@chromium.org
Labels: ReleaseBlock-Stable Target-73 M-73 FoundIn-73 Stability-Crash
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
Labels: OS-Windows
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Labels: Merge-Request-72
Project Member

Comment 10 by sheriffbot@chromium.org, Dec 8

Labels: -Merge-Request-72 Hotlist-Merge-Approved Merge-Approved-72
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
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 10

Labels: -merge-approved-72 merge-merged-3626
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

Status: Fixed (was: Started)
Not seeing any crashes post 73.0.3634.0 after patch in #8 landed.
Labels: Merge-Merged-72-3626
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