Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1666863002:
Fix race between Thread ctor/dtor and MessageQueueManager registrations. (Closed)
DescriptionFix race between Thread ctor/dtor and MessageQueueManager registrations.
This CL fixes a race where for Thread objects the parent MessageQueue
constructor registers the object in the MessageQueueManager even though
the Thread is not constructed completely yet. Same happens during
destruction.
BUG=webrtc:1225
Committed: https://crrev.com/25d1f28fa978f555d9a5a8855712db5a74d6828d
Cr-Commit-Position: refs/heads/master@{#11497}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Call "DoDestroy" from AutoThread/ComThread. #Patch Set 3 : Added unittest. #
Total comments: 2
Patch Set 4 : Added comment. #
Total comments: 1
Patch Set 5 : Added comment to MessageQueue constructor. #
Total comments: 6
Patch Set 6 : Added DoInit for initialization #Patch Set 7 : Update other Thread/MessageQueue classes. #Patch Set 8 : Reverted PS #7, updated comments #
Messages
Total messages: 27 (4 generated)
jbauch@webrtc.org changed reviewers: + pthatcher@webrtc.org
Ptal. Do you have an idea how this could be tested?
https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/messagequeue.cc#n... webrtc/base/messagequeue.cc:133: MessageQueueManager::Add(this); If ww want to call MessageQueueManager::Add before ss_->SetMessageQueue(this) in side of Thread(), why not just always do that? What's so special about Thread()? Also, why is that necessary to fix the race condition? https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (left): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#oldcode155 webrtc/base/thread.cc:155: Clear(NULL); Why do we want to not call Clear(NULL) here? Is it because we want this to be called first? SignalQueueDestroyed(); MessageQueueManager::Remove(this) ? That's the only difference I see. If we're just trying to fix the race condition, it seems at least a little dangerous to introduce another behavior change at the same time. https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (right): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#newcode151 webrtc/base/thread.cc:151: MessageQueueManager::Add(this); So, is this basically to make sure that SocketServer::SetMessageQueue is called after MessageQueueManager::Add is called? Because that's the only different that I can see. If that's the case, can you explain why that's needed? https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#newcode156 webrtc/base/thread.cc:156: DoDestroy(); Can you leave a comment like "We can't rely on the implicit call to MessageQueue::~MessageQueue because by then, the vtable will have been altered in a way that causes the call to MessageQueueManager::Remove to fail." ?
I came up with an idea for a test that doesn't exactly trigger the race but also crashes with the previous code due to a partially destroyed Thread object. Please let me know what you think.
Sorry for updating changes while you were reviewing. Please see my answers to your comments. https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/messagequeue.cc File webrtc/base/messagequeue.cc (right): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/messagequeue.cc#n... webrtc/base/messagequeue.cc:133: MessageQueueManager::Add(this); On 2016/02/03 23:51:47, pthatcher1 wrote: > If ww want to call MessageQueueManager::Add before ss_->SetMessageQueue(this) in > side of Thread(), why not just always do that? What's so special about > Thread()? > > Also, why is that necessary to fix the race condition? With this call flow: 1. Start of Thread ctor 2. MessageQueue ctor 3. Object is added to MessageQueueManager 4. Remaining Thread ctor If between 3 and 4 a different thread calls MessageQueueManager::Clear, the partially created object will be used and a race happens. https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (left): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#oldcode155 webrtc/base/thread.cc:155: Clear(NULL); On 2016/02/03 23:51:47, pthatcher1 wrote: > Why do we want to not call Clear(NULL) here? Is it because we want this to be > called first? > > SignalQueueDestroyed(); > MessageQueueManager::Remove(this) > > ? > > > That's the only difference I see. If we're just trying to fix the race > condition, it seems at least a little dangerous to introduce another behavior > change at the same time. We want to have the object removed from the MessageQueueManager before the constructor starts running. This is done by DoDestroy in the parent class. Otherwise another thread might run MessageQueueManager::Clear causing a call to Clear in this object while it is already partially destroyed. https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (right): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#newcode151 webrtc/base/thread.cc:151: MessageQueueManager::Add(this); On 2016/02/03 23:51:47, pthatcher1 wrote: > So, is this basically to make sure that SocketServer::SetMessageQueue is called > after MessageQueueManager::Add is called? Because that's the only different > that I can see. > > If that's the case, can you explain why that's needed? Please see my comment in messagequeue.cc. The change is about calling MessageQueueManager::Add after the Thread object is created completely. https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#newcode156 webrtc/base/thread.cc:156: DoDestroy(); On 2016/02/03 23:51:47, pthatcher1 wrote: > Can you leave a comment like "We can't rely on the implicit call to > MessageQueue::~MessageQueue because by then, the vtable will have been altered > in a way that causes the call to MessageQueueManager::Remove to fail." > > ? I added a similar comment at the destructor of MessageQueue.
I like the test idea. Looking forward to seeing the new comments. https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (left): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#oldcode155 webrtc/base/thread.cc:155: Clear(NULL); On 2016/02/04 00:01:20, joachim wrote: > On 2016/02/03 23:51:47, pthatcher1 wrote: > > Why do we want to not call Clear(NULL) here? Is it because we want this to be > > called first? > > > > SignalQueueDestroyed(); > > MessageQueueManager::Remove(this) > > > > ? > > > > > > That's the only difference I see. If we're just trying to fix the race > > condition, it seems at least a little dangerous to introduce another behavior > > change at the same time. > > We want to have the object removed from the MessageQueueManager before the > constructor starts running. This is done by DoDestroy in the parent class. > Otherwise another thread might run MessageQueueManager::Clear causing a call to > Clear in this object while it is already partially destroyed. That makes it clear why we should call DoDestroy() in the destructor. But why not still call Clear(NULL) before calling DoDestroy? https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (right): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#newcode156 webrtc/base/thread.cc:156: DoDestroy(); On 2016/02/04 00:01:20, joachim wrote: > On 2016/02/03 23:51:47, pthatcher1 wrote: > > Can you leave a comment like "We can't rely on the implicit call to > > MessageQueue::~MessageQueue because by then, the vtable will have been altered > > in a way that causes the call to MessageQueueManager::Remove to fail." > > > > ? > > I added a similar comment at the destructor of MessageQueue. FYI, I don't see the new comments. https://codereview.webrtc.org/1666863002/diff/40001/webrtc/base/thread_unitte... File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1666863002/diff/40001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:392: thread_->SetName("foo", nullptr); Can you leave a comment here, perhaps something like "Makes sure that if we access the Thread while it's being destroyed, that it doesn't cause a problem because the vtable has been modified."
https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (left): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#oldcode155 webrtc/base/thread.cc:155: Clear(NULL); On 2016/02/04 00:50:38, pthatcher1 wrote: > On 2016/02/04 00:01:20, joachim wrote: > > On 2016/02/03 23:51:47, pthatcher1 wrote: > > > Why do we want to not call Clear(NULL) here? Is it because we want this to > be > > > called first? > > > > > > SignalQueueDestroyed(); > > > MessageQueueManager::Remove(this) > > > > > > ? > > > > > > > > > That's the only difference I see. If we're just trying to fix the race > > > condition, it seems at least a little dangerous to introduce another > behavior > > > change at the same time. > > > > We want to have the object removed from the MessageQueueManager before the > > constructor starts running. This is done by DoDestroy in the parent class. > > Otherwise another thread might run MessageQueueManager::Clear causing a call > to > > Clear in this object while it is already partially destroyed. > > That makes it clear why we should call DoDestroy() in the destructor. But why > not still call Clear(NULL) before calling DoDestroy? DoDestroy calls Clear(NULL) internally so I think it's redundant calling it also from here. https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (right): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#newcode156 webrtc/base/thread.cc:156: DoDestroy(); On 2016/02/04 00:50:38, pthatcher1 wrote: > On 2016/02/04 00:01:20, joachim wrote: > > On 2016/02/03 23:51:47, pthatcher1 wrote: > > > Can you leave a comment like "We can't rely on the implicit call to > > > MessageQueue::~MessageQueue because by then, the vtable will have been > altered > > > in a way that causes the call to MessageQueueManager::Remove to fail." > > > > > > ? > > > > I added a similar comment at the destructor of MessageQueue. > > FYI, I don't see the new comments. Sorry for not being clearer, it's in messagequeue.h at the declaration of the dtor (adopted from a similar comment in thread.h about calling "Stop"): ----- // NOTE: ALL SUBCLASSES OF MessageQueue MUST CALL DoDestroy() IN THEIR // DESTRUCTORS! This is required to avoid a data race between the destructor // modifying the vtable, and the MessageQueue being removed from the // MessageQueueManager. ----- https://codereview.webrtc.org/1666863002/diff/40001/webrtc/base/thread_unitte... File webrtc/base/thread_unittest.cc (right): https://codereview.webrtc.org/1666863002/diff/40001/webrtc/base/thread_unitte... webrtc/base/thread_unittest.cc:392: thread_->SetName("foo", nullptr); On 2016/02/04 00:50:38, pthatcher1 wrote: > Can you leave a comment here, perhaps something like "Makes sure that if we > access the Thread while it's being destroyed, that it doesn't cause a problem > because the vtable has been modified." Done.
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
Taylor, can you also review this? I'd like another pair of eyes on it, given how subtle/tricky ctors/dtors/vtables are. https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (right): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#newcode156 webrtc/base/thread.cc:156: DoDestroy(); On 2016/02/04 01:00:09, joachim wrote: > On 2016/02/04 00:50:38, pthatcher1 wrote: > > On 2016/02/04 00:01:20, joachim wrote: > > > On 2016/02/03 23:51:47, pthatcher1 wrote: > > > > Can you leave a comment like "We can't rely on the implicit call to > > > > MessageQueue::~MessageQueue because by then, the vtable will have been > > altered > > > > in a way that causes the call to MessageQueueManager::Remove to fail." > > > > > > > > ? > > > > > > I added a similar comment at the destructor of MessageQueue. > > > > FYI, I don't see the new comments. > > Sorry for not being clearer, it's in messagequeue.h at the declaration of the > dtor (adopted from a similar comment in thread.h about calling "Stop"): > ----- > // NOTE: ALL SUBCLASSES OF MessageQueue MUST CALL DoDestroy() IN THEIR > // DESTRUCTORS! This is required to avoid a data race between the destructor > // modifying the vtable, and the MessageQueue being removed from the > // MessageQueueManager. > ----- Right, but can you put one in the ctor also explaining the need for the "don't add to manager" boolean?
Added the comment. https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc File webrtc/base/thread.cc (right): https://codereview.webrtc.org/1666863002/diff/1/webrtc/base/thread.cc#newcode156 webrtc/base/thread.cc:156: DoDestroy(); On 2016/02/04 03:14:11, pthatcher1 wrote: > On 2016/02/04 01:00:09, joachim wrote: > > On 2016/02/04 00:50:38, pthatcher1 wrote: > > > On 2016/02/04 00:01:20, joachim wrote: > > > > On 2016/02/03 23:51:47, pthatcher1 wrote: > > > > > Can you leave a comment like "We can't rely on the implicit call to > > > > > MessageQueue::~MessageQueue because by then, the vtable will have been > > > altered > > > > > in a way that causes the call to MessageQueueManager::Remove to fail." > > > > > > > > > > ? > > > > > > > > I added a similar comment at the destructor of MessageQueue. > > > > > > FYI, I don't see the new comments. > > > > Sorry for not being clearer, it's in messagequeue.h at the declaration of the > > dtor (adopted from a similar comment in thread.h about calling "Stop"): > > ----- > > // NOTE: ALL SUBCLASSES OF MessageQueue MUST CALL DoDestroy() IN THEIR > > // DESTRUCTORS! This is required to avoid a data race between the destructor > > // modifying the vtable, and the MessageQueue being removed from the > > // MessageQueueManager. > > ----- > > Right, but can you put one in the ctor also explaining the need for the "don't > add to manager" boolean? Done.
A couple questions: Is there a reason why AutoThread and ComThread call DoDestroy, but not any of the other Thread subclasses? And second, can you think of any way to test the constructor race condition? The best I can come up with is having a thread calling MessageQueueManager::Clear in a busy loop while Threads are constructed. But that's not very elegant. https://codereview.webrtc.org/1666863002/diff/60001/webrtc/base/thread.cc File webrtc/base/thread.cc (right): https://codereview.webrtc.org/1666863002/diff/60001/webrtc/base/thread.cc#new... webrtc/base/thread.cc:151: MessageQueueManager::Add(this); What if an AutoThread or ComThread is being constructed, and right after this Add, MessageQueueManager::Clear() is called on another thread? Won't you run into the same issue? In other words, don't you need "bool add_to_queue_manager = true" on this as well? https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.... webrtc/base/messagequeue.h:176: bool add_to_queue_manager = true); Instead of the "add_to_queue_manager" approach, why not have a "DoInit()" which calls MessageQueueManager::Add, to mirror "DoDestroy()"? And in MessageQueueManager::AddInternal, do nothing if a MessageQueue is added twice. That would make the rule simpler for subclasses: "Call DoInit in the constructor, and DoDestroy in the destructor" https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.... webrtc/base/messagequeue.h:181: // MessageQueueManager. Isn't it more of a race between the MessageQueue being removed from the MessageQueueManager and MessageQueueManager::Clear being called on another thread?
Thanks for your feedback. About not changing other Thread subclasses: I wanted to get some more feedback first before modifying lots of code that I probably would have to change again depending on feedback. About ctor race testcase: that also was the only idea I had so far but I didn't try yet how stable it can be triggered. I was hoping to find something more reliable. I also added a flag to the Thread ctor so subclasses can control when initialization happens. https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.... webrtc/base/messagequeue.h:176: bool add_to_queue_manager = true); On 2016/02/04 19:16:38, Taylor Brandstetter wrote: > Instead of the "add_to_queue_manager" approach, why not have a "DoInit()" which > calls MessageQueueManager::Add, to mirror "DoDestroy()"? And in > MessageQueueManager::AddInternal, do nothing if a MessageQueue is added twice. > That would make the rule simpler for subclasses: "Call DoInit in the > constructor, and DoDestroy in the destructor" Hmm, wouldn't that be the same race as before if both MessageQueue and Thread call DoInit in their constructors? 1. Thread ctor start 2. MessageQueue ctor start 3. MessageQueue ctor calls DoInit -> partial object gets registered 4. Thread ctor calls DoInit -> noop, object is already registered However a better naming instead of "add_to_queue_manager" would probably be "init_queue" and instead of calling "MessageQueueManager::Add" from the subclass call a method "DoInit" that calls MQM::Add. I uploaded a new patchset for further discussion. https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.... webrtc/base/messagequeue.h:181: // MessageQueueManager. On 2016/02/04 19:16:38, Taylor Brandstetter wrote: > Isn't it more of a race between the MessageQueue being removed from the > MessageQueueManager and MessageQueueManager::Clear being called on another > thread? Right, I updated the comment.
Looks good, as long as the other Thread classes (including JingleThreadWrapper, in Chromium) are eventually updated. https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.... webrtc/base/messagequeue.h:176: bool add_to_queue_manager = true); On 2016/02/04 20:22:31, joachim wrote: > On 2016/02/04 19:16:38, Taylor Brandstetter wrote: > > Instead of the "add_to_queue_manager" approach, why not have a "DoInit()" > which > > calls MessageQueueManager::Add, to mirror "DoDestroy()"? And in > > MessageQueueManager::AddInternal, do nothing if a MessageQueue is added twice. > > That would make the rule simpler for subclasses: "Call DoInit in the > > constructor, and DoDestroy in the destructor" > > Hmm, wouldn't that be the same race as before if both MessageQueue and Thread > call DoInit in their constructors? > 1. Thread ctor start > 2. MessageQueue ctor start > 3. MessageQueue ctor calls DoInit -> partial object gets registered > 4. Thread ctor calls DoInit -> noop, object is already registered > You're right; I wasn't thinking straight... > However a better naming instead of "add_to_queue_manager" would probably be > "init_queue" and instead of calling "MessageQueueManager::Add" from the subclass > call a method "DoInit" that calls MQM::Add. > > I uploaded a new patchset for further discussion. I like what you did in the new patchset. Though the name "init_thread" may be a little confusing.
I updated the other Thread/MessageQueue classes and changed some "virtual" to "override" on the way. How should I update JingleThreadWrapper in Chromium? Add a new CL that just adds the new parameter to the Thread ctor, land/roll that, update Chromium and then continue here, or is it fine to do that after this has landed/rolled? https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.h File webrtc/base/messagequeue.h (right): https://codereview.webrtc.org/1666863002/diff/80001/webrtc/base/messagequeue.... webrtc/base/messagequeue.h:176: bool add_to_queue_manager = true); On 2016/02/04 21:05:58, Taylor Brandstetter wrote: > On 2016/02/04 20:22:31, joachim wrote: > > On 2016/02/04 19:16:38, Taylor Brandstetter wrote: > > > Instead of the "add_to_queue_manager" approach, why not have a "DoInit()" > > which > > > calls MessageQueueManager::Add, to mirror "DoDestroy()"? And in > > > MessageQueueManager::AddInternal, do nothing if a MessageQueue is added > twice. > > > That would make the rule simpler for subclasses: "Call DoInit in the > > > constructor, and DoDestroy in the destructor" > > > > Hmm, wouldn't that be the same race as before if both MessageQueue and Thread > > call DoInit in their constructors? > > 1. Thread ctor start > > 2. MessageQueue ctor start > > 3. MessageQueue ctor calls DoInit -> partial object gets registered > > 4. Thread ctor calls DoInit -> noop, object is already registered > > > > You're right; I wasn't thinking straight... > > > However a better naming instead of "add_to_queue_manager" would probably be > > "init_queue" and instead of calling "MessageQueueManager::Add" from the > subclass > > call a method "DoInit" that calls MQM::Add. > > > > I uploaded a new patchset for further discussion. > > I like what you did in the new patchset. Though the name "init_thread" may be a > little confusing. I think I'll leave it at "init_queue" even for the Thread classes because it's the initialization of the underlying MessageQueue that is controlled through the flag.
After thinking about this more: Do we really need to call DoInit()/DoDestroy() in *all* subclasses of MessageQueue? Or is that only necessary if the subclass overrides Clear()? In other words: Is it ok if Clear is called on a partially constructed object, as long as the lowermost class that overrides "Clear" has been constructed? It seems like this should be well-defined behavior. As for updating JingleThreadWrapper, that can just be done in a follow-up CL to this one.
On 2016/02/04 23:51:02, Taylor Brandstetter wrote: > After thinking about this more: Do we really need to call DoInit()/DoDestroy() > in *all* subclasses of MessageQueue? Or is that only necessary if the subclass > overrides Clear()? > > In other words: Is it ok if Clear is called on a partially constructed object, > as long as the lowermost class that overrides "Clear" has been constructed? It > seems like this should be well-defined behavior. To avoid the race with MessageQueueManager::Clear I think it should be fine to only call the methods for classes that override "Clear". Also subscribers of "SignalQueueDestroyed" can only expect objects to have complete vtables if these objects call "DoDestroy" in their destructor. Not really a problem now in WebRTC because all subscribers just cleanup internal state on the signal. I'm fine with reverting the latest patch set and only adding that logic to classes that override "Clear" (should be only "MessageQueue" and "Thread"), it will at least fix the race described in the referenced issue. It's either your or Peter's call ;-) > As for updating JingleThreadWrapper, that can just be done in a follow-up CL to > this one. Ok
On 2016/02/05 00:10:12, joachim wrote: > On 2016/02/04 23:51:02, Taylor Brandstetter wrote: > > After thinking about this more: Do we really need to call DoInit()/DoDestroy() > > in *all* subclasses of MessageQueue? Or is that only necessary if the subclass > > overrides Clear()? > > > > In other words: Is it ok if Clear is called on a partially constructed object, > > as long as the lowermost class that overrides "Clear" has been constructed? It > > seems like this should be well-defined behavior. > > To avoid the race with MessageQueueManager::Clear I think it should be fine to > only call the methods for classes that override "Clear". > > Also subscribers of "SignalQueueDestroyed" can only expect objects to have > complete > vtables if these objects call "DoDestroy" in their destructor. Not really a > problem > now in WebRTC because all subscribers just cleanup internal state on the signal. > > I'm fine with reverting the latest patch set and only adding that logic to > classes > that override "Clear" (should be only "MessageQueue" and "Thread"), it will at > least fix the race described in the referenced issue. > > It's either your or Peter's call ;-) > > > As for updating JingleThreadWrapper, that can just be done in a follow-up CL > to > > this one. > > Ok You might as well wait to hear Peter's opinion too, but I think you can revert the last patch, and add a comment saying something like "Subclasses only need to use DoInit/DoDestroy if they override Clear." Also, sorry for changing my mind about this at the last minute...
On 2016/02/05 00:22:36, Taylor Brandstetter wrote: > On 2016/02/05 00:10:12, joachim wrote: > > On 2016/02/04 23:51:02, Taylor Brandstetter wrote: > > > After thinking about this more: Do we really need to call > DoInit()/DoDestroy() > > > in *all* subclasses of MessageQueue? Or is that only necessary if the > subclass > > > overrides Clear()? > > > > > > In other words: Is it ok if Clear is called on a partially constructed > object, > > > as long as the lowermost class that overrides "Clear" has been constructed? > It > > > seems like this should be well-defined behavior. > > > > To avoid the race with MessageQueueManager::Clear I think it should be fine to > > only call the methods for classes that override "Clear". > > > > Also subscribers of "SignalQueueDestroyed" can only expect objects to have > > complete > > vtables if these objects call "DoDestroy" in their destructor. Not really a > > problem > > now in WebRTC because all subscribers just cleanup internal state on the > signal. > > > > I'm fine with reverting the latest patch set and only adding that logic to > > classes > > that override "Clear" (should be only "MessageQueue" and "Thread"), it will at > > least fix the race described in the referenced issue. > > > > It's either your or Peter's call ;-) > > > > > As for updating JingleThreadWrapper, that can just be done in a follow-up CL > > to > > > this one. > > > > Ok > > You might as well wait to hear Peter's opinion too, but I think you can revert > the last patch, and add a comment saying something like "Subclasses only need to > use DoInit/DoDestroy if they override Clear." Also, sorry for changing my mind > about this at the last minute... I also think it makes sense to revert the changes - just did that and updated the comments.
lgtm
On 2016/02/05 02:12:42, Taylor Brandstetter wrote: > lgtm lgtm Taylor, can you submit for him (just check the Commit box)?
On 2016/02/05 02:16:27, pthatcher1 wrote: > On 2016/02/05 02:12:42, Taylor Brandstetter wrote: > > lgtm > > lgtm > > Taylor, can you submit for him (just check the Commit box)? I'll wait until tomorrow morning; I'm a little cautious about committing something at the end of the day, because it may break Chromium for some unforeseen reason, and I like to be around to fix it.
On 2016/02/05 02:16:27, pthatcher1 wrote: > On 2016/02/05 02:12:42, Taylor Brandstetter wrote: > > lgtm > > lgtm > > Taylor, can you submit for him (just check the Commit box)? Thanks, I have commit rights myself here, so I'll trigger the CQ.
The CQ bit was checked by jbauch@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666863002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666863002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fix race between Thread ctor/dtor and MessageQueueManager registrations. This CL fixes a race where for Thread objects the parent MessageQueue constructor registers the object in the MessageQueueManager even though the Thread is not constructed completely yet. Same happens during destruction. BUG=webrtc:1225 ========== to ========== Fix race between Thread ctor/dtor and MessageQueueManager registrations. This CL fixes a race where for Thread objects the parent MessageQueue constructor registers the object in the MessageQueueManager even though the Thread is not constructed completely yet. Same happens during destruction. BUG=webrtc:1225 Committed: https://crrev.com/25d1f28fa978f555d9a5a8855712db5a74d6828d Cr-Commit-Position: refs/heads/master@{#11497} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/25d1f28fa978f555d9a5a8855712db5a74d6828d Cr-Commit-Position: refs/heads/master@{#11497} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
