base: Fix DCHECK when thread creation fails

base: Fix DCHECK when thread creation fails

When thread creation fails[1], base::Thread tries to delete an unbound
MessageLoop on the originating thread. If that thread already has a
MessageLoop, this trips a DCHECK in MessageLoop's destructor that checks
against having two MessageLoops on the same thread.

This patch adjusts the DCHECK logic to ignore destroying unbound message
loops on a thread that already has a separate message loop (which it was
already trying to do according to the comments).

[1] https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thread.cc&l=116

BUG=480100
Committed: https://crrev.com/3ae7dab6fb40b0fe1d5c18eda603a89a455394fd
Cr-Commit-Position: refs/heads/master@{#368057}

[Sami, 2016-01-05 15:54:40]

Description was changed from

==========
base: Fix DCHECK when thread creation fails

When thread creation fails[1], base::Thread tries to delete an unbound
MessageLoop on the originating thread. If that thread already has a
MessageLoop, this trips a DCHECK in MessageLoop's destructor that checks
against having two MessageLoops on the same thread.

This patch adjusts the DCHECK logic to ignore destroying unbound message
loops on a thread that already has a separate message loop (which it was
already trying to do according to the comments).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

BUG=480100
==========

to

==========
base: Fix DCHECK when thread creation fails

When thread creation fails[1], base::Thread tries to delete an unbound
MessageLoop on the originating thread. If that thread already has a
MessageLoop, this trips a DCHECK in MessageLoop's destructor that checks
against having two MessageLoops on the same thread.

This patch adjusts the DCHECK logic to ignore destroying unbound message
loops on a thread that already has a separate message loop (which it was
already trying to do according to the comments).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

BUG=480100
==========

[Sami, 2016-01-05 15:54:40]

skyostil@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2016-01-05 19:40:15]

https://codereview.chromium.org/1559353002/diff/1/base/message_loop/message_l...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1559353002/diff/1/base/message_loop/message_l...
base/message_loop/message_loop.cc:138: DCHECK(current() == this || !pump_);
This no longer captures that current() is either |this| or |nullptr| though, but
the comment claims current() would be NULL otherwise.

I believe that this comment is wrong and current() may be some other message
loop if pump_ is not set which means BindToCurrentThread() was never called for
this MessageLoop yet. Can you confirm and fix the comment?

[Sami, 2016-01-06 10:56:19]

https://codereview.chromium.org/1559353002/diff/1/base/message_loop/message_l...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1559353002/diff/1/base/message_loop/message_l...
base/message_loop/message_loop.cc:138: DCHECK(current() == this || !pump_);
On 2016/01/05 19:40:15, danakj (behind on reviews) wrote:
> This no longer captures that current() is either |this| or |nullptr| though,
but
> the comment claims current() would be NULL otherwise.
> 
> I believe that this comment is wrong and current() may be some other message
> loop if pump_ is not set which means BindToCurrentThread() was never called
for
> this MessageLoop yet. Can you confirm and fix the comment?

You're right, I just noticed the same thing while adding a test. I think the
main scenarios are:

1) The loop has been bound and is the current one.
2) The loop hasn't been bound and there is another current loop.
3) The loop hasn't been bound and there's no other loop either.

I've changed the comment and the logic accordingly.

[danakj, 2016-01-06 21:03:22]

LGTM w/ 1 comment

https://codereview.chromium.org/1559353002/diff/20001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1559353002/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.cc:139: DCHECK((pump_ && current() == this) ||
current() != this);
This would still pass if pump_ && current() != this. I think you want

DCHECK((pump_ && current() == this) || (!pump_ && current() != this))

ya?

https://codereview.chromium.org/1559353002/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.cc:181: if (current() == this)
Yay for tests :o

[Sami, 2016-01-07 10:46:10]

https://codereview.chromium.org/1559353002/diff/20001/base/message_loop/messa...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1559353002/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.cc:139: DCHECK((pump_ && current() == this) ||
current() != this);
On 2016/01/06 21:03:22, danakj (behind on reviews) wrote:
> This would still pass if pump_ && current() != this. I think you want
> 
> DCHECK((pump_ && current() == this) || (!pump_ && current() != this))
> 
> ya?

Ah, right you are. Fixed, thanks.

https://codereview.chromium.org/1559353002/diff/20001/base/message_loop/messa...
base/message_loop/message_loop.cc:181: if (current() == this)
On 2016/01/06 21:03:22, danakj (behind on reviews) wrote:
> Yay for tests :o

They're the best.

[Sami, 2016-01-07 10:46:13]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2016-01-07 10:46:13]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1559353002/#ps40001
(title: "Tweak assert.")

[commit-bot: I haz the power, 2016-01-07 10:46:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1559353002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1559353002/40001

[commit-bot: I haz the power, 2016-01-07 12:15:52]

Description was changed from

==========
base: Fix DCHECK when thread creation fails

When thread creation fails[1], base::Thread tries to delete an unbound
MessageLoop on the originating thread. If that thread already has a
MessageLoop, this trips a DCHECK in MessageLoop's destructor that checks
against having two MessageLoops on the same thread.

This patch adjusts the DCHECK logic to ignore destroying unbound message
loops on a thread that already has a separate message loop (which it was
already trying to do according to the comments).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

BUG=480100
==========

to

==========
base: Fix DCHECK when thread creation fails

When thread creation fails[1], base::Thread tries to delete an unbound
MessageLoop on the originating thread. If that thread already has a
MessageLoop, this trips a DCHECK in MessageLoop's destructor that checks
against having two MessageLoops on the same thread.

This patch adjusts the DCHECK logic to ignore destroying unbound message
loops on a thread that already has a separate message loop (which it was
already trying to do according to the comments).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

BUG=480100
==========

[commit-bot: I haz the power, 2016-01-07 12:15:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-07 12:16:52]

Description was changed from

==========
base: Fix DCHECK when thread creation fails

When thread creation fails[1], base::Thread tries to delete an unbound
MessageLoop on the originating thread. If that thread already has a
MessageLoop, this trips a DCHECK in MessageLoop's destructor that checks
against having two MessageLoops on the same thread.

This patch adjusts the DCHECK logic to ignore destroying unbound message
loops on a thread that already has a separate message loop (which it was
already trying to do according to the comments).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

BUG=480100
==========

to

==========
base: Fix DCHECK when thread creation fails

When thread creation fails[1], base::Thread tries to delete an unbound
MessageLoop on the originating thread. If that thread already has a
MessageLoop, this trips a DCHECK in MessageLoop's destructor that checks
against having two MessageLoops on the same thread.

This patch adjusts the DCHECK logic to ignore destroying unbound message
loops on a thread that already has a separate message loop (which it was
already trying to do according to the comments).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

BUG=480100

Committed: https://crrev.com/3ae7dab6fb40b0fe1d5c18eda603a89a455394fd
Cr-Commit-Position: refs/heads/master@{#368057}
==========

[commit-bot: I haz the power, 2016-01-07 12:16:52]

Patchset 3 (id:??) landed as
https://crrev.com/3ae7dab6fb40b0fe1d5c18eda603a89a455394fd
Cr-Commit-Position: refs/heads/master@{#368057}