Holding on to MessageLoop pointers

244 views
Skip to first unread message

Sanjeev Radhakrishnan

unread,
Apr 7, 2011, 1:03:56 AM4/7/11
to chromium-dev
If you find yourself caching a MessageLoop* in your class simply so you can post tasks to a specific thread or so that you can check it against the current thread like so:
DCHECK_EQ(MessageLoop::current(), message_loop_);

then, please use a scoped_refptr<MessageLoopProxy> instead. MessageLoopProxy can outlive the target thread and is a safe way to hold on to message loop pointers and leads to far less brittle code. If you care whether your PostTask actually was posted to the target thread or vanished into oblivion, then you can check the return value.

Once again, please do not cache MessageLoop pointers unless you absolutely absolutely have to (actually I can't really think of a good reason to hold on to a MessageLoop*).

Paweł Hajdan, Jr.

unread,
Apr 7, 2011, 2:19:46 AM4/7/11
to sanj...@chromium.org, chromium-dev
Can we write a clang plugin to detect cases like this? That would be cool! If it sounds good, please file a bug with TaskForce-GreenTree label and CC-me, I'm definitely interested in making this happen.

By the way, if possible, it's often simpler to just use BrowserThread IDs, like BrowserThread::UI, IO, FILE and so on.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

Sanjeev Radhakrishnan

unread,
Apr 7, 2011, 2:34:34 AM4/7/11
to Paweł Hajdan, Jr., chromium-dev
We certainly could, but unfortunately there are a lot of examples of this in the code currently which we would have to clean up. Yes, it is simpler to use BrowserThread if your code lives in the browser. If not, use a MessageLoopProxy.

Jonathan Dixon

unread,
Apr 7, 2011, 5:24:54 AM4/7/11
to sanj...@chromium.org, chromium-dev
On 7 April 2011 06:03, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:
If you find yourself caching a MessageLoop* in your class simply so you can post tasks to a specific thread or so that you can check it against the current thread like so:
DCHECK_EQ(MessageLoop::current(), message_loop_);

then, please use a scoped_refptr<MessageLoopProxy> instead. MessageLoopProxy can outlive the target thread and is a safe way to hold on to message loop pointers and leads to far less brittle code. If you care whether your PostTask actually was posted to the target thread or vanished into oblivion, then you can check the return value.

...but of you do: Note that even if the task is posted, there's no guarantee that it will run, since the target thread may already have a Quit message in its queue.


Once again, please do not cache MessageLoop pointers unless you absolutely absolutely have to (actually I can't really think of a good reason to hold on to a MessageLoop*).

--

Bernhard Bauer

unread,
Apr 7, 2011, 5:35:55 AM4/7/11
to joth+p...@google.com, Jonathan Dixon, sanj...@chromium.org, chromium-dev
On Thu, Apr 7, 2011 at 11:24, Jonathan Dixon <jo...@chromium.org> wrote:
>
>
> On 7 April 2011 06:03, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:
>>
>> If you find yourself caching a MessageLoop* in your class simply so you
>> can post tasks to a specific thread or so that you can check it against the
>> current thread like so:
>> DCHECK_EQ(MessageLoop::current(), message_loop_);
>> then, please use a scoped_refptr<MessageLoopProxy> instead.
>> MessageLoopProxy can outlive the target thread and is a safe way to hold on
>> to message loop pointers and leads to far less brittle code. If you care
>> whether your PostTask actually was posted to the target thread or vanished
>> into oblivion, then you can check the return value.
>
> ...but of you do: Note that even if the task is posted, there's no guarantee
> that it will run, since the target thread may already have a Quit message in
> its queue.
> http://src.chromium.org/viewvc/chrome/trunk/src/base/message_loop_proxy.h?revision=79524&view=markup

OTOH, even if the message loop already contains a QuitTask, it will
still run *all* tasks until it would block otherwise
(http://codesearch.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/base/message_loop.cc&l=248).

Jonathan Dixon

unread,
Apr 7, 2011, 5:45:15 AM4/7/11
to Bernhard Bauer, joth+p...@google.com, sanj...@chromium.org, chromium-dev
On 7 April 2011 10:35, Bernhard Bauer <bau...@google.com> wrote:
On Thu, Apr 7, 2011 at 11:24, Jonathan Dixon <jo...@chromium.org> wrote:
>
>
> On 7 April 2011 06:03, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:
>>
>> If you find yourself caching a MessageLoop* in your class simply so you
>> can post tasks to a specific thread or so that you can check it against the
>> current thread like so:
>> DCHECK_EQ(MessageLoop::current(), message_loop_);
>> then, please use a scoped_refptr<MessageLoopProxy> instead.
>> MessageLoopProxy can outlive the target thread and is a safe way to hold on
>> to message loop pointers and leads to far less brittle code. If you care
>> whether your PostTask actually was posted to the target thread or vanished
>> into oblivion, then you can check the return value.
>
> ...but of you do: Note that even if the task is posted, there's no guarantee
> that it will run, since the target thread may already have a Quit message in
> its queue.
> http://src.chromium.org/viewvc/chrome/trunk/src/base/message_loop_proxy.h?revision=79524&view=markup

OTOH, even if the message loop already contains a QuitTask, it will
still run *all* tasks until it would block otherwise
(http://codesearch.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/base/message_loop.cc&l=248).


Which seems to contract the documentation I quoted above. (Here in full)

  // These methods are the same as in message_loop.h, but are guaranteed to
  // either post the Task to the MessageLoop (if it's still alive), or to
  // delete the Task otherwise.
  // They return true iff the thread existed and the task was posted.  Note that
  // even if the task is posted, there's no guarantee that it will run, since
  // the target thread may already have a Quit message in its queue.
  virtual bool PostTask(const tracked_objects::Location& from_here,
                        Task* task) = 0;

William Chan (陈智昌)

unread,
Apr 7, 2011, 5:50:04 AM4/7/11
to bau...@google.com, joth+p...@google.com, Jonathan Dixon, sanj...@chromium.org, chromium-dev
On Thu, Apr 7, 2011 at 11:35 AM, Bernhard Bauer <bau...@google.com> wrote:
On Thu, Apr 7, 2011 at 11:24, Jonathan Dixon <jo...@chromium.org> wrote:
>
>
> On 7 April 2011 06:03, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:
>>
>> If you find yourself caching a MessageLoop* in your class simply so you
>> can post tasks to a specific thread or so that you can check it against the
>> current thread like so:
>> DCHECK_EQ(MessageLoop::current(), message_loop_);
>> then, please use a scoped_refptr<MessageLoopProxy> instead.
>> MessageLoopProxy can outlive the target thread and is a safe way to hold on
>> to message loop pointers and leads to far less brittle code. If you care
>> whether your PostTask actually was posted to the target thread or vanished
>> into oblivion, then you can check the return value.
>
> ...but of you do: Note that even if the task is posted, there's no guarantee
> that it will run, since the target thread may already have a Quit message in
> its queue.
> http://src.chromium.org/viewvc/chrome/trunk/src/base/message_loop_proxy.h?revision=79524&view=markup

OTOH, even if the message loop already contains a QuitTask, it will
still run *all* tasks until it would block otherwise
(http://codesearch.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/base/message_loop.cc&l=248).

Note that MessageLoop::PostTask_Helper() will always accept a task even if it can't be run. It might even be the case that MessageLoop::Run() has already finished, but the MessageLoop hasn't been destroyed yet, so the MessageLoopProxy can still try to forward to the MessageLoop. To see why this is the case, look at http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/base/threading/thread.cc&exact_package=chromium&d=3&l=164:

  {
    // The message loop for this thread.
    MessageLoop message_loop(startup_data_->options.message_loop_type);

    // Complete the initialization of our Thread object.
    thread_id_ = PlatformThread::CurrentId();
    PlatformThread::SetName(name_.c_str());
    ANNOTATE_THREAD_NAME(name_.c_str());  // Tell the name to race detector.
    message_loop.set_thread_name(name_);
    message_loop_ = &message_loop;
    message_loop_proxy_ = MessageLoopProxy::CreateForCurrentThread();

    // Let the thread do extra initialization.
    // Let's do this before signaling we are started.
    Init();

    startup_data_->event.Signal();
    // startup_data_ can't be touched anymore since the starting thread is now
    // unlocked.

    Run(message_loop_);

    // Let the thread do extra cleanup.
    CleanUp();

    // Assert that MessageLoop::Quit was called by ThreadQuitTask.
    DCHECK(GetThreadWasQuitProperly());

    // We can't receive messages anymore.
    message_loop_ = NULL;
    message_loop_proxy_ = NULL;
  }

Remember that the MessageLoopProxy uses a MessageLoop::DestructionObserver to detect that we're in ~MessageLoop(), so we should stop posting tasks. But obviously, there is space between when MessageLoop::Run() finishes and |message_loop| in base::Thread::ThreadMain() goes out of scope and is destroyed.

Bottom line, just because you post a task doesn't mean it'll get run. MessageLoopProxy::PostTask() returning false just lets you know for sure that it's not getting run :P

Jonathan Dixon

unread,
Apr 7, 2011, 5:52:58 AM4/7/11
to William Chan (陈智昌), bau...@google.com, joth+p...@google.com, sanj...@chromium.org, chromium-dev
On 7 April 2011 10:50, William Chan (陈智昌) <will...@chromium.org> wrote:

Bottom line, just because you post a task doesn't mean it'll get run. MessageLoopProxy::PostTask() returning false just lets you know for sure that it's not getting run :P


OK sounds like the essence of the comment I quoted is the same, just the rationale it gives is incorrect.

Bernhard Bauer

unread,
Apr 7, 2011, 5:57:42 AM4/7/11
to jo...@chromium.org, joth+p...@google.com, sanj...@chromium.org, chromium-dev

Yeah, I wondered about that as well. Maybe we should just remove the


"since the target thread may already have a Quit message in its queue"

part but still keep the comment that the task may not actually run (or
add Will's explanation instead)? That way, we don't create false
expectations about the reasons for not running the task.

William Chan (陈智昌)

unread,
Apr 7, 2011, 6:10:19 AM4/7/11
to bau...@google.com, jo...@chromium.org, joth+p...@google.com, sanj...@chromium.org, chromium-dev
I agree we should update the comment. Perhaps one of you fine gentlemen can put together a changelist and send it to sanjeevr? I'd do so myself but I'm busy conducting self-experiments on inebriation. I'd just delete the explanation, just emphasize that there's no guarantee that the task will be run, even if MessageLoopProxy::PostTask() returns true.

Craig Schlenter

unread,
Apr 7, 2011, 6:14:54 AM4/7/11
to sanj...@chromium.org, chromium-dev

There is some useful info here btw.:

http://www.chromium.org/developers/design-documents/threading/suble-threading-bugs-and-patterns-to-avoid-them

(and it also mentions QuitTask .. maybe it needs editing too?)

--Craig

Jonathan Dixon

unread,
Apr 7, 2011, 6:26:31 AM4/7/11
to William Chan (陈智昌), bau...@google.com, joth+p...@google.com, sanj...@chromium.org, chromium-dev
Yep, I'll take a stab at it.
Enjoy your science!

Darin Fisher

unread,
Apr 7, 2011, 1:13:18 PM4/7/11
to will...@chromium.org, bau...@google.com, joth+p...@google.com, Jonathan Dixon, sanj...@chromium.org, chromium-dev
...and this is precisely why we never had MessageLoopProxy (or something like it) in the beginning.  I wanted to see the crashes that would occur if someone tried to post to a junk MessageLoop instead of just having those get dropped on the floor.

Keep in mind that when the product crashes, you learn about it via crash reports.  But, when the product silently drops something on the floor, you don't learn about it.  Instead, some aspect of the user experience is degraded.  Maybe a user will file a meaningful bug report, or maybe not.  In my experience, crashes, generated near to the source of the problem, are more helpful to developers.

I changed my thinking on MessageLoopProxy because of how valuable BrowserThread has been.  Both work similarly.  There are times when you really don't care if messages get dropped on the floor! :-)

-Darin

Sanjeev Radhakrishnan

unread,
Apr 7, 2011, 1:30:39 PM4/7/11
to Darin Fisher, William Chan (陈智昌), bau...@google.com, joth+p...@google.com, Jonathan Dixon, chromium-dev
Reiterating what Will said: There is no difference in the guarantee of the task executing between using MessageLoop and MessageLoopProxy. What MessageLoopProxy guarantees is that it will not crash if the MessageLoop is gone.

Darin, I agree with the "it is better to crash right away rather than limp along" philosophy. However, to do that I would rather use a CHECK for the return value of MessageLoopProxy::PostTask rather than rely on the crash on accessing an invalid pointer. In fact, the main reason that prompted my email was a debugging experience I had yesterday. A unit-test was crashing strangely on one of the trybots. The crash was nowhere near any of the code I had touched, nor did it have a meaningful call stack. The problem happened because of some code in HostResolver acccessing a MessageLoop* after it had gone. It did not crash right away but caused a memory corruption that caused a crash much later in an unrelated place. The only I found this was by enabling App Verifier on the unit-test executable and running it under WinDbg. When I did this, it crashed right away at the right place. (As an aside, App Verifier is a wonderful tool for such cases on Windows, it is a pity we don't use it more often).

Darin Fisher

unread,
Apr 7, 2011, 1:43:12 PM4/7/11
to Sanjeev Radhakrishnan, William Chan (陈智昌), bau...@google.com, joth+p...@google.com, Jonathan Dixon, chromium-dev
On Thu, Apr 7, 2011 at 10:30 AM, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:
Reiterating what Will said: There is no difference in the guarantee of the task executing between using MessageLoop and MessageLoopProxy. What MessageLoopProxy guarantees is that it will not crash if the MessageLoop is gone.

True.

 

Darin, I agree with the "it is better to crash right away rather than limp along" philosophy. However, to do that I would rather use a CHECK for the return value of MessageLoopProxy::PostTask rather than rely on the crash on accessing an invalid pointer. In fact, the main reason that prompted my email was a debugging experience I had yesterday. A unit-test was crashing strangely on one of the trybots. The crash was nowhere near any of the code I had touched, nor did it have a meaningful call stack. The problem happened because of some code in HostResolver acccessing a MessageLoop* after it had gone. It did not crash right away but caused a memory corruption that caused a crash much later in an unrelated place. The only I found this was by enabling App Verifier on the unit-test executable and running it under WinDbg. When I did this, it crashed right away at the right place. (As an aside, App Verifier is a wonderful tool for such cases on Windows, it is a pity we don't use it more often).


This is also a good point.  The crash you'd get from dereferencing a junk MessageLoop is not guaranteed to be localized to the PostTask call.  Hmm...

OK, I can get behind the "use MessageLoopProxy everywhere" bandwagon :-)

I still wonder if we wouldn't be perhaps better served by generalizing the BrowserThread class, so that even the network stack could use something like it.

-Darin

John Abd-El-Malek

unread,
Apr 7, 2011, 1:44:13 PM4/7/11
to sanj...@chromium.org, Darin Fisher, William Chan (陈智昌), bau...@google.com, joth+p...@google.com, Jonathan Dixon, chromium-dev
Most of the points have already been raised, I'd just like to add that checking the result of PostTask/DeleteSoon/ReleaseSoon etc isn't a pattern that I think will help the code.  IMO it's as useful as DCHECKing/CHECKING pointers in the top of a function before using them.

The reason I don't that pattern is that it gives a false sense of security that the task has been posted.  It would be better IMO if we just took out the return value from these functions.  People use these functions because they want something to happen on another thread.  If they're told that it definitely won't, in most situations there's not much they can do.  i.e. if an object needs to be deleted on a different thread, or a method has to run on it, nothing can be done on this thread to fix that.  Pretty much all the cases where this happens in shipping code is on shutdown, when we're ok with leaks.  I would prefer that we complicate unit test code to deal with the shorter lifetime of message loops in these situations rather than modify the shipping code.

On Thu, Apr 7, 2011 at 10:30 AM, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:

Sanjeev Radhakrishnan

unread,
Apr 7, 2011, 1:49:31 PM4/7/11
to John Abd-El-Malek, Darin Fisher, William Chan (陈智昌), bau...@google.com, joth+p...@google.com, Jonathan Dixon, chromium-dev
On Thu, Apr 7, 2011 at 10:44 AM, John Abd-El-Malek <j...@chromium.org> wrote:
Most of the points have already been raised, I'd just like to add that checking the result of PostTask/DeleteSoon/ReleaseSoon etc isn't a pattern that I think will help the code.  IMO it's as useful as DCHECKing/CHECKING pointers in the top of a function before using them.

Agreed. My point above was simply comparing 2 scenarios: Crash because of accessing a junk MessageLoop* and Crash because of CHECKing the return value of MessageLoopProxy::PostTask. What I was saying is that, if you want to crash in this scenario, the second method is a more predictable way to crash. Whether you want to crash at all in these scenarios is a different issue.

John Abd-El-Malek

unread,
Apr 7, 2011, 2:06:01 PM4/7/11
to Sanjeev Radhakrishnan, Darin Fisher, William Chan (陈智昌), bau...@google.com, joth+p...@google.com, Jonathan Dixon, chromium-dev
On Thu, Apr 7, 2011 at 10:49 AM, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:

On Thu, Apr 7, 2011 at 10:44 AM, John Abd-El-Malek <j...@chromium.org> wrote:
Most of the points have already been raised, I'd just like to add that checking the result of PostTask/DeleteSoon/ReleaseSoon etc isn't a pattern that I think will help the code.  IMO it's as useful as DCHECKing/CHECKING pointers in the top of a function before using them.

Agreed. My point above was simply comparing 2 scenarios: Crash because of accessing a junk MessageLoop* and Crash because of CHECKing the return value of MessageLoopProxy::PostTask. What I was saying is that, if you want to crash in this scenario, the second method is a more predictable way to crash. Whether you want to crash at all in these scenarios is a different issue.

yep I agree that it's definitely better to use MLP instead of raw ML.  I would just like to caution people against checking the result.

William Chan (陈智昌)

unread,
Apr 7, 2011, 4:10:44 PM4/7/11
to sanj...@chromium.org, chromium-dev

Wow, seems like quite a debate started here. Lemme chime in with my point of view.

Executive summary:

MessageLoopProxy/BrowserThread take shutdown crash / heap corruption bugs and turn them into leaks (which are bugs too!). They're bandaids. Use them, but always think about how your code is supposed to behave on shutdown.

<rant>

First, all this shutdown crap is a fool's errand. The right way to handle this is to just exit the process after we've synced any important data to disk. See http://code.google.com/p/chromium/issues/detail?id=43782 for details. The problem is, no one's assigned to work on it. It's a difficult PITA (our shutdown code is absolute madness, may whatever spiritual deity pity the lunatic who tries to understand it) and fixing it isn't the most glorious of tasks. But it'd fix a whole lot of issues and speed up shutdown.

Back to reality. Posting a task where you don't know that it'll run successfully is a bug. You can't tell from MessageLoop::PostTask() or MessageLoopProxy::PostTask() if it'll run successfully as we've already noted. MessageLoopProxy/BrowserThread are designed to take these bugs, that previously caused crashes / heap corruption, and turned them into leaks. In almost all cases, this is a better user experience. It's still a leak. Something that may or may not have been important to run now is not getting run. Valgrind complains. Have you seen our valgrind suppressions file? Ugh.

MessageLoopProxy and BrowserThread are bandaids. Pretty useful ones, admittedly. DeleteOnFooThreadTraits is also a bandaid. DeleteOnUIThread is the worst of them. Guess which thread's message loop is stopped first? DeleteOnUIThread is a guaranteed leak on shutdown.

Lest I seem like I'm coming off too harsh, let me reassert that crashing is generally worse. At least we're still flushing our data to disk and not cluttering the crash reports and causing flaky tests. Only valgrind complains and the minor user experience difference is _usually_ not noticed since you're shutting down anyways, so does it really matter? In code reviews, I will almost always tell someone to use MessageLoopProxy/BrowserThread. But I will also ask them what they think is going to happen on shutdown. Do they even know the order of thread shutdown in Chrome? Generally, they haven't thought about it. This is a problem.

</rant>

On Thu, Apr 7, 2011 at 7:03 AM, Sanjeev Radhakrishnan <sanj...@chromium.org> wrote:
If you find yourself caching a MessageLoop* in your class simply so you can post tasks to a specific thread or so that you can check it against the current thread like so:
DCHECK_EQ(MessageLoop::current(), message_loop_);

then, please use a scoped_refptr<MessageLoopProxy> instead. MessageLoopProxy can outlive the target thread and is a safe way to hold on to message loop pointers and leads to far less brittle code. If you care whether your PostTask actually was posted to the target thread or vanished into oblivion, then you can check the return value.

Once again, please do not cache MessageLoop pointers unless you absolutely absolutely have to (actually I can't really think of a good reason to hold on to a MessageLoop*).

Darin Fisher

unread,
Apr 11, 2011, 1:01:48 PM4/11/11
to will...@chromium.org, sanj...@chromium.org, chromium-dev
On Thu, Apr 7, 2011 at 1:10 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

Wow, seems like quite a debate started here. Lemme chime in with my point of view.

Executive summary:

MessageLoopProxy/BrowserThread take shutdown crash / heap corruption bugs and turn them into leaks (which are bugs too!). They're bandaids. Use them, but always think about how your code is supposed to behave on shutdown.

<rant>

First, all this shutdown crap is a fool's errand. The right way to handle this is to just exit the process after we've synced any important data to disk. See http://code.google.com/p/chromium/issues/detail?id=43782 for details. The problem is, no one's assigned to work on it. It's a difficult PITA (our shutdown code is absolute madness, may whatever spiritual deity pity the lunatic who tries to understand it) and fixing it isn't the most glorious of tasks. But it'd fix a whole lot of issues and speed up shutdown.


Yes, I would really really really :) like to see that bug resolved.  Fast shutdown is in conflict though with leak checking, and I'm not sure how to deal effectively with that.

 

Back to reality. Posting a task where you don't know that it'll run successfully is a bug. You can't tell from MessageLoop::PostTask() or MessageLoopProxy::PostTask() if it'll run successfully as we've already noted. MessageLoopProxy/BrowserThread are designed to take these bugs, that previously caused crashes / heap corruption, and turned them into leaks. In almost all cases, this is a better user experience. It's still a leak. Something that may or may not have been important to run now is not getting run. Valgrind complains. Have you seen our valgrind suppressions file? Ugh.

MessageLoopProxy and BrowserThread are bandaids. Pretty useful ones, admittedly. DeleteOnFooThreadTraits is also a bandaid. DeleteOnUIThread is the worst of them. Guess which thread's message loop is stopped first? DeleteOnUIThread is a guaranteed leak on shutdown.

Lest I seem like I'm coming off too harsh, let me reassert that crashing is generally worse. At least we're still flushing our data to disk and not cluttering the crash reports and causing flaky tests. Only valgrind complains and the minor user experience difference is _usually_ not noticed since you're shutting down anyways, so does it really matter? In code reviews, I will almost always tell someone to use MessageLoopProxy/BrowserThread. But I will also ask them what they think is going to happen on shutdown. Do they even know the order of thread shutdown in Chrome? Generally, they haven't thought about it. This is a problem.


I completely agree with this last bit.  People aren't able to reason about thread shutdown order and do the right thing.  Somehow, we need to make the system guide people to the right thing.  Hmm...

-Darin

Jói Sigurðsson

unread,
Apr 11, 2011, 1:17:12 PM4/11/11
to da...@google.com, Darin Fisher, will...@chromium.org, sanj...@chromium.org, chromium-dev
Darin wrote:
> [...] Fast

> shutdown is in conflict though with leak checking, and I'm not sure how to
> deal effectively with that.

Developer/testing builds could shut down normally most of the time
(based on a flag) and we could keep e.g. a 1% per-startup randomized
experiment on our whole user base (or dev + beta) going so that we
could continue to collect problems with normal shutdown from the wild.

Cheers,
Jói

Paweł Hajdan, Jr.

unread,
Apr 11, 2011, 1:29:22 PM4/11/11
to will...@chromium.org, sanj...@chromium.org, chromium-dev
On Thu, Apr 7, 2011 at 22:10, William Chan (陈智昌) <will...@chromium.org> wrote:

MessageLoopProxy and BrowserThread are bandaids. Pretty useful ones, admittedly. DeleteOnFooThreadTraits is also a bandaid. DeleteOnUIThread is the worst of them. Guess which thread's message loop is stopped first? DeleteOnUIThread is a guaranteed leak on shutdown.

Does "guaranteed" mean it'll always leak? Also, doesn't the UI thread outlive all the other threads?

Lest I seem like I'm coming off too harsh, let me reassert that crashing is generally worse. At least we're still flushing our data to disk and not cluttering the crash reports and causing flaky tests.

nit: A task that doesn't get run can also cause flakiness, possibly more difficult to diagnose (of course a crash generally will cause the test to fail, and an unimportant leaked task rather won't). 

William Chan (陈智昌)

unread,
Apr 11, 2011, 1:33:11 PM4/11/11
to Paweł Hajdan, Jr., sanj...@chromium.org, chromium-dev
On Mon, Apr 11, 2011 at 7:29 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:
On Thu, Apr 7, 2011 at 22:10, William Chan (陈智昌) <will...@chromium.org> wrote:

MessageLoopProxy and BrowserThread are bandaids. Pretty useful ones, admittedly. DeleteOnFooThreadTraits is also a bandaid. DeleteOnUIThread is the worst of them. Guess which thread's message loop is stopped first? DeleteOnUIThread is a guaranteed leak on shutdown.

Does "guaranteed" mean it'll always leak? Also, doesn't the UI thread outlive all the other threads?

Randy asked me about this too. Yes! The UI thread outlives all other threads. But its MessageLoop is the first to stop running. After the UI MessageLoop stops running, we enter the browser_shutdown code which deletes g_browser_process which stops all other well-known threads. Note that we won't pump the UI MessageLoop again, so any tasks posted to the UI MessageLoop at this point will not be run. Hence, any DeleteOnUIThread objects will leak at this point.

Jói Sigurðsson

unread,
Apr 11, 2011, 1:46:57 PM4/11/11
to will...@chromium.org, Paweł Hajdan, Jr., sanj...@chromium.org, chromium-dev
> Randy asked me about this too. Yes! The UI thread outlives all other
> threads. [...]

I was under the impression that worker threads (via base::WorkerPool)
could outlive the UI thread, but that yes, all well-known threads have
exited their message loop before the UI thread ends.

Cheers,
Jói


On Mon, Apr 11, 2011 at 1:33 PM, William Chan (陈智昌)

William Chan (陈智昌)

unread,
Apr 11, 2011, 1:49:36 PM4/11/11
to Jói Sigurðsson, Paweł Hajdan, Jr., sanj...@chromium.org, chromium-dev
On Mon, Apr 11, 2011 at 7:46 PM, Jói Sigurðsson <j...@chromium.org> wrote:
> Randy asked me about this too. Yes! The UI thread outlives all other
> threads. [...]

I was under the impression that worker threads (via base::WorkerPool)
could outlive the UI thread, but that yes, all well-known threads have
exited their message loop before the UI thread ends.

Sorry, I neglected to add the "well-known" portion there. Thanks for the correction. Worker threads are the other threads people always forget about during shutdown.

Darin Fisher

unread,
Apr 11, 2011, 2:58:35 PM4/11/11
to Jói Sigurðsson, will...@chromium.org, sanj...@chromium.org, chromium-dev
On Mon, Apr 11, 2011 at 10:17 AM, Jói Sigurðsson <j...@chromium.org> wrote:
Darin wrote:
> [...] Fast
> shutdown is in conflict though with leak checking, and I'm not sure how to
> deal effectively with that.

Developer/testing builds could shut down normally most of the time
(based on a flag) and we could keep e.g. a 1% per-startup randomized
experiment on our whole user base (or dev + beta) going so that we
could continue to collect problems with normal shutdown from the wild.

This is good, but it means we don't get to benefit from simplifying the code
as well as improving shutdown speed.  I would like both :-)

-Darin

Darin Fisher

unread,
Apr 11, 2011, 2:59:13 PM4/11/11
to j...@chromium.org, will...@chromium.org, Paweł Hajdan, Jr., sanj...@chromium.org, chromium-dev
This is true, at least on Windows.
-Darin
Reply all
Reply to author
Forward
0 new messages