feature_engagement::IncognitoWindowTracker::OnBrowsingDataCleared crash |
|||||
Issue description
,
Apr 12 2018
I saw this TODO: // TODO(bdea): Remove early return once https://crbug.com/811982 is fixed. Not sure what "fixing" this bug would mean, because the bug is that app menu button didn't exist, and the fix was to add the early-return. Under what circumstances should we remove that early-return? Context: I am auditing all of these usages of app_menu_button to make sure they all have null checks (Issue 826596), and this looks correct to me (i.e., I would remove the TODO). Out of interest, was the crash happening in hosted app windows (whose toolbar returns null for app_menu_button)?
,
Apr 12 2018
Unfortunately I could never reproduce the problem and the only idea that I had at that time when crashes were happening was that the app menu button was somehow null. I never tried the hosted app windows but that could be a probable cause. I can send out a CL to remove the TODO. Thanks for letting me know!
,
Apr 12 2018
No worries, I'll remove the TODO as part of my CL to update null checks: https://chromium-review.googlesource.com/c/chromium/src/+/1010002
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c852c8db3273cf728bdeb36b5ecd152aea8271c commit 8c852c8db3273cf728bdeb36b5ecd152aea8271c Author: Matt Giuca <mgiuca@chromium.org> Date: Thu Apr 12 22:07:54 2018 Added null checks for properties on ToolbarView that can be null. BrowserView's toolbar() will have null app_menu_button() and browser_actions() in hosted app windows, which could result in null dereferences in certain corner cases. This adds null checks where it's obviously safe to do so. A better fix would be to use toolbar_button_provider() to access the hosted app window's real app menu button and actions container, but that requires more careful consideration. Bug: 826596, 811982 Change-Id: Iea0a0ea3acffe5ed1016e61bd8ae2fe36971c79d Reviewed-on: https://chromium-review.googlesource.com/1010002 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Matt Giuca <mgiuca@chromium.org> Cr-Commit-Position: refs/heads/master@{#550381} [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/feature_engagement/incognito_window/incognito_window_tracker.cc [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc
,
Apr 12 2018
The TODO was removed. I think it was appropriate to add a null-check here (because app menu can be null, e.g., in hosted app windows) and the crash was fixed long ago.
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c852c8db3273cf728bdeb36b5ecd152aea8271c commit 8c852c8db3273cf728bdeb36b5ecd152aea8271c Author: Matt Giuca <mgiuca@chromium.org> Date: Thu Apr 12 22:07:54 2018 Added null checks for properties on ToolbarView that can be null. BrowserView's toolbar() will have null app_menu_button() and browser_actions() in hosted app windows, which could result in null dereferences in certain corner cases. This adds null checks where it's obviously safe to do so. A better fix would be to use toolbar_button_provider() to access the hosted app window's real app menu button and actions container, but that requires more careful consideration. Bug: 826596, 811982 Change-Id: Iea0a0ea3acffe5ed1016e61bd8ae2fe36971c79d Reviewed-on: https://chromium-review.googlesource.com/1010002 Reviewed-by: Avi Drissman <avi@chromium.org> Reviewed-by: Michael Wasserman <msw@chromium.org> Commit-Queue: Matt Giuca <mgiuca@chromium.org> Cr-Commit-Position: refs/heads/master@{#550381} [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/feature_engagement/incognito_window/incognito_window_tracker.cc [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc [modify] https://crrev.com/8c852c8db3273cf728bdeb36b5ecd152aea8271c/chrome/browser/ui/views/toolbar/media_router_action_platform_delegate_views.cc |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by bugdroid1@chromium.org
, Feb 14 2018