New issue
Advanced search Search tips

Issue 818603 link

Starred by 2 users
Status: Fixed
Owner:
jamescook@chromium.org
Closed: Mar 2018
Cc:
katie@chromium.org
est...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug
Team-Accessibility



Sign in to add a comment

heap-use-after-free in IsDragDropInProgress ui/views/corewm/tooltip_controller.cc:361:28

Project Member Reported by grt@chromium.org, Mar 5 2018 
https://chromium-swarm.appspot.com/task?id=3c02e0e68311e210&refresh=10&show_raw=1

I wonder if r540550 could have provoked this (note ash::ShelfWidget::Shutdown() in the crash stack). It looks to me like Shell::~Shell() should be doing something like this:
  aura::client::SetDragDropClient(root_window, nullptr);

just before
  drag_drop_controller_.reset();

since it appears that the thing being used-after-free is the DragDropController. Then again, I'm not at all familiar with this code. jamescook@: would you please investigate?

These failures coincide with r540714, but I don't see how that CL could be the cause.

=================================================================
==26643==ERROR: AddressSanitizer: heap-use-after-free on address 0x6120000eb840 at pc 0x00000d15c2ec bp 0x7ffcaed4d030 sp 0x7ffcaed4d028
READ of size 8 at 0x6120000eb840 thread T0 (interactive_ui_)
    #0 0xd15c2eb in IsDragDropInProgress ui/views/corewm/tooltip_controller.cc:361:28
    #1 0xd15c2eb in views::corewm::TooltipController::UpdateIfRequired() ui/views/corewm/tooltip_controller.cc:277
    #2 0xd15b639 in views::corewm::TooltipController::UpdateTooltip(aura::Window*) ui/views/corewm/tooltip_controller.cc:148:5
    #3 0xd177acb in views::TooltipManagerAura::UpdateTooltipForTarget(views::View*, gfx::Point const&, aura::Window*) ui/views/widget/tooltip_manager_aura.cc:141:38
    #4 0xd1775c7 in views::TooltipManagerAura::UpdateTooltip() ui/views/widget/tooltip_manager_aura.cc:100:5
    #5 0xd0ef772 in UpdateTooltip ui/views/view.cc:2731:34
    #6 0xd0ef772 in views::View::DoRemoveChildView(views::View*, bool, bool, bool, views::View*) ui/views/view.cc:2134
    #7 0xd0eb59b in RemoveChildView ui/views/view.cc:296:3
    #8 0xd0eb59b in views::View::~View() ui/views/view.cc:156
    #9 0x140e2643 in ~WebNotificationItem ash/system/web_notification/web_notification_tray.cc:124:7
    #10 0x140e2643 in ash::WebNotificationLabel::~WebNotificationLabel() ash/system/web_notification/web_notification_tray.cc:252
    #11 0x140d870c in operator() buildtools/third_party/libc++/trunk/include/memory:2286:5
    #12 0x140d870c in reset buildtools/third_party/libc++/trunk/include/memory:2599
    #13 0x140d870c in ~unique_ptr buildtools/third_party/libc++/trunk/include/memory:2553
    #14 0x140d870c in ash::WebNotificationTray::~WebNotificationTray() ash/system/web_notification/web_notification_tray.cc:332
    #15 0x140d8bbd in ash::WebNotificationTray::~WebNotificationTray() ash/system/web_notification/web_notification_tray.cc:327:45
    #16 0x14063a2d in operator() buildtools/third_party/libc++/trunk/include/memory:2286:5
    #17 0x14063a2d in reset buildtools/third_party/libc++/trunk/include/memory:2599
    #18 0x14063a2d in ash::StatusAreaWidget::~StatusAreaWidget() ash/system/status_area_widget.cc:99
    #19 0x1406442d in ash::StatusAreaWidget::~StatusAreaWidget() ash/system/status_area_widget.cc:96:39
    #20 0x13fb35a2 in operator() buildtools/third_party/libc++/trunk/include/memory:2286:5
    #21 0x13fb35a2 in reset buildtools/third_party/libc++/trunk/include/memory:2599
    #22 0x13fb35a2 in ash::ShelfWidget::Shutdown() ash/shelf/shelf_widget.cc:239
    #23 0x13f618ce in ash::RootWindowController::CloseChildWindows() ash/root_window_controller.cc:488:11
    #24 0x13fca3ff in ash::Shell::CloseAllRootWindowChildWindows() ash/shell.cc:1257:19
    #25 0x13fc521a in ash::Shell::~Shell() ash/shell.cc:757:3
    #26 0x13fca64d in ash::Shell::~Shell() ash/shell.cc:670:17
    #27 0x153751a1 in AshShellInit::~AshShellInit() chrome/browser/ui/ash/ash_shell_init.cc:105:5
    #28 0x14c83e0f in operator() buildtools/third_party/libc++/trunk/include/memory:2286:5
    #29 0x14c83e0f in reset buildtools/third_party/libc++/trunk/include/memory:2599
    #30 0x14c83e0f in ChromeBrowserMainExtraPartsAsh::PostMainMessageLoopRun() chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc:313
    #31 0xaa7015c in ChromeBrowserMainParts::PostMainMessageLoopRun() chrome/browser/chrome_browser_main.cc:2200:29
    #32 0x7174d08 in chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() chrome/browser/chromeos/chrome_browser_main_chromeos.cc:1147:32
    #33 0x409f6ad in content::BrowserMainLoop::ShutdownThreadsAndCleanUp() content/browser/browser_main_loop.cc:1143:13
    #34 0x40a9ce6 in content::BrowserMainRunnerImpl::Shutdown() content/browser/browser_main_runner.cc:220:19
    #35 0x40924fb in content::BrowserMain(content::MainFunctionParams const&) content/browser/browser_main.cc:48:16
    #36 0xa78fd4b in content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:423:14
    #37 0xa7931e8 in content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:703:12
    #38 0x11543c76 in service_manager::Main(service_manager::MainParams const&) services/service_manager/embedder/main.cc:453:29
    #39 0xa78e2fb in content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:10
    #40 0xba0522c in content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:320:3
    #41 0xaa36716 in InProcessBrowserTest::SetUp() chrome/test/base/in_process_browser_test.cc:225:20
    #42 0xdf3e5c in testing::Test::Run() third_party/googletest/src/googletest/src/gtest-internal-inl.h
    #43 0xdf6084 in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2651:11
    #44 0xdf73e6 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2769:28
    #45 0xe0fb86 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:4642:43
    #46 0xe0f0c8 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc
    #47 0xd721a2b in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2314:46
    #48 0xd721a2b in base::TestSuite::Run() base/test/test_suite.cc:271
    #49 0x9d82e2 in InteractiveUITestSuiteRunner::RunTestSuite(int, char**) chrome/test/base/interactive_ui_tests_main.cc:117:47
    #50 0xba33300 in content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) content/public/test/test_launcher.cc:623:31
    #51 0xaa1deb9 in LaunchChromeTests(unsigned long, content::TestLauncherDelegate*, int, char**) chrome/test/base/chrome_test_launcher.cc:171:10
    #52 0x9d8100 in main chrome/test/base/interactive_ui_tests_main.cc:141:10
    #53 0x7f4d72e10f44 in __libc_start_main /build/eglibc-ripdx6/eglibc-2.19/csu/libc-start.c:287

