Fix asan test failure in P2PSocketClient::Init. Make sure P2PSocketClient::Init only access |deleg…

Fix asan test failure in P2PSocketClient::Init. Make sure  P2PSocketClient::Init only access |delegate_| on the delegate_message_loop_.

Please see the bug for more information.

BUG=150842
TEST= https://apprtc.appspot.com/?r&debug=loopback - I have not been able to reproduce the problem with asan since it will very rarely happen.

TBR=sergeyu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157944

[perkj_chrome, 2012-09-20 11:40:26]

Hi - can you please review this fix?

[tommi (sloooow) - chröme, 2012-09-20 11:48:44]

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
File content/renderer/p2p/socket_client.cc (right):

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:31:
DCHECK(delegate_message_loop_->BelongsToCurrentThread());
if the delegate and ipc threads are not always the same, then this DCHECK will
fire because you post a task to this function from the ipc thread below.

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:33: delegate_ = delegate;
...and since you post that task from the ipc thread, you end up touching this
variable from both threads always. Hence, I don't think you've fixed the
problem.

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:35: if
(!ipc_message_loop_->BelongsToCurrentThread()) {
btw, maybe you could add a note here (assuming that it's true), that
ipc_message_loop_ and delegate_message_loop_ can be the same loop?

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:38: remote_address, delegate));
in order to not use the |delegate| parameter on the other thread, can you set it
to NULL here and add documentation for why?

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:42: DCHECK_EQ(state_,
STATE_UNINITIALIZED);
and then here, you can DCHECK that delegate_ is not NULL :)

[perkj_chrome, 2012-09-20 12:22:32]

PTAL - Sorry- I sent out the first patch for review to early.

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
File content/renderer/p2p/socket_client.cc (right):

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:31:
DCHECK(delegate_message_loop_->BelongsToCurrentThread());
On 2012/09/20 11:48:44, tommi wrote:
> if the delegate and ipc threads are not always the same, then this DCHECK will
> fire because you post a task to this function from the ipc thread below.

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:33: delegate_ = delegate;
On 2012/09/20 11:48:44, tommi wrote:
> ...and since you post that task from the ipc thread, you end up touching this
> variable from both threads always. Hence, I don't think you've fixed the
> problem.

Humm- Stupid me. Sorry - should not have sent out for review before testintg and
reviewing myself.this is still the reason..

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:35: if
(!ipc_message_loop_->BelongsToCurrentThread()) {
In chrome now, they are not the same. 
But I don't think this class should have to know that or care. I remove the if
statement. 


On 2012/09/20 11:48:44, tommi wrote:
> btw, maybe you could add a note here (assuming that it's true), that
> ipc_message_loop_ and delegate_message_loop_ can be the same loop?

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:38: remote_address, delegate));
On 2012/09/20 11:48:44, tommi wrote:
> in order to not use the |delegate| parameter on the other thread, can you set
it
> to NULL here and add documentation for why?

Done.

http://codereview.chromium.org/10962010/diff/1/content/renderer/p2p/socket_cl...
content/renderer/p2p/socket_client.cc:42: DCHECK_EQ(state_,
STATE_UNINITIALIZED);
On 2012/09/20 11:48:44, tommi wrote:
> and then here, you can DCHECK that delegate_ is not NULL :)

Done.

[tommi (sloooow) - chröme, 2012-09-20 13:47:46]

lgtm. very nice.  One question below which you can address or not depending on
what's the right thing to do :)

http://codereview.chromium.org/10962010/diff/5001/content/renderer/p2p/socket...
File content/renderer/p2p/socket_client.cc (right):

http://codereview.chromium.org/10962010/diff/5001/content/renderer/p2p/socket...
content/renderer/p2p/socket_client.cc:33: delegate_ = delegate;
maybe first DCHECK(!delegate_) or is it OK for delegate_ to be valid?

[perkj_chrome, 2012-09-20 15:36:48]

Sergey- I hope you don't mind if we land while you are oo. I can do a follow up
cl next week if you have anything you want me to change.
We want to land this to make sure asan problem is fixed by this.  

On 2012/09/20 13:47:46, tommi wrote:
> lgtm. very nice.  One question below which you can address or not depending on
> what's the right thing to do :)
> 
>
http://codereview.chromium.org/10962010/diff/5001/content/renderer/p2p/socket...
> File content/renderer/p2p/socket_client.cc (right):
> 
>
http://codereview.chromium.org/10962010/diff/5001/content/renderer/p2p/socket...
> content/renderer/p2p/socket_client.cc:33: delegate_ = delegate;
> maybe first DCHECK(!delegate_) or is it OK for delegate_ to be valid?

[commit-bot: I haz the power, 2012-09-20 15:37:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/perkj@chromium.org/10962010/5001

[commit-bot: I haz the power, 2012-09-20 19:07:45]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Sergey Ulanov, 2012-09-26 16:32:38]

LGTM with a couple of minor nits.

http://codereview.chromium.org/10962010/diff/2002/content/renderer/p2p/socket...
File content/renderer/p2p/socket_client.cc (right):

http://codereview.chromium.org/10962010/diff/2002/content/renderer/p2p/socket...
content/renderer/p2p/socket_client.cc:43: DCHECK_EQ(state_,
STATE_UNINITIALIZED);
Add DCHECK here to make sure we are on IPC thread.

http://codereview.chromium.org/10962010/diff/2002/content/renderer/p2p/socket...
content/renderer/p2p/socket_client.cc:44: DCHECK(delegate_);
nit: delegate_ should be used only on the delegate thread, so it's not correct
to check it here. ASan may show warning about it.