GlobalErrorBubbleView holds a WeakPtr to the GlobalErrorWithStandardBubble, but doesn't always check it |
||||
Issue descriptionChanges to GlobalErrorBubbleView a couple of years ago were made assuming that the |error_| WeakPtr is always valid for the newly-added APIs. This leads to crashes like 8ae44c6300000000: 0x00007f69f8a96c10 (chrome -global_error_bubble_view.cc:66 ) GlobalErrorBubbleView::GetWindowTitle 0x00007f69f7ca3380 (chrome -dialog_delegate.cc:258 ) views::DialogDelegateView::GetAccessibleState 0x00007f69f7c6a1b3 (chrome -bubble_dialog_delegate.cc:316 ) views::BubbleDialogDelegateView::HandleVisibilityChanged 0x00007f69f7c9df46 (chrome -widget.cc:1057 ) views::Widget::OnNativeWidgetVisibilityChanged 0x00007f69f7fca87e (chrome -window.cc:742 ) aura::Window::SetVisible 0x00007f69f8a7d9b9 (chrome -multi_user_window_manager_chromeos.cc:730 ) chrome::MultiUserWindowManagerChromeOS::RemoveTransientOwnerRecursive 0x00007f69f7fd68c2 (chrome -transient_window_manager.cc:94 ) wm::TransientWindowManager::RemoveTransientChild 0x00007f69f7fd693a (chrome -transient_window_manager.cc:215 ) wm::TransientWindowManager::OnWindowDestroying 0x00007f69f7fca04d (chrome -window.cc:112 ) aura::Window::~Window 0x00007f69f7fca100 (chrome -window.cc:171 ) aura::Window::~Window 0x00007f69f57755eb (chrome -callback.h:388 ) base::debug::TaskAnnotator::RunTask 0x00007f69f575dc7b (chrome -message_loop.cc:488 ) base::MessageLoop::DoWork 0x00007f69f575e152 (chrome -message_pump_libevent.cc:217 ) base::MessagePumpLibevent::Run 0x00007f69f70d59d7 (chrome -run_loop.cc:35 ) base::RunLoop::Run 0x00007f69f6db61a4 (chrome -chrome_browser_main.cc:2130 ) ChromeBrowserMainParts::MainMessageLoopRun 0x00007f69f5c17f8a (chrome -browser_main_loop.cc:956 ) content::BrowserMainLoop::RunMainMessageLoopParts 0x00007f69f5c19da4 (chrome -browser_main_runner.cc:155 ) content::BrowserMainRunnerImpl::Run 0x00007f69f5c14b1b (chrome -browser_main.cc:46 ) content::BrowserMain 0x00007f69f6d62000 (chrome -content_main_runner.cc:786 ) content::ContentMainRunnerImpl::Run 0x00007f69f6d60b7a (chrome -content_main.cc:20 ) content::ContentMain 0x00007f69f5a127c1 (chrome -chrome_main.cc:85 ) ChromeMain 0x00007f69f304cfb5 (libc-2.19.so -libc-start.c:292 ) __libc_start_main 0x00007f69f5a125eb (chrome + 0x011c95eb ) _start 0x00007ffca9ce6d77
,
Dec 8 2016
Looks like this was changed in https://codereview.chromium.org/933893003/ and then propagated in https://codereview.chromium.org/1846033002.
,
Dec 9 2016
We should (1) add unit-tests to verify that all of the GlobalErrorBubbleView methods are safe to call even after the underlying GlobalError is gone and (2) fix the ones which currently aren't safe (i.e. the ones which don't bother checking the WeakPtr).
,
Dec 22 2016
Going to add other unittests as well.
,
Feb 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40d0dc39f4bfaf7bdf919ce98dfdfab1e6eeb38b commit 40d0dc39f4bfaf7bdf919ce98dfdfab1e6eeb38b Author: lethalantidote <lethalantidote@chromium.org> Date: Fri Feb 10 20:53:01 2017 Fixes ungraceful handling of destroyed GlobalError GlobalErrorBubbleView. This CL fixes incorrect handling of destroyed GlobalError |error_| in GlobalErrorBubbleView. The code should now account for cases in which the |error_| is null. Unit tests have been added to reinforce this, check for access of |error_| and for behavior when |error_| is null. BUG= 672344 Review-Url: https://codereview.chromium.org/2650923002 Cr-Commit-Position: refs/heads/master@{#449727} [modify] https://crrev.com/40d0dc39f4bfaf7bdf919ce98dfdfab1e6eeb38b/chrome/browser/ui/global_error/global_error.h [modify] https://crrev.com/40d0dc39f4bfaf7bdf919ce98dfdfab1e6eeb38b/chrome/browser/ui/views/global_error_bubble_view.cc [add] https://crrev.com/40d0dc39f4bfaf7bdf919ce98dfdfab1e6eeb38b/chrome/browser/ui/views/global_error_bubble_view_unittest.cc [modify] https://crrev.com/40d0dc39f4bfaf7bdf919ce98dfdfab1e6eeb38b/chrome/test/BUILD.gn
,
Feb 10 2017
|
||||
►
Sign in to add a comment |
||||
Comment 1 by w...@chromium.org
, Dec 8 2016