0x6120000eb840 is located 0 bytes inside of 280-byte region [0x6120000eb840,0x6120000eb958)
freed by thread T0 (interactive_ui_) here:
    #0 0x5801f2 in operator delete(void*) /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:149:3
    #1 0x13fc4e67 in operator() buildtools/third_party/libc++/trunk/include/memory:2286:5
    #2 0x13fc4e67 in reset buildtools/third_party/libc++/trunk/include/memory:2599
    #3 0x13fc4e67 in ash::Shell::~Shell() ash/shell.cc:732
    #4 0x13fca64d in ash::Shell::~Shell() ash/shell.cc:670:17
    #5 0x153751a1 in AshShellInit::~AshShellInit() chrome/browser/ui/ash/ash_shell_init.cc:105:5
    #6 0x14c83e0f in operator() buildtools/third_party/libc++/trunk/include/memory:2286:5
    #7 0x14c83e0f in reset buildtools/third_party/libc++/trunk/include/memory:2599
    #8 0x14c83e0f in ChromeBrowserMainExtraPartsAsh::PostMainMessageLoopRun() chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc:313
    #9 0xaa7015c in ChromeBrowserMainParts::PostMainMessageLoopRun() chrome/browser/chrome_browser_main.cc:2200:29
    #10 0x7174d08 in chromeos::ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() chrome/browser/chromeos/chrome_browser_main_chromeos.cc:1147:32
    #11 0x409f6ad in content::BrowserMainLoop::ShutdownThreadsAndCleanUp() content/browser/browser_main_loop.cc:1143:13
    #12 0x40a9ce6 in content::BrowserMainRunnerImpl::Shutdown() content/browser/browser_main_runner.cc:220:19
    #13 0x40924fb in content::BrowserMain(content::MainFunctionParams const&) content/browser/browser_main.cc:48:16
    #14 0xa78fd4b in content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:423:14
    #15 0xa7931e8 in content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:703:12
    #16 0x11543c76 in service_manager::Main(service_manager::MainParams const&) services/service_manager/embedder/main.cc:453:29
    #17 0xa78e2fb in content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:10
    #18 0xba0522c in content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:320:3
    #19 0xaa36716 in InProcessBrowserTest::SetUp() chrome/test/base/in_process_browser_test.cc:225:20
    #20 0xdf3e5c in testing::Test::Run() third_party/googletest/src/googletest/src/gtest-internal-inl.h
    #21 0xdf6084 in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2651:11
    #22 0xdf73e6 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2769:28
    #23 0xe0fb86 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:4642:43
    #24 0xe0f0c8 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc
    #25 0xd721a2b in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2314:46
    #26 0xd721a2b in base::TestSuite::Run() base/test/test_suite.cc:271
    #27 0x9d82e2 in InteractiveUITestSuiteRunner::RunTestSuite(int, char**) chrome/test/base/interactive_ui_tests_main.cc:117:47
    #28 0xba33300 in content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) content/public/test/test_launcher.cc:623:31
    #29 0xaa1deb9 in LaunchChromeTests(unsigned long, content::TestLauncherDelegate*, int, char**) chrome/test/base/chrome_test_launcher.cc:171:10
    #30 0x9d8100 in main chrome/test/base/interactive_ui_tests_main.cc:141:10
    #31 0x7f4d72e10f44 in __libc_start_main /build/eglibc-ripdx6/eglibc-2.19/csu/libc-start.c:287

