WebRTC Code Reviews
Help | Chromium Project | Sign in
(1846)

Issue 15089004: Fix a problem in Thread::Send. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 10 months ago by jiayang
Modified:
4 years, 8 months ago
CC:
webrtc-reviews_webrtc.org, rillian-moz, yujie.mao, ajm, tterriberry, qiang.lu, Niklas, leozwang1, serya, peter
Base URL:
https://webrtc.googlecode.com/svn/trunk
Visibility:
Public.

Description

Fix a problem in Thread::Send. Previously if thread A->Send is called on thread B, B->ReceiveSends will be called, which enables an arbitrary thread to invoke calls on B while B is wait for A->Send to return. This caused mutliple problems like issue 3559, 3579. The fix is to limit B->ReceiveSends to only process requests from A. Also disallow the worker thread invoking other threads. BUG=3559 R=juberti@webrtc.org Committed: https://code.google.com/p/webrtc/source/detail?r=7290

Patch Set 1 #

Total comments: 7

Patch Set 2 : #

Total comments: 6

Patch Set 3 : #

Total comments: 10

Patch Set 4 : #

Patch Set 5 : sync #

Messages

Total messages: 18 (0 generated)
jiayang
PTAL
4 years, 10 months ago (2014-08-04 23:10:24 UTC) #1
juberti_webrtc
https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h File webrtc/base/thread.h (right): https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h#newcode223 webrtc/base/thread.h:223: void set_disallowed_send_to_thread(Thread* thread) { I think we can re-use ...
4 years, 10 months ago (2014-08-04 23:26:44 UTC) #2
jiayang
https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h File webrtc/base/thread.h (right): https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h#newcode223 webrtc/base/thread.h:223: void set_disallowed_send_to_thread(Thread* thread) { On 2014/08/04 23:26:44, juberti_webrtc wrote: ...
4 years, 10 months ago (2014-08-04 23:53:49 UTC) #3
juberti_webrtc
https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h File webrtc/base/thread.h (right): https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h#newcode223 webrtc/base/thread.h:223: void set_disallowed_send_to_thread(Thread* thread) { On 2014/08/04 23:53:49, jiayang wrote: ...
4 years, 10 months ago (2014-08-05 00:27:52 UTC) #4
jiayang
PTAL https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h File webrtc/base/thread.h (right): https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h#newcode223 webrtc/base/thread.h:223: void set_disallowed_send_to_thread(Thread* thread) { On 2014/08/05 00:27:52, juberti_webrtc ...
4 years, 10 months ago (2014-08-05 16:33:28 UTC) #5
jiayang
https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h File webrtc/base/thread.h (right): https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h#newcode223 webrtc/base/thread.h:223: void set_disallowed_send_to_thread(Thread* thread) { Actually there are problems in ...
4 years, 10 months ago (2014-08-05 18:53:23 UTC) #6
serya
https://webrtc-codereview.appspot.com/15089004/diff/40001/talk/app/webrtc/peerconnectionfactory.cc File talk/app/webrtc/peerconnectionfactory.cc (right): https://webrtc-codereview.appspot.com/15089004/diff/40001/talk/app/webrtc/peerconnectionfactory.cc#newcode186 talk/app/webrtc/peerconnectionfactory.cc:186: rtc::Bind(&rtc::Thread::SetAllowBlockingCalls, worker_thread_, false)); By the way, chromium thinks that ...
4 years, 10 months ago (2014-08-07 16:41:10 UTC) #7
jiayang
On 2014/08/07 16:41:10, serya wrote: > https://webrtc-codereview.appspot.com/15089004/diff/40001/talk/app/webrtc/peerconnectionfactory.cc > File talk/app/webrtc/peerconnectionfactory.cc (right): > > https://webrtc-codereview.appspot.com/15089004/diff/40001/talk/app/webrtc/peerconnectionfactory.cc#newcode186 > ...
4 years, 10 months ago (2014-08-07 19:10:10 UTC) #8
jiayang
Justin, PTAL.
4 years, 10 months ago (2014-08-07 19:10:26 UTC) #9
jiayang
Ping Justin.
4 years, 10 months ago (2014-08-18 21:03:44 UTC) #10
juberti_webrtc
https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h File webrtc/base/thread.h (right): https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h#newcode223 webrtc/base/thread.h:223: void set_disallowed_send_to_thread(Thread* thread) { On 2014/08/05 18:53:23, jiayang wrote: ...
4 years, 9 months ago (2014-08-23 00:33:27 UTC) #11
jiayang
PTAL https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h File webrtc/base/thread.h (right): https://webrtc-codereview.appspot.com/15089004/diff/1/webrtc/base/thread.h#newcode223 webrtc/base/thread.h:223: void set_disallowed_send_to_thread(Thread* thread) { Done 1). On 2014/08/23 ...
4 years, 9 months ago (2014-08-29 00:03:33 UTC) #12
juberti_webrtc
https://webrtc-codereview.appspot.com/15089004/diff/60001/talk/app/webrtc/peerconnectionfactory.cc File talk/app/webrtc/peerconnectionfactory.cc (right): https://webrtc-codereview.appspot.com/15089004/diff/60001/talk/app/webrtc/peerconnectionfactory.cc#newcode184 talk/app/webrtc/peerconnectionfactory.cc:184: if (worker_thread_ != signaling_thread_) { ChannelManager might be a ...
4 years, 9 months ago (2014-08-29 00:41:14 UTC) #13
jiayang
PTLA https://webrtc-codereview.appspot.com/15089004/diff/60001/talk/app/webrtc/peerconnectionfactory.cc File talk/app/webrtc/peerconnectionfactory.cc (right): https://webrtc-codereview.appspot.com/15089004/diff/60001/talk/app/webrtc/peerconnectionfactory.cc#newcode184 talk/app/webrtc/peerconnectionfactory.cc:184: if (worker_thread_ != signaling_thread_) { On 2014/08/29 00:41:14, ...
4 years, 9 months ago (2014-09-02 19:52:58 UTC) #14
juberti_webrtc
lgtm with nits https://webrtc-codereview.appspot.com/15089004/diff/100001/webrtc/base/thread.cc File webrtc/base/thread.cc (right): https://webrtc-codereview.appspot.com/15089004/diff/100001/webrtc/base/thread.cc#newcode492 webrtc/base/thread.cc:492: if (it->thread == source) { You ...
4 years, 8 months ago (2014-09-22 05:07:32 UTC) #15
jiayang
https://webrtc-codereview.appspot.com/15089004/diff/100001/webrtc/base/thread.cc File webrtc/base/thread.cc (right): https://webrtc-codereview.appspot.com/15089004/diff/100001/webrtc/base/thread.cc#newcode492 webrtc/base/thread.cc:492: if (it->thread == source) { On 2014/09/22 05:07:32, juberti_webrtc ...
4 years, 8 months ago (2014-09-22 19:44:40 UTC) #16
juberti_webrtc
lgtm
4 years, 8 months ago (2014-09-22 22:51:20 UTC) #17
jiayang
4 years, 8 months ago (2014-09-24 17:14:20 UTC) #18
Message was sent while issue was closed.
Committed patchset #5 (id:140001) manually as 7290 (presubmit successful).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted