Support re-entrant calls to MessageQueueManager::Clear.

Support re-entrant calls to MessageQueueManager::Clear.

BUG=webrtc:7908
Review-Url: https://codereview.webrtc.org/2968753002
Cr-Commit-Position: refs/heads/master@{#18923}
Committed: https://chromium.googlesource.com/external/webrtc/+/5b361730d0776fba4c6f1324a9dc1efcc148cdba

[joachim, 2017-06-30 17:13:49]

jbauch@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[Taylor Brandstetter, 2017-07-05 22:41:48]

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue.cc (left):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue.cc:46: };
This still seems like a useful type of helper class, to ensure we never
increment "processing_" and forget to decrement it.

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue.cc (right):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue.cc:106: CritScope cs(&crit_);
May want to comment here that this allows a deleted Message object to cause
ClearInternal to be called again in a re-entrant fashion.

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue.h (right):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue.h:74: size_t processing_ GUARDED_BY(crit_);
It would be nice if there was a comment describing the overall approach to
re-entrancy. Maybe here would be a good place?

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue_unittest.cc (right):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue_unittest.cc:233: TEST(MessageQueueManager,
ClearReentrant) {
Can you leave a comment summarizing how this test works? For example, "When the
empty handler is destroyed, it will clear messages queued for itself. The
message to be cleared itself wraps a MessageHandler object (RefCountedHandler),
so this will cause the message queue to be cleared again in a re-entrant
fashion, which previously triggered a DCHECK."

Also, this test would be more robust if it posted a message to both handlers
(maybe to the inner one first, then outer). Right now I think we're safe,
because the message list is an std::list (which doesn't invalidate iterators
when things are removed). But we should have a test that would break if someone
were to change it to an std::vector without considering this use case.

[joachim, 2017-07-05 23:03:11]

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue.cc (left):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue.cc:46: };
On 2017/07/05 22:41:47, Taylor Brandstetter wrote:
> This still seems like a useful type of helper class, to ensure we never
> increment "processing_" and forget to decrement it.

Done (renamed to "MarkProcessingCritScope").

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue.cc (right):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue.cc:106: CritScope cs(&crit_);
On 2017/07/05 22:41:47, Taylor Brandstetter wrote:
> May want to comment here that this allows a deleted Message object to cause
> ClearInternal to be called again in a re-entrant fashion.

Done.

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue.h (right):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue.h:74: size_t processing_ GUARDED_BY(crit_);
On 2017/07/05 22:41:47, Taylor Brandstetter wrote:
> It would be nice if there was a comment describing the overall approach to
> re-entrancy. Maybe here would be a good place?

Done.

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue_unittest.cc (right):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue_unittest.cc:233: TEST(MessageQueueManager,
ClearReentrant) {
On 2017/07/05 22:41:48, Taylor Brandstetter wrote:
> Can you leave a comment summarizing how this test works? For example, "When
the
> empty handler is destroyed, it will clear messages queued for itself. The
> message to be cleared itself wraps a MessageHandler object
(RefCountedHandler),
> so this will cause the message queue to be cleared again in a re-entrant
> fashion, which previously triggered a DCHECK."

Done.

> Also, this test would be more robust if it posted a message to both handlers
> (maybe to the inner one first, then outer). Right now I think we're safe,
> because the message list is an std::list (which doesn't invalidate iterators
> when things are removed). But we should have a test that would break if
someone
> were to change it to an std::vector without considering this use case.

I don't think this is necessary. The list of message queues that is processed
doesn't change when the handler objects are destroyed - the only queue is the
Thread.

[Taylor Brandstetter, 2017-07-06 15:51:09]

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue_unittest.cc (right):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue_unittest.cc:233: TEST(MessageQueueManager,
ClearReentrant) {
On 2017/07/05 23:03:11, joachim wrote:
> On 2017/07/05 22:41:48, Taylor Brandstetter wrote:
> > Also, this test would be more robust if it posted a message to both handlers
> > (maybe to the inner one first, then outer). Right now I think we're safe,
> > because the message list is an std::list (which doesn't invalidate iterators
> > when things are removed). But we should have a test that would break if
> someone
> > were to change it to an std::vector without considering this use case.
> 
> I don't think this is necessary. The list of message queues that is processed
> doesn't change when the handler objects are destroyed - the only queue is the
> Thread.

That's what I'm talking about. The thread's message list gets modified while
it's being iterated.

[joachim, 2017-07-06 16:54:57]

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
File webrtc/rtc_base/messagequeue_unittest.cc (right):

https://codereview.webrtc.org/2968753002/diff/20001/webrtc/rtc_base/messagequ...
webrtc/rtc_base/messagequeue_unittest.cc:233: TEST(MessageQueueManager,
ClearReentrant) {
On 2017/07/06 15:51:09, Taylor Brandstetter wrote:
> On 2017/07/05 23:03:11, joachim wrote:
> > On 2017/07/05 22:41:48, Taylor Brandstetter wrote:
> > > Also, this test would be more robust if it posted a message to both
handlers
> > > (maybe to the inner one first, then outer). Right now I think we're safe,
> > > because the message list is an std::list (which doesn't invalidate
iterators
> > > when things are removed). But we should have a test that would break if
> > someone
> > > were to change it to an std::vector without considering this use case.
> > 
> > I don't think this is necessary. The list of message queues that is
processed
> > doesn't change when the handler objects are destroyed - the only queue is
the
> > Thread.
> 
> That's what I'm talking about. The thread's message list gets modified while
> it's being iterated.

Ah sorry, that was a misunderstanding then - updated the test.

[Taylor Brandstetter, 2017-07-06 21:24:56]

lgtm

[joachim, 2017-07-07 06:23:16]

The CQ bit was checked by jbauch@webrtc.org

[commit-bot: I haz the power, 2017-07-07 06:23:26]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-07 06:51:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499408596362260,
"parent_rev": "876088ac77e8a5f79e9759c8a169c44e3dc3d5bb", "commit_rev":
"5b361730d0776fba4c6f1324a9dc1efcc148cdba"}

[commit-bot: I haz the power, 2017-07-07 06:51:42]

Description was changed from

==========
Support re-entrant calls to MessageQueueManager::Clear.

BUG=webrtc:7908
==========

to

==========
Support re-entrant calls to MessageQueueManager::Clear.

BUG=webrtc:7908

Review-Url: https://codereview.webrtc.org/2968753002
Cr-Commit-Position: refs/heads/master@{#18923}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5b361730d0776fba4c6f1324a...
==========

[commit-bot: I haz the power, 2017-07-07 06:51:42]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/5b361730d0776fba4c6f1324a...