previously allocated by thread T0 (interactive_ui_) here:
    #0 0x57f612 in operator new(unsigned long) /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/asan_new_delete.cc:92:3
    #1 0x13fb90cf in make_unique<ash::DragDropController> buildtools/third_party/libc++/trunk/include/memory:3079:28
    #2 0x13fb90cf in ash::Shell::Init(ui::ContextFactory*, ui::ContextFactoryPrivate*) ash/shell.cc:1048
    #3 0x13fb677d in ash::Shell::CreateInstance(ash::ShellInitParams) ash/shell.cc:242:14
    #4 0x15374e6c in CreateClassicShell chrome/browser/ui/ash/ash_shell_init.cc:48:3
    #5 0x15374e6c in AshShellInit::AshShellInit() chrome/browser/ui/ash/ash_shell_init.cc:89
    #6 0x14c821c6 in make_unique<AshShellInit> buildtools/third_party/libc++/trunk/include/memory:3079:32
    #7 0x14c821c6 in ChromeBrowserMainExtraPartsAsh::PreProfileInit() chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc:192
    #8 0xaa6decf in ChromeBrowserMainParts::PreProfileInit() chrome/browser/chrome_browser_main.cc:1461:29
    #9 0xaa72546 in ChromeBrowserMainPartsLinux::PreProfileInit() chrome/browser/chrome_browser_main_linux.cc:83:32
    #10 0x7170b79 in chromeos::ChromeBrowserMainPartsChromeos::PreProfileInit() chrome/browser/chromeos/chrome_browser_main_chromeos.cc:811:32
    #11 0xaa6b1aa in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() chrome/browser/chrome_browser_main.cc:1805:3
    #12 0xaa69e6e in ChromeBrowserMainParts::PreMainMessageLoopRun() chrome/browser/chrome_browser_main.cc:1440:18
    #13 0x716fda9 in chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() chrome/browser/chromeos/chrome_browser_main_chromeos.cc:710:32
    #14 0x409e657 in content::BrowserMainLoop::PreMainMessageLoopRun() content/browser/browser_main_loop.cc:1087:13
    #15 0x51641e7 in Run base/callback.h:124:12
    #16 0x51641e7 in content::StartupTaskRunner::RunAllTasksNow() content/browser/startup_task_runner.cc:45
    #17 0x4099bcb in content::BrowserMainLoop::CreateStartupTasks() content/browser/browser_main_loop.cc:970:25
    #18 0x40a8430 in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) content/browser/browser_main_runner.cc:139:17
    #19 0x409247f in content::BrowserMain(content::MainFunctionParams const&) content/browser/browser_main.cc:42:32
    #20 0xa78fd4b in content::RunNamedProcessTypeMain(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, content::MainFunctionParams const&, content::ContentMainDelegate*) content/app/content_main_runner.cc:423:14
    #21 0xa7931e8 in content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:703:12
    #22 0x11543c76 in service_manager::Main(service_manager::MainParams const&) services/service_manager/embedder/main.cc:453:29
    #23 0xa78e2fb in content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:10
    #24 0xba0522c in content::BrowserTestBase::SetUp() content/public/test/browser_test_base.cc:320:3
    #25 0xaa36716 in InProcessBrowserTest::SetUp() chrome/test/base/in_process_browser_test.cc:225:20
    #26 0xdf3e5c in testing::Test::Run() third_party/googletest/src/googletest/src/gtest-internal-inl.h
    #27 0xdf6084 in testing::TestInfo::Run() third_party/googletest/src/googletest/src/gtest.cc:2651:11
    #28 0xdf73e6 in testing::TestCase::Run() third_party/googletest/src/googletest/src/gtest.cc:2769:28
    #29 0xe0fb86 in testing::internal::UnitTestImpl::RunAllTests() third_party/googletest/src/googletest/src/gtest.cc:4642:43
    #30 0xe0f0c8 in testing::UnitTest::Run() third_party/googletest/src/googletest/src/gtest.cc
    #31 0xd721a2b in RUN_ALL_TESTS third_party/googletest/src/googletest/include/gtest/gtest.h:2314:46
    #32 0xd721a2b in base::TestSuite::Run() base/test/test_suite.cc:271
    #33 0x9d82e2 in InteractiveUITestSuiteRunner::RunTestSuite(int, char**) chrome/test/base/interactive_ui_tests_main.cc:117:47
    #34 0xba33300 in content::LaunchTests(content::TestLauncherDelegate*, unsigned long, int, char**) content/public/test/test_launcher.cc:623:31
