Reland (3rd try): Lazily initialize MessageLoop for faster thread startup
Original review: https://codereview.chromium.org/1011683002/
2nd try: https://codereview.chromium.org/1129953004/
2nd try reverted due to race reports on Linux:
https://crbug.com/489263 Data races on valid_thread_id_ after r330329
This fixes:
- Race in MessageLoopProxyImpl by introducing lock
- Race in BrowserMainLoop/BrowserThreadImpl, where BrowserThread::CurrentlyOn()
called on one of BrowserThreads tries to touch other thread's message_loop()
via global thread table.
Reg: the latter race, the code flow that causes this race is like following:
// On the main thread, we create all known browser threads:
for (...) {
{
AutoLock lock(g_lock);
g_threads[id] = new BrowserProcessSubThread();
}
// [A] This initializes the thread's message_loop, which causes a race
// against [B] in the new code because new threads can start running
// immediately.
thread->StartWithOptions();
}
// On the new thread's main function, it calls CurrentlyOn() which does:
{
// [B] This touches other thread's Thread::message_loop.
AutoLock lock(g_lock);
return g_threads[other_thread_id] &&
g_threads[other_thread_id]->message_loop() == MessageLoop::current();
}
This was safe before because both message_loop initialization and the first
call to CurrentlyOn() on the new thread was done synchronously in
StartWithOptions() while the main thread was blocked. In the new code
new threads can start accessing message_loop() asynchronously while
the main thread's for loop is running.
PS1 is the original patch (2nd try) that got reverted.
BUG=465458, 489263
Review URL: https://codereview.chromium.org/1131513007
Cr-Commit-Position: refs/heads/master@{#331235}
CrOS-Libchrome-Original-Commit: 7f68f8735355c1c73557c3cfb294b441901fc31d
diff --git a/base/message_loop/message_loop.cc b/base/message_loop/message_loop.cc
index eb0f968..4222c77 100644
--- a/base/message_loop/message_loop.cc
+++ b/base/message_loop/message_loop.cc
@@ -100,6 +100,10 @@
}
#endif // !defined(OS_NACL_SFI)
+scoped_ptr<MessagePump> ReturnPump(scoped_ptr<MessagePump> pump) {
+ return pump;
+}
+
} // namespace
//------------------------------------------------------------------------------
@@ -116,41 +120,19 @@
//------------------------------------------------------------------------------
MessageLoop::MessageLoop(Type type)
- : type_(type),
-#if defined(OS_WIN)
- pending_high_res_tasks_(0),
- in_high_res_mode_(false),
-#endif
- nestable_tasks_allowed_(true),
-#if defined(OS_WIN)
- os_modal_loop_(false),
-#endif // OS_WIN
- message_histogram_(NULL),
- run_loop_(NULL) {
- Init();
-
- pump_ = CreateMessagePumpForType(type).Pass();
+ : MessageLoop(type, MessagePumpFactoryCallback()) {
+ BindToCurrentThread();
}
MessageLoop::MessageLoop(scoped_ptr<MessagePump> pump)
- : pump_(pump.Pass()),
- type_(TYPE_CUSTOM),
-#if defined(OS_WIN)
- pending_high_res_tasks_(0),
- in_high_res_mode_(false),
-#endif
- nestable_tasks_allowed_(true),
-#if defined(OS_WIN)
- os_modal_loop_(false),
-#endif // OS_WIN
- message_histogram_(NULL),
- run_loop_(NULL) {
- DCHECK(pump_.get());
- Init();
+ : MessageLoop(TYPE_CUSTOM, Bind(&ReturnPump, Passed(&pump))) {
+ BindToCurrentThread();
}
MessageLoop::~MessageLoop() {
- DCHECK_EQ(this, current());
+ // current() could be NULL if this message loop is destructed before it is
+ // bound to a thread.
+ DCHECK(current() == this || !current());
// iOS just attaches to the loop, it doesn't Run it.
// TODO(stuartmorgan): Consider wiring up a Detach().
@@ -299,11 +281,13 @@
}
void MessageLoop::Run() {
+ DCHECK(pump_);
RunLoop run_loop;
run_loop.Run();
}
void MessageLoop::RunUntilIdle() {
+ DCHECK(pump_);
RunLoop run_loop;
run_loop.RunUntilIdle();
}
@@ -383,13 +367,43 @@
//------------------------------------------------------------------------------
-void MessageLoop::Init() {
+scoped_ptr<MessageLoop> MessageLoop::CreateUnbound(
+ Type type, MessagePumpFactoryCallback pump_factory) {
+ return make_scoped_ptr(new MessageLoop(type, pump_factory));
+}
+
+MessageLoop::MessageLoop(Type type, MessagePumpFactoryCallback pump_factory)
+ : type_(type),
+#if defined(OS_WIN)
+ pending_high_res_tasks_(0),
+ in_high_res_mode_(false),
+#endif
+ nestable_tasks_allowed_(true),
+#if defined(OS_WIN)
+ os_modal_loop_(false),
+#endif // OS_WIN
+ pump_factory_(pump_factory),
+ message_histogram_(NULL),
+ run_loop_(NULL),
+ incoming_task_queue_(new internal::IncomingTaskQueue(this)),
+ message_loop_proxy_(
+ new internal::MessageLoopProxyImpl(incoming_task_queue_)) {
+ // If type is TYPE_CUSTOM non-null pump_factory must be given.
+ DCHECK_EQ(type_ == TYPE_CUSTOM, !pump_factory_.is_null());
+}
+
+void MessageLoop::BindToCurrentThread() {
+ DCHECK(!pump_);
+ if (!pump_factory_.is_null())
+ pump_ = pump_factory_.Run();
+ else
+ pump_ = CreateMessagePumpForType(type_);
+
DCHECK(!current()) << "should only have one message loop per thread";
lazy_tls_ptr.Pointer()->Set(this);
- incoming_task_queue_ = new internal::IncomingTaskQueue(this);
- message_loop_proxy_ =
- new internal::MessageLoopProxyImpl(incoming_task_queue_);
+ incoming_task_queue_->StartScheduling();
+ message_loop_proxy_->BindToCurrentThread();
thread_task_runner_handle_.reset(
new ThreadTaskRunnerHandle(message_loop_proxy_));
}