Revert "Makes EmbeddedServiceRunner block until background thread completes"
This reverts commit 9525c34399f488dd3cc41b509b5a18446e9e0208.
Reason for revert: Looks like this broke interactive_ui_tests on Linux Chromium OS ASan LSan Tests (1):
https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/22950
=================================================================
==27422==ERROR: AddressSanitizer: heap-use-after-free on address 0x6110001fbb88 at pc 0x0000113dc8f8 bp 0x7ffce4666c90 sp 0x7ffce4666c88
READ of size 8 at 0x6110001fbb88 thread T0 (interactive_ui_)
#0 0x113dc8f7 in begin buildtools/third_party/libc++/trunk/include/vector:1465:30
#1 0x113dc8f7 in base::ObserverListBase<chromeos::OobeUI::Observer>::RemoveObserver(chromeos::OobeUI::Observer*) base/observer_list.h:286
#2 0x11ba806a in chromeos::ArcTermsOfServiceScreenHandler::~ArcTermsOfServiceScreenHandler() chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.cc:40:11
#3 0x11ba8edd in chromeos::ArcTermsOfServiceScreenHandler::~ArcTermsOfServiceScreenHandler() chrome/browser/ui/webui/chromeos/login/arc_terms_of_service_screen_handler.cc:39:67
#4 0x3b94103 in operator() buildtools/third_party/libc++/trunk/include/memory:2272:5
#5 0x3b94103 in reset buildtools/third_party/libc++/trunk/include/memory:2585
#6 0x3b94103 in ~unique_ptr buildtools/third_party/libc++/trunk/include/memory:2539
#7 0x3b94103 in destroy buildtools/third_party/libc++/trunk/include/memory:1853
#8 0x3b94103 in __destroy<std::__1::unique_ptr<content::WebUIMessageHandler, std::__1::default_delete<content::WebUIMessageHandler> > > buildtools/third_party/libc++/trunk/include/memory:1721
#9 0x3b94103 in destroy<std::__1::unique_ptr<content::WebUIMessageHandler, std::__1::default_delete<content::WebUIMessageHandler> > > buildtools/third_party/libc++/trunk/include/memory:1589
#10 0x3b94103 in __destruct_at_end buildtools/third_party/libc++/trunk/include/vector:418
#11 0x3b94103 in clear buildtools/third_party/libc++/trunk/include/vector:361
#12 0x3b94103 in ~__vector_base buildtools/third_party/libc++/trunk/include/vector:446
#13 0x3b94103 in content::WebUIImpl::~WebUIImpl() content/browser/webui/web_ui_impl.cc:91
#14 0x3b9428d in content::WebUIImpl::~WebUIImpl() content/browser/webui/web_ui_impl.cc:87:25
#15 0x303a2c5 in content::RenderFrameHostManager::ClearWebUIInstances() content/browser/frame_host/render_frame_host_manager.cc:679:25
#16 0x3aee854 in content::WebContentsImpl::~WebContentsImpl() content/browser/web_contents/web_contents_impl.cc:600:29
#17 0x3af1b8d in content::WebContentsImpl::~WebContentsImpl() content/browser/web_contents/web_contents_impl.cc:571:37
#18 0x12091e89 in operator() buildtools/third_party/libc++/trunk/include/memory:2272:5
#19 0x12091e89 in reset buildtools/third_party/libc++/trunk/include/memory:2585
#20 0x12091e89 in views::WebView::SetWebContents(content::WebContents*) ui/views/controls/webview/webview.cc:73
#21 0x120932f1 in ~WebView ui/views/controls/webview/webview.cc:44:3
#22 0x120932f1 in views::WebView::~WebView() ui/views/controls/webview/webview.cc:43
#23 0x520969f in operator() buildtools/third_party/libc++/trunk/include/memory:2272:5
#24 0x520969f in reset buildtools/third_party/libc++/trunk/include/memory:2585
#25 0x520969f in ~unique_ptr buildtools/third_party/libc++/trunk/include/memory:2539
#26 0x520969f in chromeos::WebUILoginView::~WebUILoginView() chrome/browser/chromeos/login/ui/webui_login_view.cc:224
#27 0x520a07d in chromeos::WebUILoginView::~WebUILoginView() chrome/browser/chromeos/login/ui/webui_login_view.cc:199:35
#28 0xad7565b in operator() buildtools/third_party/libc++/trunk/include/memory:2272:5
#29 0xad7565b in reset buildtools/third_party/libc++/trunk/include/memory:2585
#30 0xad7565b in ~unique_ptr buildtools/third_party/libc++/trunk/include/memory:2539
#31 0xad7565b in views::View::DoRemoveChildView(views::View*, bool, bool, bool, views::View*) ui/views/view.cc:2136
#32 0xad77048 in views::View::RemoveAllChildViews(bool) ui/views/view.cc:302:5
#33 0xad9b615 in views::internal::RootView::~RootView() ui/views/widget/root_view.cc:181:5
#34 0xad9b7cd in views::internal::RootView::~RootView() ui/views/widget/root_view.cc:177:23
#35 0xada5ac1 in operator() buildtools/third_party/libc++/trunk/include/memory:2272:5
#36 0xada5ac1 in reset buildtools/third_party/libc++/trunk/include/memory:2585
#37 0xada5ac1 in DestroyRootView ui/views/widget/widget.cc:1434
#38 0xada5ac1 in views::Widget::~Widget() ui/views/widget/widget.cc:180
#39 0xada61ad in views::Widget::~Widget() ui/views/widget/widget.cc:179:19
#40 0xade9e6f in views::NativeWidgetAura::~NativeWidgetAura() ui/views/widget/native_widget_aura.cc
#41 0xadea10d in views::NativeWidgetAura::~NativeWidgetAura() ui/views/widget/native_widget_aura.cc:986:39
#42 0xe11b498 in aura::Window::~Window() ui/aura/window.cc:123:16
#43 0xe11c90d in aura::Window::~Window() ui/aura/window.cc:77:19
#44 0xadeb043 in Invoke<const base::WeakPtr<views::NativeWidgetAura> &> base/bind_internal.h:196:12
#45 0xadeb043 in MakeItSo<void (views::NativeWidgetAura::*const &)(), const base::WeakPtr<views::NativeWidgetAura> &> base/bind_internal.h:285
#46 0xadeb043 in RunImpl<void (views::NativeWidgetAura::*const &)(), const std::__1::tuple<base::WeakPtr<views::NativeWidgetAura> > &, 0> base/bind_internal.h:340
#47 0xadeb043 in base::internal::Invoker<base::internal::BindState<void (views::NativeWidgetAura::*)(), base::WeakPtr<views::NativeWidgetAura> >, void ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:319
#48 0xb35e805 in Run base/callback.h:91:12
#49 0xb35e805 in base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) base/debug/task_annotator.cc:59
#50 0xb3d46da in base::MessageLoop::RunTask(base::PendingTask*) base/message_loop/message_loop.cc:410:19
#51 0xb3d5040 in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) base/message_loop/message_loop.cc:421:5
#52 0xb3d5d81 in base::MessageLoop::DoWork() base/message_loop/message_loop.cc:528:13
#53 0xb3e0820 in base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_libevent.cc:220:31
#54 0xb3d3535 in base::MessageLoop::Run() base/message_loop/message_loop.cc:350:10
#55 0xb472370 in base::RunLoop::Run() base/run_loop.cc:123:14
#56 0x9304487 in RunThisRunLoop content/public/test/test_utils.cc:125:13
#57 0x9304487 in content::RunAllPendingInMessageLoop() content/public/test/test_utils.cc:133
#58 0x82fe2ba in InProcessBrowserTest::PostRunTestOnMainThread() chrome/test/base/in_process_browser_test.cc:607:3
#59 0x92d79f8 in content::BrowserTestBase::ProxyRunTestOnMainThreadLoop() content/public/test/browser_test_base.cc:319:5
#60 0x83331b2 in Run base/callback.h:80:12
#61 0x83331b2 in ChromeBrowserMainParts::PreMainMessageLoopRunImpl() chrome/browser/chrome_browser_main.cc:1857
#62 0x83304b3 in ChromeBrowserMainParts::PreMainMessageLoopRun() chrome/browser/chrome_browser_main.cc:1210:18
#63 0x4dff120 in chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() chrome/browser/chromeos/chrome_browser_main_chromeos.cc:655:32
#64 0x2b32584 in content::BrowserMainLoop::PreMainMessageLoopRun() content/browser/browser_main_loop.cc:1151:13
#65 0x3a66b08 in Run base/callback.h:80:12
#66 0x3a66b08 in content::StartupTaskRunner::RunAllTasksNow() content/browser/startup_task_runner.cc:45
#67 0x2b2d47f in content::BrowserMainLoop::CreateStartupTasks() content/browser/browser_main_loop.cc:935:25
#68 0x2b3b21d in content::BrowserMainRunnerImpl::Initialize(content::MainFunctionParams const&) content/browser/browser_main_runner.cc:131:17
#69 0x2b252a6 in content::BrowserMain(content::MainFunctionParams const&) content/browser/browser_main.cc:42:32
#70 0x8013aae 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:408:14
#71 0x8015aa9 in content::ContentMainRunnerImpl::Run() content/app/content_main_runner.cc:690:12
#72 0xe62b2ec in service_manager::Main(service_manager::MainParams const&) services/service_manager/embedder/main.cc:469:29
#73 0x8011e73 in content::ContentMain(content::ContentMainParams const&) content/app/content_main.cc:19:10
Original change's description:
> Makes EmbeddedServiceRunner block until background thread completes
>
> Prior to this when EmbeddedServiceRunner was destroyed the background
> thread would most likely still be running. Worse yet it was entirely
> possible for ~InstanceManager to run on the same thread as the
> service was running on, which attempts to destroy thread_ (because it
> assumes thread_ was destroyed already). When this happens you hit a
> CHECK in thread_ because you can't join with a thread while you are on
> the thread.
>
> By making Shutdown() block until the background thread has said it's
> ready to be shutdown, and then destroy the thread from the initiating
> thread we never hit this situation.
>
> BUG=none
> TEST=covered by test
>
> Change-Id: I09ebc5167247e2e8a1b15b625fdff8dc8b8d2e8f
> Reviewed-on: https://chromium-review.googlesource.com/611392
> Commit-Queue: Scott Violet <sky@chromium.org>
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#493878}
TBR=sky@chromium.org,rockot@chromium.org
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug: none
Change-Id: Ib5a4bdf57e78f5ecb61983c6c29ca8f80d6da04f
Reviewed-on: https://chromium-review.googlesource.com/612810
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494001}diff --git a/services/service_manager/embedder/BUILD.gn b/services/service_manager/embedder/BUILD.gn
index 6511fc7..126bb9a 100644
--- a/services/service_manager/embedder/BUILD.gn
+++ b/services/service_manager/embedder/BUILD.gn
@@ -15,7 +15,6 @@
"embedded_service_info.cc",
"embedded_service_runner.cc",
"manifest_utils.cc",
- "service_manager_embedder_export.h",
]
# iOS embeds the Service Manager but does not use service_manager::Main() (and
@@ -33,6 +32,7 @@
sources += [
"main.cc",
"main_delegate.cc",
+ "service_manager_embedder_export.h",
"set_process_title.cc",
"set_process_title_linux.cc",
"shared_file_util.cc",
@@ -82,22 +82,12 @@
testonly = true
sources = [
- "embedded_instance_manager_unittest.cc",
"manifest_utils_unittest.cc",
]
- # These headers are duplicated here so that they can remain private in the
- # "embedder" target. See http://crbug.com/732993 for a way to make it
- # unnecessary to do this.
- sources += [
- "embedded_instance_manager.h",
- "service_manager_embedder_export.h",
- ]
-
deps = [
":embedder",
"//base",
- "//base/test:test_support",
"//testing/gtest",
]
}
diff --git a/services/service_manager/embedder/embedded_instance_manager.cc b/services/service_manager/embedder/embedded_instance_manager.cc
index 16d2b7e..b5c4104 100644
--- a/services/service_manager/embedder/embedded_instance_manager.cc
+++ b/services/service_manager/embedder/embedded_instance_manager.cc
@@ -61,7 +61,6 @@
FROM_HERE,
base::Bind(&EmbeddedInstanceManager::QuitOnServiceSequence, this));
}
- thread_.reset();
}
EmbeddedInstanceManager::~EmbeddedInstanceManager() {
diff --git a/services/service_manager/embedder/embedded_instance_manager.h b/services/service_manager/embedder/embedded_instance_manager.h
index c1755a66..4a9519d 100644
--- a/services/service_manager/embedder/embedded_instance_manager.h
+++ b/services/service_manager/embedder/embedded_instance_manager.h
@@ -29,8 +29,6 @@
namespace service_manager {
-class EmbeddedInstanceManagerTestApi;
-
// EmbeddedInstanceManager is an implementation detail of EmbeddedServiceRunner.
// Outside of tests there is no need to use it directly.
class SERVICE_MANAGER_EMBEDDER_EXPORT EmbeddedInstanceManager
@@ -46,7 +44,6 @@
private:
friend class base::RefCountedThreadSafe<EmbeddedInstanceManager>;
- friend class EmbeddedInstanceManagerTestApi;
~EmbeddedInstanceManager();
diff --git a/services/service_manager/embedder/embedded_instance_manager_unittest.cc b/services/service_manager/embedder/embedded_instance_manager_unittest.cc
deleted file mode 100644
index dc011ed..0000000
--- a/services/service_manager/embedder/embedded_instance_manager_unittest.cc
+++ /dev/null
@@ -1,68 +0,0 @@
-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "services/service_manager/embedder/embedded_instance_manager.h"
-
-#include <memory>
-
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/memory/ptr_util.h"
-#include "base/single_thread_task_runner.h"
-#include "base/test/scoped_task_environment.h"
-#include "base/threading/thread.h"
-#include "services/service_manager/embedder/embedded_service_info.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace service_manager {
-namespace {
-
-void OnQuit(bool* quit_called) {
- *quit_called = true;
-}
-
-std::unique_ptr<Service> CreateTestService() {
- return base::MakeUnique<Service>();
-}
-
-} // namespace
-
-class EmbeddedInstanceManagerTestApi {
- public:
- EmbeddedInstanceManagerTestApi(EmbeddedInstanceManager* instance)
- : instance_(instance) {}
-
- base::Thread* GetThread() { return instance_->thread_.get(); }
-
- private:
- EmbeddedInstanceManager* instance_;
-
- DISALLOW_COPY_AND_ASSIGN(EmbeddedInstanceManagerTestApi);
-};
-
-TEST(EmbeddedInstanceManager, ShutdownWaitsForThreadToQuit) {
- base::test::ScopedTaskEnvironment scoped_task_environment;
- EmbeddedServiceInfo embedded_service_info;
- embedded_service_info.use_own_thread = true;
- embedded_service_info.factory = base::Bind(&CreateTestService);
- bool quit_called = false;
- scoped_refptr<EmbeddedInstanceManager> instance_manager =
- new EmbeddedInstanceManager("test", embedded_service_info,
- base::Bind(&OnQuit, &quit_called));
- instance_manager->BindServiceRequest(nullptr);
- EmbeddedInstanceManagerTestApi test_api(instance_manager.get());
- ASSERT_TRUE(test_api.GetThread());
- scoped_refptr<base::SingleThreadTaskRunner> thread_task_runner =
- test_api.GetThread()->task_runner();
- instance_manager->ShutDown();
- EXPECT_FALSE(test_api.GetThread());
- // Further verification the thread was shutdown.
- EXPECT_FALSE(
- thread_task_runner->PostTask(FROM_HERE, base::Bind(&base::DoNothing)));
- // Because Shutdown() was explicitly called with the thread running the
- // quit closure should not have run.
- EXPECT_FALSE(quit_called);
-}
-
-} // namespace service_manager