Project Member

Comment 1 by bugdroid1@chromium.org, Mar 5 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d11349090e540863fd7ec681ac4449afb9eae42

commit 3d11349090e540863fd7ec681ac4449afb9eae42
Author: Greg Thompson <grt@chromium.org>
Date: Mon Mar 05 10:35:35 2018

Disable SelectToSpeakTest.ContinuesReadingDuringResize under ASAN.

BUG= 818603 
TBR=jamescook@chromium.org

Change-Id: Icaa17e76bfa8067091870ddb818b89210866ce30
Reviewed-on: https://chromium-review.googlesource.com/947975
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540801}
[modify] https://crrev.com/3d11349090e540863fd7ec681ac4449afb9eae42/chrome/browser/chromeos/accessibility/select_to_speak_browsertest.cc
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 5 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6949ff07672b27f41d30459796305fcd7862a4bd

commit 6949ff07672b27f41d30459796305fcd7862a4bd
Author: Greg Thompson <grt@chromium.org>
Date: Mon Mar 05 14:32:41 2018

Revert "cros: Cleanup ShelfWidget::DelegateView shutdown"

This reverts commit cc4be1cb9d4b439feacae23757d1b339b302e349.

Reason for revert: speculative revert for test crashes deep in ShelfWidget::Shutdown; see  https://crbug.com/818603 .
BUG= 818603 

