Fixing a couple cases that cause ProcessAllMessageQueues to hang.

Fixing a couple cases that cause ProcessAllMessageQueues to hang.

The two situations are:
1. A thread is in the process of shutting down, so it won't handle any
   more messages.
2. A message queue is cleared before it has a chance to process pending
   messages.

In both of those cases, we should consider processing done at that
point.

R=honghaiz@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/fe7d091f57707efdb43f9c83470203369e59cfea
Cr-Commit-Position: refs/heads/master@{#14245}

[pthatcher1, 2016-09-09 17:11:33]

pthatcher@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[pthatcher1, 2016-09-09 17:11:34]

Shouldn't we have a unit test to capture these cases that you've found?  Or is
it to difficult to write such tests?

https://codereview.webrtc.org/2319303004/diff/1/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2319303004/diff/1/webrtc/base/messagequeue.cc#n...
webrtc/base/messagequeue.cc:138: AutoDecrementer(volatile int* value) :
value_(value) {}
Why not also have the class increment in the constructor?   Then it becomes kind
of like unique_ptr/scoped_ptr, but becomes something like ScopedIncrement.

[Taylor Brandstetter, 2016-09-09 17:28:33]

It will be tricky to write unit tests, but I can probably figure something out.

https://codereview.webrtc.org/2319303004/diff/1/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2319303004/diff/1/webrtc/base/messagequeue.cc#n...
webrtc/base/messagequeue.cc:138: AutoDecrementer(volatile int* value) :
value_(value) {}
On 2016/09/09 17:11:34, pthatcher1 wrote:
> Why not also have the class increment in the constructor?   Then it becomes
kind
> of like unique_ptr/scoped_ptr, but becomes something like ScopedIncrement.  

Sure, might as well.

[honghaiz3, 2016-09-09 17:59:02]

honghaiz@webrtc.org changed reviewers:
+ honghaiz@webrtc.org

[honghaiz3, 2016-09-09 17:59:02]

https://codereview.webrtc.org/2319303004/diff/1/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2319303004/diff/1/webrtc/base/messagequeue.cc#n...
webrtc/base/messagequeue.cc:136: class AutoDecrementer : public MessageData {
Why do you replace the original Message handler with a new NoOpHandler and an
AutoDecrementer? 
You replaced two lines of code with two Classes. 
The NoOphandler is not really no op. You just hide the op in the Message
Constructor/Destructor.

[Taylor Brandstetter, 2016-09-09 23:33:12]

https://codereview.webrtc.org/2319303004/diff/1/webrtc/base/messagequeue.cc
File webrtc/base/messagequeue.cc (right):

https://codereview.webrtc.org/2319303004/diff/1/webrtc/base/messagequeue.cc#n...
webrtc/base/messagequeue.cc:136: class AutoDecrementer : public MessageData {
On 2016/09/09 17:59:02, honghaiz3 wrote:
> Why do you replace the original Message handler with a new NoOpHandler and an
> AutoDecrementer? 
> You replaced two lines of code with two Classes. 
> The NoOphandler is not really no op. You just hide the op in the Message
> Constructor/Destructor.

I tried to explain in the comment above. It's needed for the case where the
message queue is cleared (typically due to the thread being destroyed) before
the posted message is processed. This will delete the MessageData, ultimately
decrementing the count, even though the message handler isn't invoked.

As far as the name "NoOpHandler": a MessageHandler is required to delete the
MessageData to avoid memory leaks. So I guess it's not a "no op", but it's the
smallest set of necessary operations. I'm open to suggestions on a name.

[Taylor Brandstetter, 2016-09-09 23:45:52]

On 2016/09/09 23:33:12, Taylor Brandstetter wrote:
> As far as the name "NoOpHandler": a MessageHandler is required to delete the
> MessageData to avoid memory leaks. So I guess it's not a "no op", but it's the
> smallest set of necessary operations. I'm open to suggestions on a name.

Actually I was able to get rid of NoOpMessageHandler entirely. There's already
an established way of posting a message to delete something and I can just use
that.

[honghaiz3, 2016-09-10 04:14:02]

lgtm. Thanks for the explanation.

[pthatcher1, 2016-09-15 18:42:46]

lgtm

[Taylor Brandstetter, 2016-09-15 23:56:49]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2016-09-15 23:56:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Taylor Brandstetter, 2016-09-16 00:47:51]

Description was changed from

==========
Fixing a couple cases that cause ProcessAllMessageQueues to hang.

The two situations are:
1. A thread is in the process of shutting down, so it won't handle any
   more messages.
2. A message queue is cleared before it has a chance to process pending
   messages.

In both of those cases, we should consider processing done at that
point.
==========

to

==========
Fixing a couple cases that cause ProcessAllMessageQueues to hang.

The two situations are:
1. A thread is in the process of shutting down, so it won't handle any
   more messages.
2. A message queue is cleared before it has a chance to process pending
   messages.

In both of those cases, we should consider processing done at that
point.

R=honghaiz@webrtc.org, pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/fe7d091f57707efdb43f9c834...
==========

[commit-bot: I haz the power, 2016-09-16 00:47:53]

Description was changed from

==========
Fixing a couple cases that cause ProcessAllMessageQueues to hang.

The two situations are:
1. A thread is in the process of shutting down, so it won't handle any
   more messages.
2. A message queue is cleared before it has a chance to process pending
   messages.

In both of those cases, we should consider processing done at that
point.

R=honghaiz@webrtc.org, pthatcher@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/fe7d091f57707efdb43f9c834...
==========

to

==========
Fixing a couple cases that cause ProcessAllMessageQueues to hang.

The two situations are:
1. A thread is in the process of shutting down, so it won't handle any
   more messages.
2. A message queue is cleared before it has a chance to process pending
   messages.

In both of those cases, we should consider processing done at that
point.

R=honghaiz@webrtc.org, pthatcher@webrtc.org

Committed: https://crrev.com/fe7d091f57707efdb43f9c83470203369e59cfea
Cr-Commit-Position: refs/heads/master@{#14245}
==========

[Taylor Brandstetter, 2016-09-16 00:47:53]

Committed patchset #3 (id:40001) manually as
fe7d091f57707efdb43f9c83470203369e59cfea (presubmit successful).