Why MessageLoop* message_loop_ in class base::Thread is created and destructed in different threads ?

[冷二荣]

Dear All,

I want to @ the threading owner, j...@chromium.org

Refer to base/threading codes,
message_loop_ was created in Thread::StartWithOptions function and destructed in Thread::ThreadMain
bool Thread::StartWithOptions(const Options& options) {
   scoped_ptr<MessageLoop> message_loop = MessageLoop::CreateUnbound(
      type, options.message_pump_factory);
  message_loop_ = message_loop.get();
}

As I know, these two function are running in different threads.
a object was created in A thread, but was destroyed in B thread, it is not a good coding style.
There will be potential issue when try to destroy an thread object.
Thread::~Thread() {
  Stop();
}
If the sequence running like:
1. cpu run in io thread, call  XXXThread::Stop, check message_loop_ has valid value, then goes to  task_runner function
2. cpu switch to  XXXThread thread, it goes to the end of its ThreadMain, and set message_loop_ to null
3. cpu switch to io thread again, and execute 
message_loop_->task_runner(), at that time, message_loop_ was null, then it will cause the segment fault issue.

So I want to ask is it designed to handle message_loop_ so in chromium ?
If not, I want to commit a patch to make message_loop_ create and destruct in same thread like :

diff --git a/base/threading/thread.cc b/base/threading/thread.cc
index cf7e313..d8ea35b 100755
--- a/base/threading/thread.cc
+++ b/base/threading/thread.cc
@@ -71,7 +71,11 @@ Thread::Thread(const std::string& name)
 }
 
 Thread::~Thread() {
+  scoped_ptr<MessageLoop> message_loop(message_loop_);
   Stop();
+  // We can't receive messages anymore.
+  // (The message loop is destructed at the end of this block)
+  message_loop_ = nullptr;
 }
 
 bool Thread::Start() {
@@ -230,7 +234,6 @@ void Thread::ThreadMain() {
 
   // Lazily initialize the message_loop so that it can run on this thread.
   DCHECK(message_loop_);
-  scoped_ptr<MessageLoop> message_loop(message_loop_);
   message_loop_->BindToCurrentThread();
   message_loop_->set_thread_name(name_);
   message_loop_->SetTimerSlack(message_loop_timer_slack_);
@@ -270,10 +273,6 @@ void Thread::ThreadMain() {
 
   // Assert that MessageLoop::Quit was called by ThreadQuitHelper.
   DCHECK(GetThreadWasQuitProperly());
-
-  // We can't receive messages anymore.
-  // (The message loop is destructed at the end of this block)
-  message_loop_ = nullptr;
 }

Best Regards
Errong Leng

[Lei Zhang]

The version of the code you are looking at is out of date. We
completely got rid of scoped_ptr in our code base over a year ago.
Your suggested patch will not apply to the code in modern Chromium.

You should look at the current code. If you still think there are
problems, please file bugs. If you like to contribute code, see:
https://www.chromium.org/developers/contributing-code

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
> ---
> You received this message because you are subscribed to the Google Groups
> "Chromium-dev" group.
> To view this discussion on the web visit
> https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/f19fa21f-a5ec-4585-8fe0-ec8a3c179e08%40chromium.org.

[冷二荣]

Sorry, My code is not latest.
In the latest code, scoped_ptr changed to std::unique_ptr.
but I mentioned issue not related to scoped_ptr,
even with std::unique_ptr, the issue still might be encountered.

在 2017年6月23日星期五 UTC+8下午4:52:01，Lei Zhang写道：

[冷二荣]

And My main question is that whether is it intend to create and destroy message_loop_ in different threads ?

在 2017年6月23日星期五 UTC+8下午5:23:32，冷二荣写道：

[Lei Zhang]

In the latest code, we have done a lot more work than just change
scoped_ptr to std::unique_ptr. Please try reading the latest code. You
may find some of the changes are of interest to you.

> https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/c5fb05e2-e4ec-444c-9214-142e16663089%40chromium.org.

[Gabriel Charette]

Yes we still destroy the MessageLoop on ThreadMain, this is intended and correct. Ownership is thread unsafe not thread affine. So long as usage is sequenced, it is safe, it doesn't need to be on the same thread. Stop() results in a join which explicitly sequences following operations after the end of ThreadMain()

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACSHbcQ3ouGpATT64u0Cp_nH5ZUJQPPVSAgbvOyVziGGazsa1w%40mail.gmail.com.