Original change's description:
> cros: Cleanup ShelfWidget::DelegateView shutdown
> 
> Make it use DeleteDelegate() to be more consistent with
> StatusAreaWidgetDelegate. Defer closing the widget until all the
> top-level windows are closed.
> 
> Bug:  628655 
> Test: ash_unittests
> Change-Id: I066b45dab9c848b8f990f7fb51f0170e5e66a861
> Reviewed-on: https://chromium-review.googlesource.com/907390
> Commit-Queue: James Cook <jamescook@chromium.org>
> Reviewed-by: Michael Wasserman <msw@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#540550}

TBR=jamescook@chromium.org,msw@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  628655 
Change-Id: Ia3eb845627c77761bb15e94f9bbc9b3f1643f584
Reviewed-on: https://chromium-review.googlesource.com/948494
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540819}
[modify] https://crrev.com/6949ff07672b27f41d30459796305fcd7862a4bd/ash/root_window_controller.cc
[modify] https://crrev.com/6949ff07672b27f41d30459796305fcd7862a4bd/ash/shelf/shelf.cc
[modify] https://crrev.com/6949ff07672b27f41d30459796305fcd7862a4bd/ash/shelf/shelf_widget.cc
[modify] https://crrev.com/6949ff07672b27f41d30459796305fcd7862a4bd/ash/shelf/shelf_widget.h
[modify] https://crrev.com/6949ff07672b27f41d30459796305fcd7862a4bd/ash/shell.cc

Comment 3 by jamescook@chromium.org, Mar 5 2018

Cc: est...@chromium.org
Status: Started (was: Assigned)
Yeah, this is probably my CL. I'll take a look.

+estade just FYI, since I suspect the problem is related to having a notification open during shutdown. However, I suspect it's because I made some changes to shelf teardown ordering.

Comment 4 by katydek@google.com, Mar 5 2018 

I'm hoping to cherry-pick r540714 into M65 and M66. Does this mean I should also cherry pick the changes made at comment #1 and comment #2?

Comment 5 by jamescook@chromium.org, Mar 5 2018 

I don't think you need to merge those. My bad CL landed at r540550, which was after the M66 branch (at r540276), so it isn't in M66. You shouldn't need to disable that test in the branch either.

I'm hoping to re-enable the test after I figure out what went wrong with my code.

Comment 6 by grt@chromium.org, Mar 5 2018 

FWIW: interactive_ui_tests passed in the build that included the revert: https://ci.chromium.org/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/26446.

It seems problematic to me that the DragDropController is destroyed while it's still the client for the root win. Maybe your CL simply unmasked an existing bug?
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 5 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3a43106b54c6338ba3b3ae9382008dd8972f57af

commit 3a43106b54c6338ba3b3ae9382008dd8972f57af
Author: Greg Thompson <grt@chromium.org>
Date: Mon Mar 05 19:59:24 2018

Revert "Disable SelectToSpeakTest.ContinuesReadingDuringResize under ASAN."

This reverts commit 3d11349090e540863fd7ec681ac4449afb9eae42.

Reason for revert: I don't think that this test was at fault. Re-enabling.

Original change's description:
> Disable SelectToSpeakTest.ContinuesReadingDuringResize under ASAN.
> 
> BUG= 818603 
> TBR=jamescook@chromium.org
> 
> Change-Id: Icaa17e76bfa8067091870ddb818b89210866ce30
> Reviewed-on: https://chromium-review.googlesource.com/947975
> Reviewed-by: Greg Thompson <grt@chromium.org>
> Commit-Queue: Greg Thompson <grt@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#540801}

TBR=jamescook@chromium.org,grt@chromium.org

Change-Id: I9dfa369c8ff1c01584b73a18f6c7f9e0e55c2df4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  818603 
Reviewed-on: https://chromium-review.googlesource.com/949682
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540905}
[modify] https://crrev.com/3a43106b54c6338ba3b3ae9382008dd8972f57af/chrome/browser/chromeos/accessibility/select_to_speak_browsertest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 6 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e0f3ffe2a0a0d08de56130c4634bd8a2429ec13

commit 1e0f3ffe2a0a0d08de56130c4634bd8a2429ec13
Author: James Cook <jamescook@chromium.org>
Date: Tue Mar 06 22:52:45 2018

Fix potential DragDropController use-after-free in ash shutdown

ash::Shell::drag_drop_controller_ is reset before widgets and window
containers are closed. aura::client::SetDragDropClient null is called
later, after windows close. Thus if closing any window triggers a
call to GetDragDropClient it will access deleted memory.

This code has been refactored multiple times, including this 2013 CL
to change the window close order:
https://chromium.googlesource.com/chromium/src/+/8b3e3d8f101deab32cc3482ad752e99802efe109

and this 2016 CL that moved SetDragDropClient as part of the mustash
"wm common" refactor:
https://chromium.googlesource.com/chromium/src/+/29f31715ad035be45c19469afe2012209cb92125

Fix by moving SetDragDropClient to immediately before the controller
is reset, which means it is reset in the same class that allocates
the object. All calls to GetDragDropClient check the pointer for null
before access. If someone adds a call and doesn't check for null it
will cause a crash instead of UAF, which seems better.

Bug:  818603 
Test: asan interactive_ui_tests, see bug
Change-Id: I6d1a468dceff26da36e5c7c4af4962941db46a87
Reviewed-on: https://chromium-review.googlesource.com/951622
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541222}
[modify] https://crrev.com/1e0f3ffe2a0a0d08de56130c4634bd8a2429ec13/ash/root_window_controller.cc
[modify] https://crrev.com/1e0f3ffe2a0a0d08de56130c4634bd8a2429ec13/ash/shell.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e

commit ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e
Author: James Cook <jamescook@chromium.org>
Date: Sat Mar 10 01:48:10 2018

Reland: cros: Cleanup ShelfWidget shutdown

Make it use DeleteDelegate() to be more consistent with
StatusAreaWidgetDelegate. Defer closing the widget until all the
top-level windows are closed. Don't try to close all windows more than
once during shutdown, which simplifies the shelf cleanup code.

Originally landed as cc4be1cb9d4b439feacae23757d1b339b302e349.

That version exposed a use-after-free in ash drag and drop code,
which was fixed separately. See  https://crbug.com/818603 

TBR=sadrul@chromium.org

Bug:  628655 ,  818603 
Test: ash_unittests
Change-Id: I4a8ade351456b476a19225f2c740e031f6b0e122
Reviewed-on: https://chromium-review.googlesource.com/956924
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542308}
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/ash/magnifier/magnification_controller.cc
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/ash/root_window_controller.cc
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/ash/root_window_controller.h
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/ash/shelf/shelf.cc
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/ash/shelf/shelf_layout_manager_unittest.cc
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/ash/shelf/shelf_widget.cc
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/ash/shelf/shelf_widget.h
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/ash/shell.cc
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/testing/buildbot/filters/mash.ash_unittests.filter
[modify] https://crrev.com/ee2daef503bf195efdb101bcd8c5ad7df9b6ff3e/ui/aura/mus/window_tree_host_mus.cc

Comment 10 by jamescook@chromium.org, Mar 12 2018

Status: Fixed (was: Started)
►

Sign in to add a comment